Fix crash when trying to suggest a fix, when there are two different …

…styles of goog.require for the same namespace. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=184353406
google · Feb 5, 2018 · bd6bb5b · bd6bb5b
1 parent 8f7ac7a
commit bd6bb5b
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 16 deletions.
diff --git a/src/com/google/javascript/jscomp/ClosureCheckModule.java b/src/com/google/javascript/jscomp/ClosureCheckModule.java
@@ -160,7 +160,8 @@ public final class ClosureCheckModule extends AbstractModuleCallback
   private static class ModuleInfo {
     // Name of the module in question (i.e. the argument to goog.module)
     private final String name;
-    // Mapping from fully qualified goog.required names to the import LHS node
+    // Mapping from fully qualified goog.required names to the import LHS node.
+    // For standalone goog.require()s the value is the EXPR_RESULT node.
     private final Map<String, Node> importsByLongRequiredName = new HashMap<>();
     // Module-local short names for goog.required symbols.
     private final Set<String> shortImportNames = new HashSet<>();
@@ -417,7 +418,10 @@ private void checkRequireCall(NodeTraversal t, Node callNode, Node parent) {
     }
     switch (parent.getToken()) {
       case EXPR_RESULT:
-        currentModule.importsByLongRequiredName.put(extractFirstArgumentName(callNode), parent);
+        String key = extractFirstArgumentName(callNode);
+        if (!currentModule.importsByLongRequiredName.containsKey(key)) {
+          currentModule.importsByLongRequiredName.put(key, parent);
+        }
         return;
       case NAME:
       case DESTRUCTURING_LHS:

diff --git a/src/com/google/javascript/refactoring/SuggestedFix.java b/src/com/google/javascript/refactoring/SuggestedFix.java
@@ -811,23 +811,29 @@ public Builder removeGoogRequire(Match m, String namespace) {
       return this;
     }
 
+    /**
+     * Find the goog.require node for the given namespace (or null if there isn't one).
+     * If there is more than one, this will return the first standalone goog.require statement.
+     */
+    @Nullable
     private static Node findGoogRequireNode(Node n, NodeMetadata metadata, String namespace) {
       Node script = NodeUtil.getEnclosingScript(n);
       if (script.getFirstChild().isModuleBody()) {
         script = script.getFirstChild();
       }
 
-      if (script != null) {
-        Node child = script.getFirstChild();
-        while (child != null) {
-          if ((NodeUtil.isExprCall(child)
-                  && Matchers.googRequire(namespace).matches(child.getFirstChild(), metadata))
-              || (NodeUtil.isNameDeclaration(child)
-                  && child.getFirstFirstChild() != null && Matchers.googRequire(namespace)
-                      .matches(child.getFirstFirstChild(), metadata))) {
-            return child;
-          }
-          child = child.getNext();
+      for (Node child : script.children()) {
+        if (NodeUtil.isExprCall(child)
+            && Matchers.googRequire(namespace).matches(child.getFirstChild(), metadata)) {
+          return child;
+        }
+      }
+
+      for (Node child : script.children()) {
+        if (NodeUtil.isNameDeclaration(child)
+            && child.getFirstFirstChild() != null
+            && Matchers.googRequire(namespace).matches(child.getFirstFirstChild(), metadata)) {
+          return child;
         }
       }
       return null;

diff --git a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java
@@ -31,7 +31,6 @@
 import java.util.Collection;
 import java.util.List;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -798,9 +797,8 @@ public void testMissingRequireInGoogModule_atExtends() {
             "function Cat() {}"));
   }
 
-  @Ignore("Currently causes a crash. See b/72864468.")
   @Test
-  public void testBothFormsOfRequire() {
+  public void testBothFormsOfRequire1() {
     assertChanges(
         LINE_JOINER.join(
             "goog.module('example');",
@@ -816,6 +814,32 @@ public void testBothFormsOfRequire() {
             "goog.module('example');",
             "",
             "const SoyRenderer = goog.require('foo.bar.SoyRenderer');",
+            "function setUp() {",
+            "  const soyService = new SoyRenderer();",
+            "}",
+            ""));
+  }
+
+  @Test
+  public void testBothFormsOfRequire2() {
+    // After this change, a second run will remove the duplicate require.
+    // See testBothFormsOfRequire1
+    assertChanges(
+        LINE_JOINER.join(
+            "goog.module('example');",
+            "",
+            "goog.require('foo.bar.SoyRenderer');",
+            "const SoyRenderer = goog.require('foo.bar.SoyRenderer');",
+            "",
+            "function setUp() {",
+            "  const soyService = new foo.bar.SoyRenderer();",
+            "}",
+            ""),
+        LINE_JOINER.join(
+            "goog.module('example');",
+            "",
+            "const SoyRenderer = goog.require('foo.bar.SoyRenderer');",
+            "goog.require('foo.bar.SoyRenderer');",
             "",
             "function setUp() {",
             "  const soyService = new SoyRenderer();",