Skip to content

Commit

Permalink
Improve parsing of closure-unaware code to only suppress parse errors…
Browse files Browse the repository at this point in the history 
… inside the relevant source ranges, instead of any parse error within the scripts annotated with @closureUnaware.

PiperOrigin-RevId: 704755116
  • Loading branch information
@12wrigja @copybara-github
12wrigja authored and copybara-github committed Dec 10, 2024
1 parent 6f54038 commit 3c7f64b
Show file tree
Hide file tree
Showing 4 changed files with 259 additions and 7 deletions.
13 changes: 13 additions & 0 deletions src/com/google/javascript/jscomp/ManageClosureUnawareCode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,19 @@
*
* <p>ManageClosureUnawareCode.unwrap() should be run after all other passes have run, to unwrap the
* code and re-expose it to the code-printing stage of the compiler.
*
* <p>Valid "closure-unaware" scripts should:
* <li>be explicitly annotated with a @fileoverview JSDoc comment that also has the @closureUnaware
* JSDoc tag. This is done so that checking whether an arbitrary node is closure-unaware is a
* very quick operation (as the JSDoc from the SCRIPT node determines whether a bit is set on
* the relevant SourceFile object for those AST nodes), and this higher-level check is used in
* several places in the compiler to avoid reporting various errors.
* <li>annotate each expression within the script that is really "closure-unaware" with a JSDoc
* comment with the @closureUnaware JSDoc tag. Currently, these comments must be attached to
* FUNCTION nodes, and the AST inside the BLOCK child node is considered the closure-unaware
* code. This allows the compiler to differentiate between the parts of the AST containing code
* that has to be "closure-aware" (such as closure module system constructs, assertions that
* should optimize away, etc) from the code that is actually closure-unaware.
*/
final class ManageClosureUnawareCode implements CompilerPass {

Expand Down
59 changes: 53 additions & 6 deletions src/com/google/javascript/jscomp/parsing/IRFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@
import com.google.javascript.rhino.dtoa.DToA;
import java.math.BigInteger;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Deque;
import java.util.LinkedHashSet;
import java.util.List;
Expand Down Expand Up @@ -244,7 +245,7 @@ class IRFactory {

private FeatureSet features = FeatureSet.BARE_MINIMUM;
private Node resultNode;
private boolean isClosureUnawareCode = false;
private final ArrayList<SourceRange> closureUnawareCodeRanges = new ArrayList<>();

private IRFactory(
StaticSourceFile sourceFile,
Expand Down Expand Up @@ -647,9 +648,6 @@ private boolean handlePossibleFileOverviewJsDoc(JsDocInfoParser jsDocParser) {
}

JSDocInfo newFileoverview = jsDocParser.getFileOverviewJSDocInfo();
if (newFileoverview != null && newFileoverview.isClosureUnawareCode()) {
this.isClosureUnawareCode = true;
}
if (identical(newFileoverview, this.firstFileoverview)) {
return false;
}
Expand Down Expand Up @@ -868,6 +866,10 @@ Node transform(ParseTree tree) {
JSDocInfo info = parseJSDocInfoOnTree(tree);
NonJSDocComment comment = parseNonJSDocCommentAt(tree.getStart(), false);

if (info != null && info.isClosureUnawareCode()) {
this.closureUnawareCodeRanges.add(tree.location);
}

Node node = transformDispatcher.process(tree);

if (info != null) {
Expand All @@ -881,6 +883,49 @@ Node transform(ParseTree tree) {
return node;
}

private boolean withinClosureUnawareCodeRange(int line, int lineColumnNo) {
// TODO: b/331840742: Improve the performance of this check, as naive O(n) is not ideal when
// there are a small number of ranges but many checks against those ranges.
for (SourceRange range : this.closureUnawareCodeRanges) {
if (withinRange(range, line, lineColumnNo)) {
return true;
}
}
return false;
}

private final boolean withinRange(SourceRange range, int line, int lineColumnNo) {
int startLine = range.start.line;
int startColumnNo = range.start.column;
int endLine = range.end.line;
int endColumnNo = range.end.column;
// range starts after line
if (afterPosition(startLine, startColumnNo, line, lineColumnNo, true)) {
return false;
}

// range ends before line
if (afterPosition(line, lineColumnNo, endLine, endColumnNo, false)) {
return false;
}

return true;
}

private boolean afterPosition(
int aLine, int aLineColumnNo, int bLine, int bColumnNo, boolean inclusive) {
if (aLine > bLine) {
return true;
}
if (aLine < bLine) {
return false;
}
if (inclusive && aLineColumnNo == bColumnNo) {
return true;
}
return aLineColumnNo > bColumnNo;
}

private Node maybeInjectCastNode(ParseTree node, JSDocInfo info, Node irNode) {
if (node.type == ParseTreeType.PAREN_EXPRESSION && info.hasType()) {
irNode = newNode(Token.CAST, irNode);
Expand Down Expand Up @@ -1084,15 +1129,17 @@ private ClosureUnawareCodeSkippingJsDocInfoErroReporter(

@Override
public void error(String message, String sourceName, int line, int lineOffset) {
if (host.isClosureUnawareCode) {
// Line numbers are 1-indexed, but the SourcePosition uses 0-indexed.
if (host.withinClosureUnawareCodeRange(line - 1, lineOffset)) {
return;
}
delegate.error(message, sourceName, line, lineOffset);
}

@Override
public void warning(String message, String sourceName, int line, int lineOffset) {
if (host.isClosureUnawareCode) {
// Line numbers are 1-indexed, but the SourcePosition uses 0-indexed.
if (host.withinClosureUnawareCodeRange(line - 1, lineOffset)) {
return;
}
delegate.warning(message, sourceName, line, lineOffset);
Expand Down
25 changes: 25 additions & 0 deletions test/com/google/javascript/jscomp/ManageClosureUnawareCodeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,4 +394,29 @@ public void testAllowsSpecifyingAnnotationOnIIFE() {
"goog.module('foo.bar.baz_raw');",
"$jscomp_wrap_closure_unaware_code('{window[\"foo\"]=5}')"));
}

@Test
public void testReparseWithInvalidJSDocTags() {
// @dependency is not a valid JSDoc tag, and for normal code we would issue a parser error.
// However, for closure-unaware code we don't care at all what is in jsdoc comments.
doTest(
lines(
"/**",
" * @fileoverview",
" * @closureUnaware",
" */",
"goog.module('foo.bar.baz_raw');",
"/** @closureUnaware */",
"(function() {",
" /** @dependency */",
" window['foo'] = 5;",
"}).call(globalThis);"),
lines(
"/**",
" * @fileoverview",
" * @closureUnaware",
" */",
"goog.module('foo.bar.baz_raw');",
"$jscomp_wrap_closure_unaware_code('{window[\"foo\"]=5}')"));
}
}
169 changes: 168 additions & 1 deletion test/com/google/javascript/jscomp/integration/ClosureUnawareCodeIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,6 @@ public void testNoOptimizeClosureUnawareCode_parsesCodeWithNonClosureJsDoc() {
CompilerOptions options = createCompilerOptions();
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
CompilationLevel.ADVANCED_OPTIMIZATIONS.setTypeBasedOptimizationOptions(options);
options.setLanguageOut(LanguageMode.ECMASCRIPT_2018);

externs =
ImmutableList.<SourceFile>builder()
Expand All @@ -492,12 +491,180 @@ public void testNoOptimizeClosureUnawareCode_parsesCodeWithNonClosureJsDoc() {
"(function() {",
" /**",
" * @prop {number} a - scale x",
// @defaults isn't a valid jsdoc tag, but within the closure-unaware portions of the
// AST we should not raise a warning.
" * @defaults",
" */",
" const x = 5;",
"}).call(globalThis);"),
lines("(function() {", " const x = 5;", "}).call(globalThis);"));
}

@Test
public void
testNoOptimizeClosureUnawareCode_parsesCodeWithNonClosureJsDoc_whenNotAttachedToNode() {
CompilerOptions options = createCompilerOptions();
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
CompilationLevel.ADVANCED_OPTIMIZATIONS.setTypeBasedOptimizationOptions(options);

externs =
ImmutableList.<SourceFile>builder()
.addAll(externs)
.add(
SourceFile.fromCode(
"globalthis.js", lines("/**", " * @type {?}", " */", "var globalThis;")))
.build();

// this test-case is trying to ensure that even when there are jsdoc comments in the
// closure-unaware portions of the AST that aren't attached to specific nodes / expressions
// that we can still ignore invalid jsdoc contents.
// @defaults isn't a valid jsdoc tag, and outside of the closure-unaware portions of the AST we
// should raise a warning.

test(
options,
lines(
"/**",
" * @fileoverview",
" * @closureUnaware",
" */",
"goog.module('a.b');",
"/** @closureUnaware */",
"(function() {",
" /** @defaults */", // JSDoc comment not attached to any node
" /**",
" * @prop {number} a - scale x",
" * @defaults",
" */",
" const x = 5;",
"}).call(globalThis);"),
lines("(function() {", " const x = 5;", "}).call(globalThis);"));
}

@Test
public void
testNoOptimizeClosureUnawareCode_parsesCodeWithNonClosureJsDoc_whenNotAttachedToNode_typedJsDoc() {
CompilerOptions options = createCompilerOptions();
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
CompilationLevel.ADVANCED_OPTIMIZATIONS.setTypeBasedOptimizationOptions(options);

externs =
ImmutableList.<SourceFile>builder()
.addAll(externs)
.add(
SourceFile.fromCode(
"globalthis.js", lines("/**", " * @type {?}", " */", "var globalThis;")))
.build();

// this test-case is trying to ensure that even when there are jsdoc comments in the
// closure-unaware portions of the AST that aren't attached to specific nodes / expressions
// that we can still ignore invalid jsdoc contents.
// @type [string] does not parse as a valid "typed jsdoc" tag, and this follows a different
// parsing codepath than non-type jsdoc tags and raises a different error
// than an entirely unknown jsdoc tag.

test(
options,
lines(
"/**",
" * @fileoverview",
" * @closureUnaware",
" */",
"goog.module('a.b');",
"/** @closureUnaware */",
"(function() {",
" /** @type [string] */",
"",
" /**",
" * @prop {number} a - scale x",
" * @defaults",
" */",
" const x = 5;",
"}).call(globalThis);"),
lines("(function() {", " const x = 5;", "}).call(globalThis);"));
}

@Test
public void
testNoOptimizeClosureUnawareCode_doesntSuppressParseErrorsOutsideClosureUnawareBlocks() {
CompilerOptions options = createCompilerOptions();
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
CompilationLevel.ADVANCED_OPTIMIZATIONS.setTypeBasedOptimizationOptions(options);

externs =
ImmutableList.<SourceFile>builder()
.addAll(externs)
.add(
SourceFile.fromCode(
"globalthis.js", lines("/**", " * @type {?}", " */", "var globalThis;")))
.build();

// this test-case is trying to ensure that even when the special handling for JSDoc comments in
// closureUnaware code we can still report jsdoc errors for code that is not within the
// closure-unaware subsection of the AST.
test(
options,
lines(
"/**",
" * @fileoverview",
" * @closureUnaware",
" */",
"goog.module('a.b');",
// This is invalid JSDoc, but it isn't within the subtree of a node annotated as
// `@closureUnaware`.
// NOTE: The `@closureUnaware` in the `@fileoverview` comment does not apply this
// subtree suppression
// mechanism to the whole script - it is only to indicate that the file contains some
// closure-unaware code
// (e.g. a performance optimization).
"/**",
" * @prop {number} a - scale x",
" */",
"/** @closureUnaware */",
"(function() {",
" const x = 5;",
"}).call(globalThis);"),
DiagnosticGroups.NON_STANDARD_JSDOC);
}

@Test
public void
testNoOptimizeClosureUnawareCode_doesntSuppressParseErrorsOutsideClosureUnawareBlocks_afterRange() {
CompilerOptions options = createCompilerOptions();
CompilationLevel.ADVANCED_OPTIMIZATIONS.setOptionsForCompilationLevel(options);
CompilationLevel.ADVANCED_OPTIMIZATIONS.setTypeBasedOptimizationOptions(options);

externs =
ImmutableList.<SourceFile>builder()
.addAll(externs)
.add(
SourceFile.fromCode(
"globalthis.js", lines("/**", " * @type {?}", " */", "var globalThis;")))
.build();

// this test-case is trying to ensure that even when the special handling for JSDoc comments in
// closureUnaware code we can still report jsdoc errors for code that is not within the
// closure-unaware subsection of the AST.
// Specifically this test validates that any comments that come after a closure-unaware
// subsection of the AST don't have their jsdoc parse errors suppressed.
test(
options,
lines(
"/**",
" * @fileoverview",
" * @closureUnaware",
" */",
"goog.module('a.b');",
"/** @closureUnaware */",
"(function() {",
" const x = 5;",
"}).call(globalThis);",
"/**",
" * @prop {number} a - scale x",
" */",
""),
DiagnosticGroups.NON_STANDARD_JSDOC);
}
// TODO how can I test whether source info is being properly retained?
// TODO if there is a sourcemap comment in the IIFE, can we use that info somehow?
}

0 comments on commit 3c7f64b

Please sign in to comment.