Improve parsing of closure-unaware code to only suppress parse errors inside the relevant source ranges, instead of any parse error within the scripts annotated with @closureUnaware.

12wrigja · copybara-github · commit 3c7f64bc5e2c · 2024-12-10T10:18:11.000-08:00
PiperOrigin-RevId: 704755116
diff --git a/src/com/google/javascript/jscomp/ManageClosureUnawareCode.java b/src/com/google/javascript/jscomp/ManageClosureUnawareCode.java
@@ -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 {
 
diff --git a/src/com/google/javascript/jscomp/parsing/IRFactory.java b/src/com/google/javascript/jscomp/parsing/IRFactory.java
@@ -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;
@@ -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,
@@ -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;
     }
@@ -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) {
@@ -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);
@@ -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);
diff --git a/test/com/google/javascript/jscomp/ManageClosureUnawareCodeTest.java b/test/com/google/javascript/jscomp/ManageClosureUnawareCodeTest.java
@@ -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}')"));
+  }
 }
diff --git a/test/com/google/javascript/jscomp/integration/ClosureUnawareCodeIntegrationTest.java b/test/com/google/javascript/jscomp/integration/ClosureUnawareCodeIntegrationTest.java
@@ -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()
@@ -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?
 }
