Modify PureFunctionIdentifier to run under OptimizeCalls.

Doing so eliminates it's dependence (the last in the compiler) on `NameBasedDefinitionProvider`.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=242210304

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

@@ -100,6 +100,7 @@ public Builder addPass(CallGraphCompilerPass pass) {
     }
 
     public OptimizeCalls build() {
+      checkNotNull(compiler);
       checkNotNull(considerExterns);
 
       return new OptimizeCalls(compiler, passes.build(), considerExterns);
@@ -412,6 +413,7 @@ public void visit(NodeTraversal t, Node n, Node unused) {
 
     private void maybeAddNameReference(Node n) {
       String name = n.getString();
+      // TODO(b/129503101): Why are we limiting ourselves to global names?
       Var var = globalScope.getSlot(name);
       if (var != null && (considerExterns || !var.isExtern())) {
         // As every name declaration is unique due to normalizations, it is only necessary to build

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

@@ -62,10 +62,7 @@ protected CompilerPass getProcessor(final Compiler compiler) {
 
       @Override
       public void process(Node externs, Node root) {
-        NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler, true);
-        defFinder.process(externs, root);
-
-        new PureFunctionIdentifier(compiler, defFinder).process(externs, root);
+        new PureFunctionIdentifier.Driver(compiler).process(externs, root);
         new RemoveUnusedCode.Builder(compiler)
             .removeLocalVars(true)
             .removeGlobals(true)

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

@@ -46,9 +46,8 @@ protected CompilerPass getProcessor(final Compiler compiler) {
     return new CompilerPass() {
       @Override
       public void process(Node externs, Node root) {
-        NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler, true);
-        defFinder.process(externs, root);
-        new PureFunctionIdentifier(compiler, defFinder).process(externs, root);
+        new PureFunctionIdentifier.Driver(compiler).process(externs, root);
+
         PeepholeOptimizationsPass peepholePass =
             new PeepholeOptimizationsPass(compiler, getName(), new PeepholeRemoveDeadCode());
         peepholePass.process(externs, root);

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

@@ -199,8 +199,9 @@ protected int getNumRepetitions() {
   public void setUp() throws Exception {
     super.setUp();
 
-    enableTypeCheck();
     enableNormalize();
+    enableGatherExternProperties();
+    enableTypeCheck();
   }
 
   @Override
@@ -228,13 +229,8 @@ public void process(Node externs, Node root) {
       localResultCalls = new ArrayList<>();
       compiler.setHasRegExpGlobalReferences(regExpHaveSideEffects);
       compiler.getOptions().setUseTypesForLocalOptimization(true);
-      NameBasedDefinitionProvider defFinder = new NameBasedDefinitionProvider(compiler, true);
-      defFinder.process(externs, root);
-
-      PureFunctionIdentifier pureFunctionIdentifier =
-          new PureFunctionIdentifier(compiler, defFinder);
-      pureFunctionIdentifier.process(externs, root);
 
+      new PureFunctionIdentifier.Driver(compiler).process(externs, root);
       NodeTraversal.traverse(compiler, externs, this);
       NodeTraversal.traverse(compiler, root, this);
     }
@@ -566,24 +562,33 @@ public void testSharedFunctionName2() {
 
   @Test
   public void testAnnotationInExternStubs1() {
-    assertPureCallsMarked("o.propWithStubBefore('a');",
-        ImmutableList.of("o.propWithStubBefore"));
+    // In the case where a property is defined first as a stub and then with a FUNCTION:
+    // we have to make a conservative assumption about the behaviour of the extern, since the stub
+    // carried no information.
+    assertNoPureCalls("o.propWithStubBefore('a');");
   }
 
   @Test
   public void testAnnotationInExternStubs1b() {
-    assertPureCallsMarked("o.propWithStubBeforeWithJSDoc('a');",
-        ImmutableList.of("o.propWithStubBeforeWithJSDoc"));
+    // In the case where a property is defined first as a stub and then with a FUNCTION:
+    // we have to make a conservative assumption about the behaviour of the extern, since the stub
+    // carried no information.
+    assertNoPureCalls("o.propWithStubBeforeWithJSDoc('a');");
   }
 
   @Test
   public void testAnnotationInExternStubs2() {
-    assertPureCallsMarked("o.propWithStubAfter('a');",
-        ImmutableList.of("o.propWithStubAfter"));
+    // In the case where a property is defined first with a FUNCTION and then as a stub:
+    // we have to make a conservative assumption about the behaviour of the extern, since the stub
+    // carried no information.
+    assertNoPureCalls("o.propWithStubAfter('a');");
   }
 
   @Test
   public void testAnnotationInExternStubs3() {
+    // In the case where a property is defined first with a FUNCTION and then as a stub:
+    // we have to make a conservative assumption about the behaviour of the extern, since the stub
+    // carried no information.
     assertNoPureCalls("propWithAnnotatedStubAfter('a');");
   }
 
@@ -1586,7 +1591,9 @@ public void testAmbiguousDefinitionsAllCallThis() {
             "C.f = function() { };",
             "var g = function() {D.f()};",
             "/** @constructor */ var h = function() {E.f.apply(this)};",
-            "var i = function() {F.f.apply({})};", // it can't tell {} is local.
+            // TODO(nickreid): Detect that `{}` being passed as `this` is local. With that
+            // understanding, we could determine that `i` has no side-effects.
+            "var i = function() { F.f.apply({}); };",
             "g();",
             "new h();",
             "i();");
@@ -1679,7 +1686,7 @@ public void testAmbiguousDefinitionsDoubleDefinition2() {
             "a = function() {}", // This is the only `a` is in scope here.
             "B.x();",
             "a();");
-    assertPureCallsMarked(source, ImmutableList.of("a"));
+    assertNoPureCalls(source);
   }
 
   @Test
@@ -1700,10 +1707,12 @@ public void testAmbiguousDefinitionsDoubleDefinition4() {
     String source =
         lines(
             "var global = 1;",
+            "",
             "A.x = function a() {}",
             "B.x = function() { global++; }",
+            "",
             "B.x();",
-            "a();" // `a` is not in scope here.
+            "a();" // `a` isn't in scope here.
             );
     assertNoPureCalls(source);
   }
@@ -1713,11 +1722,13 @@ public void testAmbiguousDefinitionsDoubleDefinition5() {
     String source =
         lines(
             "var global = 1;",
+            "",
             "A.x = cond ? function a() { global++ } : function b() {}",
             "B.x = function() { global++; }",
+            "",
             "B.x();",
-            "a();", // `a` is not in scope here.
-            "b();" // `b` is not in scope here.
+            "a();", // `a` isn't in scope here.
+            "b();" // `b` isn't in scope here.
             );
     assertNoPureCalls(source);
   }
@@ -1733,6 +1744,27 @@ public void testAmbiguousDefinitionsDoubleDefinition6() {
     assertNoPureCalls(source);
   }
 
+  @Test
+  public void testInnerFunction_isNeverPure() {
+    // TODO(b/129503101): `pure` should be marked pure in this case.
+    assertNoPureCalls(
+        lines(
+            "function f() {", //
+            "  function pure() { };",
+            "  pure();",
+            "}"));
+  }
+
+  @Test
+  public void testNamedFunctionExpression_isNeverPure() {
+    // TODO(b/129503101): `pure` should be marked pure in this case.
+    assertNoPureCalls(
+        lines(
+            "(function pure() {", //
+            "  pure();",
+            "})"));
+  }
+
   @Test
   public void testCallBeforeDefinition() {
     assertPureCallsMarked("f(); function f(){}", ImmutableList.of("f"));
@@ -2341,9 +2373,7 @@ public void testDefaultValueInitializers_areConsidered_whenAnalyzingFunctions()
             "",
             "function foo(a = pure()) { }",
             "foo();"),
-        ImmutableList.of(
-            // TODO(b/128035138): Should be ["pure", "foo"]. "pure" is being polluted.
-            ));
+        ImmutableList.of("pure", "foo"));
 
     assertPureCallsMarked(
         lines(

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

@@ -470,7 +470,10 @@ public void testNoSideEffectAnnotation5() {
 
   @Test
   public void testNoSideEffectAnnotation6() {
-    test(externs("f = /** @nosideeffects */ function(){};"), srcs("var a = f();"), expected(""));
+    test(
+        externs("var f = /** @nosideeffects */ function(){};"), //
+        srcs("var a = f();"),
+        expected(""));
   }
 
   @Test
@@ -495,15 +498,11 @@ public void testNoSideEffectAnnotation8() {
   }
 
   @Test
-  public void testNoSideEffectAnnotation9() {
+  public void testNoSideEffectAnnotation_whenUsedOnDuplicateDefinitions_eliminatesSideEffects() {
     test(
         externs(
-            "f = /** @nosideeffects */ function(){};", "f = /** @nosideeffects */ function(){};"),
-        srcs("var a = f();"),
-        expected(""));
-
-    test(
-        externs("f = /** @nosideeffects */ function(){};"), //
+            "var f = /** @nosideeffects */ function(){};",
+            "var f = /** @nosideeffects */ function(){};"),
         srcs("var a = f();"),
         expected(""));
   }

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

@@ -558,10 +558,10 @@ public void testRemoveFromImportStatement_ES6Modules() {
   }
 
   @Test
-  public void testLetConstBlocks_withES6Modules() {
+  public void testLetConstBlocks_inFunction_exportedFromEs6Module() {
     test(
-        "export function f() {return 1; let a; } f();",
-        "export function f() {return 1;}");
+        "export function f() {return 1; let a; }", //
+        "export function f() {return 1;};");
 
     test(
         "export function f() {return 1; const a = 1; }",
@@ -578,7 +578,7 @@ public void testLetConstBlocks_withES6Modules() {
   // TODO(tbreisacher): Fix and enable.
   @Test
   @Ignore
-  public void testLetConstBlocks_withES6Modules2() {
+  public void testLetConstBlocks_asEs6ModuleExport() {
     test("export let x = 2;", "");
   }