Skip to content

Commit 273f1d6

Browse files
rahul-kamatcopybara-github
authored andcommitted
Fix how JSCompiler parses TTL expressions and modifies their lineno/charno
#### How does JSCompiler currently parse TTL expressions? JSDocInfoParser envokes a TTL parser which adjusts all the lineno/charno within JSDocs to the lineno/charno that the TTL should have within the source JS file. This doesn't work because the TTL parser is using the lineno/charno of the "@" in the "@template" JSDoc annotation and trying to add this lineno/charno to all nodes within the TTL ast. If the TTL spans multiple lines, the charno of the "@" is not the right number to add/adjust by. Also in some cases we need to use a 0-indexed lineno and other cases a 1-indexed lineno within the JSDoc to get things working. Currently, we set wrong lineno/charno to TTL ast nodes, and things work without errors by luck. When we need to print an error/warning message pointing to something in the TTL expression, there is a good chance the lineno/charno are inaccurate and the compiler crashes with a `String index out of bounds exception` #### What does this change do? Any time an error occurs within a TTL expression, show the error message with an error location pointing to the @template. This means every node in the TTL ast will have the same line/charno as that of the “@” and a length of 9 (length of “@template”). This means error messages within a TTL expression will be logged as: #### How will error messages look now? The `@template` will be underlined with `^` when an error occurs in the TTL expression. E.g: ``` WARNING: test.js:3:3: WARNING - [TYPENAME_UNDEFINED] Reference to an unknown type name q 3| * @template Q := cond(isUnknown(T), 'q', T) =: ^^^^^^^^^ ``` PiperOrigin-RevId: 500274723
1 parent e9c7a50 commit 273f1d6

File tree

2 files changed

+42
-7
lines changed

2 files changed

+42
-7
lines changed

src/com/google/javascript/jscomp/parsing/TypeTransformationParser.java

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@ public final class TypeTransformationParser {
4141
private final int templateCharno;
4242

4343
private static final int VAR_ARGS = Integer.MAX_VALUE;
44+
// Set the length of every TTL node as the length of "@template" (which is 9).
45+
// This is used for error message logging where we underline the error location with ^^^^^^^^^.
46+
private static final int TTL_NODE_LENGTH = "@template".length();
4447

4548
/** The classification of the keywords */
4649
public static enum OperationKind {
@@ -275,16 +278,31 @@ public boolean parseTypeTransformation() {
275278
// No need to add a new warning because the validation does it
276279
return false;
277280
}
278-
fixLineNumbers(expr);
281+
fixTTLNodeLineNoCharNoAndLength(expr);
279282
// Store the result if the AST is valid
280283
typeTransformationAst = expr;
281284
return true;
282285
}
283286

284-
private void fixLineNumbers(Node expr) {
285-
expr.setLinenoCharno(expr.getLineno() + templateLineno, expr.getCharno() + templateCharno);
287+
/**
288+
* Set the lineno/charno of the TTL node to the lineno/charno of the "@" in the JSDoc's template
289+
* annotation. This lineno/charno of the "@" is initialized by JsDocInfoParser when parsing TTLs.
290+
*
291+
* <p>The TTL AST currently has lineno/charno relative to its position within the JSDoc, and
292+
* `fixTTLNodeLineNoCharNoAndLength` changes this to be relative to its position in the source JS
293+
* file.
294+
*
295+
* <p>We are using the lineno/charno of "@" because adjusting the TTL AST node's lineno/charno to
296+
* be more accurate is tricky, especially for handling multiline TTL expressions. If we set an
297+
* inaccurate lineno/charno, the compiler may crash with an "index out of bounds" error when
298+
* logging errors that occur in the TTL expression. Instead, we will use the lineno/charno of "@"
299+
* to safely highlight which JSDoc annotation an error occurs in.
300+
*/
301+
private void fixTTLNodeLineNoCharNoAndLength(Node expr) {
302+
expr.setLinenoCharno(templateLineno, templateCharno);
303+
expr.setLength(TTL_NODE_LENGTH);
286304
for (Node child = expr.getFirstChild(); child != null; child = child.getNext()) {
287-
fixLineNumbers(child);
305+
fixTTLNodeLineNoCharNoAndLength(child);
288306
}
289307
}
290308

test/com/google/javascript/jscomp/parsing/JsDocInfoParserTest.java

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3910,14 +3910,31 @@ public void testBug16129690() {
39103910
}
39113911

39123912
@Test
3913-
public void testTTLLineNumber() {
3913+
public void testTTLLineNoCharNo() {
39143914
JSDocInfo info =
39153915
parse(
39163916
LINE_JOINER.join(
3917-
"Some text on line 1", "More text! This is line 2", "@template T := foo =:*/"));
3917+
"Some text on line 0", "More text! This is line 1", "@template T := foo =:*/"));
39183918
assertThat(info.getTypeTransformations()).hasSize(1);
39193919
Node n = info.getTypeTransformations().get("T");
3920-
assertNode(n).hasLineno(3);
3920+
assertNode(n).hasLineno(2); // lineno of "@" in "@template"
3921+
assertNode(n).hasCharno(0); // charno of "@" in "@template"
3922+
}
3923+
3924+
@Test
3925+
public void testMultilineTTLLineNoCharNo() {
3926+
JSDocInfo info =
3927+
parse(
3928+
LINE_JOINER.join(
3929+
"Some text on line 0",
3930+
"More text! This is line 1",
3931+
"@template T :=",
3932+
" foo",
3933+
"=:*/"));
3934+
assertThat(info.getTypeTransformations()).hasSize(1);
3935+
Node n = info.getTypeTransformations().get("T");
3936+
assertNode(n).hasLineno(2); // lineno of "@" in "@template"
3937+
assertNode(n).hasCharno(0); // charno of "@" in "@template"
39213938
}
39223939

39233940
@Test

0 commit comments

Comments
 (0)