diff --git a/src/com/google/javascript/jscomp/ClosureCheckModule.java b/src/com/google/javascript/jscomp/ClosureCheckModule.java index 1b19bc677c3..45c626fb4ae 100644 --- 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 importsByLongRequiredName = new HashMap<>(); // Module-local short names for goog.required symbols. private final Set 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 index d6a65d277be..78bf6c7ed02 100644 --- 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 index 2adf6fb1dcf..7cf15090ffa 100644 --- 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();",