Avoiding rerunning ProcessDefines in J2clUtilGetDefineRewriterPass

The way `J2clUtilGetDefineRewriterPass` currently works is pretty messy as the
rerun of `ProcessDefines` does not work as one would expect. When we actually
rerun it all of the `goog.define` calls have already been removed, so the define
name is actually based on the name of the node annotated with `@define`.

Instead of doing this we can have `ProcessDefines` stash the name of all the
defines that it found. That's the only information that
`J2clUtilGetDefineRewriterPass` needs to operate, and no new defines are going
to be added along the way.

This does not change the requirement that the J2CL requires the define name to
match a `goog.provide` of the same name. Resolving that problem is quite a bit
more involved. For now this change just improves the improves the performance
and reduces the brittleness of the existing solution.

PiperOrigin-RevId: 700492292

Author: kevinoconnor7

src/com/google/javascript/jscomp/AbstractCompiler.java

@@ -46,6 +46,7 @@
 import java.io.Serializable;
 import java.nio.file.Path;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.EnumSet;
 import java.util.List;
 import java.util.Set;
@@ -116,6 +117,11 @@ void afterPass(String passName) {}
   /** Gets the names that have been exported. */
   public abstract Set<String> getExportedNames();
 
+  /** Adds @define names to keep track. */
+  public abstract void setDefineNames(Collection<String> defineNames);
+
+  public abstract ImmutableSet<String> getDefineNames();
+
   /** Sets the variable renaming map */
   public abstract void setVariableMap(VariableMap variableMap);
 

src/com/google/javascript/jscomp/Compiler.java

@@ -3610,6 +3610,9 @@ public SourceMap getSourceMap() {
   /** Names exported by goog.exportSymbol. */
   private final LinkedHashSet<String> exportedNames = new LinkedHashSet<>();
 
+  /** Names defined by goog.define. */
+  private ImmutableSet<String> defineNames = ImmutableSet.of();
+
   @Override
   public void setVariableMap(VariableMap variableMap) {
     this.variableMap = variableMap;
@@ -3685,6 +3688,16 @@ public Set<String> getExportedNames() {
     return exportedNames;
   }
 
+  @Override
+  public void setDefineNames(Collection<String> defineNames) {
+    this.defineNames = ImmutableSet.copyOf(defineNames);
+  }
+
+  @Override
+  public ImmutableSet<String> getDefineNames() {
+    return defineNames;
+  }
+
   @Override
   public CompilerOptions getOptions() {
     return options;

src/com/google/javascript/jscomp/DefaultPassConfig.java

@@ -1306,7 +1306,7 @@ private void assertValidOrderForOptimizations(PassListBuilder optimizations) {
         processDefinesOptimize,
         j2clUtilGetDefineRewriterPass,
         "J2CL define re-writing should be done after processDefines since it relies on "
-            + "collectDefines which has side effects.");
+            + "Compiler#getDefineNames to have been populated by it.");
 
     optimizations.assertPassOrder(
         removeUnusedCode,

src/com/google/javascript/jscomp/J2clUtilGetDefineRewriterPass.java

@@ -35,11 +35,7 @@ public void process(Node externs, Node root) {
     if (!J2clSourceFileChecker.shouldRunJ2clPasses(compiler)) {
       return;
     }
-    defines =
-        new ProcessDefines.Builder(compiler)
-            .setMode(ProcessDefines.Mode.OPTIMIZE)
-            .build()
-            .collectDefineNames(externs, root);
+    defines = compiler.getDefineNames();
     NodeTraversal.traverse(compiler, root, this);
   }
 

src/com/google/javascript/jscomp/ProcessDefines.java

@@ -208,13 +208,6 @@ public void process(Node externs, Node root) {
     this.overrideDefines();
   }
 
-  final ImmutableSet<String> collectDefineNames(Node externs, Node root) {
-    this.initNamespace(externs, root);
-    this.collectDefines(root);
-
-    return ImmutableSet.copyOf(this.defineByDefineName.keySet());
-  }
-
   private void initNamespace(Node externs, Node root) {
     if (namespaceSupplier != null) {
       this.namespace = namespaceSupplier.get();
@@ -352,6 +345,7 @@ private void collectDefines(Node root) {
         }
       }
     }
+    compiler.setDefineNames(defineByDefineName.keySet());
   }
 
   private @Nullable Ref selectDefineDeclaration(Name name) {

test/com/google/javascript/jscomp/J2clUtilGetDefineRewriterPassTest.java

@@ -15,6 +15,7 @@
  */
 package com.google.javascript.jscomp;
 
+import com.google.javascript.jscomp.ProcessDefines.Mode;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -25,7 +26,12 @@ public class J2clUtilGetDefineRewriterPassTest extends CompilerTestCase {
 
   @Override
   protected CompilerPass getProcessor(final Compiler compiler) {
-    return new J2clUtilGetDefineRewriterPass(compiler);
+    return (externs, root) -> {
+      // We require the ProcessDefines pass to run first, so run it in optimize mode so that we
+      // replicate the state we expect the code to be in when it reaches this pass.
+      new ProcessDefines.Builder(compiler).setMode(Mode.OPTIMIZE).build().process(externs, root);
+      new J2clUtilGetDefineRewriterPass(compiler).process(externs, root);
+    };
   }
 
   @Override
@@ -37,13 +43,28 @@ protected Compiler createCompiler() {
 
   @Test
   public void testUtilGetDefine() {
-    String defineAbc = "var a={}; a.b={}; /** @define {boolean} */ a.b.c = true;\n";
     test(
-        defineAbc + "nativebootstrap.Util.$getDefine('a.b.c', 'def');",
-        defineAbc + "('def', String(a.b.c));");
+        lines(
+            "var a = {};",
+            "a.b = {}",
+            "/** @define {boolean} */ a.b.c = goog.define('a.b.c', true);",
+            "nativebootstrap.Util.$getDefine('a.b.c', 'def');"),
+        lines(
+            "var a = {};",
+            "a.b = {}",
+            "/** @define {boolean} */ a.b.c = true;",
+            "('def', String(a.b.c));"));
     test(
-        defineAbc + "nativebootstrap.Util.$getDefine('a.b.c');",
-        defineAbc + "(null, String(a.b.c));");
+        lines(
+            "var a = {};",
+            "a.b = {}",
+            "/** @define {boolean} */ a.b.c = goog.define('a.b.c', true);",
+            "nativebootstrap.Util.$getDefine('a.b.c');"),
+        lines(
+            "var a = {};",
+            "a.b = {}",
+            "/** @define {boolean} */ a.b.c = true;",
+            "(null, String(a.b.c));"));
   }
 
   @Test