Look at all sets to a name in CheckGlobalNames before reporting a mis…

…sing module dep

The previous behavior was incorrect for names set multiple times, because the pass only considered one (somewhat arbitrary) set to be the name's 'declaration'.

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

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

@@ -17,6 +17,7 @@
 package com.google.javascript.jscomp;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.javascript.jscomp.GlobalNamespace.Name;
@@ -151,10 +152,8 @@ private void checkDescendantNames(Name name, boolean nameIsDefined) {
   private void validateName(Name name, boolean isDefined) {
     // If the name is not defined, emit warnings for each reference. While
     // we're looking through each reference, check all the module dependencies.
-    Ref declaration = name.getDeclaration();
     Name parent = name.getParent();
 
-    JSModuleGraph moduleGraph = compiler.getModuleGraph();
     boolean isTypedef = isTypedef(name);
     for (Ref ref : name.getRefs()) {
       // Don't worry about global exprs.
@@ -164,10 +163,7 @@ private void validateName(Name name, boolean isDefined) {
         if (!isGlobalExpr) {
           reportRefToUndefinedName(name, ref);
         }
-      } else if (declaration != null &&
-          ref.getModule() != declaration.getModule() &&
-          !moduleGraph.dependsOn(
-              ref.getModule(), declaration.getModule())) {
+      } else if (checkForBadModuleReference(name, ref)) {
         reportBadModuleReference(name, ref);
       } else {
         // Check for late references.
@@ -216,6 +212,50 @@ private static boolean isTypedef(Name name) {
     return false;
   }
 
+  /**
+   * Returns true if this name is potentially referenced before being defined in a different module
+   *
+   * <p>For example:
+   *
+   * <ul>
+   *   <li>Module B depends on Module A. name is set in Module A and referenced in Module B. this is
+   *       fine, and this method returns false.
+   *   <li>Module A and Module B are unrelated. name is set in Module A and referenced in Module B.
+   *       this is an error, and this method returns true.
+   *   <li>name is referenced in Module A, and never set globally. This warning is not specific to
+   *       modules, so is emitted elsewhere.
+   * </ul>
+   */
+  private boolean checkForBadModuleReference(Name name, Ref ref) {
+    JSModuleGraph moduleGraph = compiler.getModuleGraph();
+    if (name.getGlobalSets() == 0 || ref.type == Ref.Type.SET_FROM_GLOBAL) {
+      // Back off if either 1) this name was never set, or 2) this reference /is/ a set.
+      return false;
+    }
+    if (name.getGlobalSets() == 1) {
+      // there is only one global set - it should be set as name.declaration
+      // just look at that declaration instead of iterating through every single reference.
+      Ref declaration = checkNotNull(name.getDeclaration());
+      return !isSetFromPrecedingModule(ref, declaration, moduleGraph);
+    }
+    // there are multiple sets, so check if any of them happens in this module or a module earlier
+    // in the dependency chain.
+    for (Ref set : name.getRefs()) {
+      if (isSetFromPrecedingModule(ref, set, moduleGraph)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  /** Whether the set is in the global scope and occurs in a module the original ref depends on */
+  private static boolean isSetFromPrecedingModule(
+      Ref originalRef, Ref set, JSModuleGraph moduleGraph) {
+    return set.type == Ref.Type.SET_FROM_GLOBAL
+        && (originalRef.getModule() == set.getModule()
+            || moduleGraph.dependsOn(originalRef.getModule(), set.getModule()));
+  }
+
   private void reportBadModuleReference(Name name, Ref ref) {
     compiler.report(
         JSError.make(

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

@@ -352,6 +352,33 @@ public void testBadModuleDep2() {
     ), STRICT_MODULE_DEP_QNAME);
   }
 
+  @Test
+  public void testGlobalNameSetTwiceInSiblingModulesAllowed() {
+    testSame(
+        createModuleStar(
+            // root module
+            "class C {};",
+            // leaf 1
+            "C.m = 1; alert(C.m);",
+            // leaf 2
+            "C.m = 1; alert(C.m);"));
+  }
+
+  @Test
+  public void testGlobalNameSetOnlyInOtherSiblingModuleNotAllowed() {
+    testSame(
+        createModuleStar(
+            // root module
+            "class C {};",
+            // leaf 1 sets than uses C.m
+            "C.m = 1; alert(C.m);",
+            // leaf 2 also sets then uses C.m
+            "C.m = 1; alert(C.m);",
+            // leaf 3 uses C.m without it having been set
+            "alert(C.m);"),
+        STRICT_MODULE_DEP_QNAME);
+  }
+
   @Test
   public void testSelfModuleDep() {
     testSame(createModuleChain(