Invert Normalization variable renaming for transpile-only cases

Instead of skipping variable renaming for the transpile-only case, we should be
doing the same rename-inversion that is done for full builds when variable
renaming is supposed to be disabled.

This will allow the transpilation passes that run after normalization to fully
rely on all variables being unique.

The rename inversion maintains the user's "no renaming" expectations and avoids
breaking runtime behavior that depends on function parameter names. In
particular Angular likes to use function parameter names to control injection
of parameter values at runtime.

Bonus: Fix the CompilerTestCase externs diff. It was backwards.
PiperOrigin-RevId: 551422263

Author: brad4d

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

@@ -170,18 +170,18 @@ protected PassListBuilder getTranspileOnlyPasses() {
     TranspilationPasses.addEarlyOptimizationTranspilationPasses(passes, options);
 
     // Passes below this point may rely on normalization and must maintain normalization.
-    passes.maybeAdd(
-        PassFactory.builder()
-            .setName(PassNames.NORMALIZE)
-            .setInternalFactory(
-                // Since we're doing only transpilation
-                // we want to avoid renaming all variables to be unique.
-                // Doing that would break Angular runtime behavior
-                // that injects function parameters based on their names.
-                (compiler) -> Normalize.builder(compiler).makeDeclaredNamesUnique(false).build())
-            .build());
-
+    passes.maybeAdd(normalize);
     TranspilationPasses.addPostNormalizationTranspilationPasses(passes, options);
+    // The transpilation passes may rely on normalize making all variables unique,
+    // but we're doing only transpilation, so we want to put back the original variable names
+    // wherever we can to meet user expectations.
+    //
+    // Also, if we don't do this, we could end up breaking runtime behavior that depends on specific
+    // variable names.
+    //
+    // The primary concern is function parameter names, because some frameworks, like Angular,
+    // do runtime injection of function call arguments based on the function parameter names.
+    passes.maybeAdd(invertContextualRenaming);
 
     passes.assertAllOneTimePasses();
     assertValidOrderForChecks(passes);

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

@@ -62,12 +62,10 @@ final class Normalize implements CompilerPass {
   private final AbstractCompiler compiler;
   private final AstFactory astFactory;
   private final boolean assertOnChange;
-  private final boolean makeDeclaredNamesUnique;
 
   Normalize(Builder builder) {
     this.compiler = builder.compiler;
     this.assertOnChange = builder.assertOnChange;
-    makeDeclaredNamesUnique = builder.makeDeclaredNamesUnique;
     this.astFactory = builder.compiler.createAstFactory();
   }
 
@@ -84,7 +82,6 @@ static Builder builder(AbstractCompiler compiler) {
   static class Builder {
     private final AbstractCompiler compiler;
     private boolean assertOnChange = false;
-    private boolean makeDeclaredNamesUnique = true;
 
     Builder(AbstractCompiler compiler) {
       this.compiler = compiler;
@@ -102,20 +99,6 @@ Builder assertOnChange(boolean assertOnChange) {
       return this;
     }
 
-    /**
-     * Rename variables as needed to enforce that every unique variable has a unique name.
-     *
-     * <p>This option should be disabled only for cases where no optimization is to be performed.
-     * Many optimizations rely on all variables having unique names in order to avoid having to
-     * worry about variable scoping.
-     *
-     * <p>This option is {@code true} by default.
-     */
-    public Builder makeDeclaredNamesUnique(boolean makeDeclaredNamesUnique) {
-      this.makeDeclaredNamesUnique = makeDeclaredNamesUnique;
-      return this;
-    }
-
     Normalize build() {
       return new Normalize(this);
     }
@@ -148,11 +131,9 @@ private void reportCodeChange(String changeDescription, Node n) {
 
   @Override
   public void process(Node externs, Node root) {
-    if (makeDeclaredNamesUnique) {
-      MakeDeclaredNamesUnique renamer =
-          MakeDeclaredNamesUnique.builder().withAssertOnChange(assertOnChange).build();
-      NodeTraversal.traverseRoots(compiler, renamer, externs, root);
-    }
+    MakeDeclaredNamesUnique renamer =
+        MakeDeclaredNamesUnique.builder().withAssertOnChange(assertOnChange).build();
+    NodeTraversal.traverseRoots(compiler, renamer, externs, root);
 
     NodeTraversal.traverseRoots(
         compiler, new NormalizeStatements(compiler, assertOnChange), externs, root);

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

@@ -1734,9 +1734,9 @@ private void testInternal(
 
       // Generally, externs should not be changed by the compiler passes.
       if (externsChange && !allowExternsChanges) {
-        assertNode(externsRootClone)
+        assertNode(externsRoot)
             .usingSerializer(createPrettyPrinter(compiler))
-            .isEqualTo(externsRoot);
+            .isEqualTo(externsRootClone);
       }
 
       if (checkAstChangeMarking) {
@@ -1867,18 +1867,21 @@ private void transpileToEs5(AbstractCompiler compiler, Node externsRoot, Node co
     TranspilationPasses.addTranspilationRuntimeLibraries(factories);
     TranspilationPasses.addRewritePolyfillPass(factories);
     TranspilationPasses.addEarlyOptimizationTranspilationPasses(factories, options);
-    // Transpilation requires most of normalization.
+    // Transpilation requires normalization.
     factories.maybeAdd(
         PassFactory.builder()
             .setName(PassNames.NORMALIZE)
-            .setInternalFactory(
-                abstractCompiler ->
-                    Normalize.builder(abstractCompiler)
-                        // Only do unique variable renaming if normalize is explicitly enabled.
-                        .makeDeclaredNamesUnique(normalizeEnabled)
-                        .build())
+            .setInternalFactory(abstractCompiler -> Normalize.builder(abstractCompiler).build())
             .build());
     TranspilationPasses.addPostNormalizationTranspilationPasses(factories, options);
+    // We need to put back the original variable names where possible once transpilation is
+    // complete. This matches the behavior in DefaultPassConfig. See comments there for further
+    // explanation.
+    factories.maybeAdd(
+        PassFactory.builder()
+            .setName("invertContextualRenaming")
+            .setInternalFactory(MakeDeclaredNamesUnique::getContextualRenameInverter)
+            .build());
     for (PassFactory factory : factories.build()) {
       factory.create(compiler).process(externsRoot, codeRoot);
     }

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

@@ -78,6 +78,14 @@ public void setUp() throws Exception {
     disableNormalize();
     disableTypeCheck();
     disableCompareJsDoc(); // optimization passes see simplified JSDoc.
+    // Normalization does renaming on the externs (parameter names and maybe other things) and the
+    // pass that inverts renaming ignores the externs, leaving them changed.
+    allowExternsChanges();
+    // Our getProcessor() returns a fake "pass" that actually runs several passes,
+    // including one that undoes some of the changes that were made.
+    // this confuses the logic for validating AST change marking.
+    // That logic is really only valid when testing a single, real pass.
+    disableValidateAstChangeMarking();
   }
 
   @Override
@@ -105,14 +113,17 @@ protected CompilerPass getProcessor(final Compiler compiler) {
     passes.maybeAdd(
         PassFactory.builder()
             .setName(PassNames.NORMALIZE)
-            .setInternalFactory(
-                // Disable creation of unique names to make the test cases less confusing and
-                // brittle. We're not trying to test whether Normalize will rename variables
-                // to be unique or not.
-                (abstractCompiler) ->
-                    Normalize.builder(abstractCompiler).makeDeclaredNamesUnique(false).build())
+            .setInternalFactory((abstractCompiler) -> Normalize.builder(abstractCompiler).build())
             .build());
     TranspilationPasses.addPostNormalizationTranspilationPasses(passes, compilerOptions);
+    // Since we're testing the transpile-only case, we need to put back the original variable names
+    // where possible once transpilation is complete. This matches the behavior in
+    // DefaultPassConfig. See comments there for further explanation.
+    passes.maybeAdd(
+        PassFactory.builder()
+            .setName("invertContextualRenaming")
+            .setInternalFactory(MakeDeclaredNamesUnique::getContextualRenameInverter)
+            .build());
     optimizer.consume(passes.build());
 
     return optimizer;

test/com/google/javascript/jscomp/lint/CheckArrayWithGoogObjectTest.java

@@ -53,6 +53,10 @@ protected CompilerOptions getOptions() {
   public void setUp() throws Exception {
     super.setUp();
     enableTranspile();
+    // Transpilation runs normalization, which does renaming on the externs.
+    // It also runs a pass that reverts the renaming, but it ignores the externs, leaving them
+    // changed.
+    allowExternsChanges();
     enableTypeCheck();
   }