From 067cb58b9c02c831abf0968db45f3e91ac827b42 Mon Sep 17 00:00:00 2001 From: tbreisacher Date: Mon, 5 Mar 2018 14:50:45 -0800 Subject: [PATCH] Merge duplicate requires into a single require statement when possible. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=187926847 --- .../refactoring/ErrorToFixMapper.java | 32 ++++++++-- .../javascript/refactoring/SuggestedFix.java | 48 ++++++++++++-- .../refactoring/ErrorToFixMapperTest.java | 64 +++++++++++++++++-- 3 files changed, 128 insertions(+), 16 deletions(-) diff --git a/src/com/google/javascript/refactoring/ErrorToFixMapper.java b/src/com/google/javascript/refactoring/ErrorToFixMapper.java index 433f4ab8d2c..61cbfeba9f8 100644 --- a/src/com/google/javascript/refactoring/ErrorToFixMapper.java +++ b/src/com/google/javascript/refactoring/ErrorToFixMapper.java @@ -36,6 +36,7 @@ import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; +import javax.annotation.Nullable; /** * Maps a {@code JSError} to a list of {@code SuggestedFix}es, if possible. @@ -323,20 +324,37 @@ private static SuggestedFix getFixForMissingRequire(JSError error, AbstractCompi return fix.build(); } + @Nullable private static SuggestedFix getFixForDuplicateRequire(JSError error, AbstractCompiler compiler) { - if (!error.node.isExprResult()) { - return null; - } Matcher regexMatcher = DUPLICATE_REQUIRE.matcher(error.description); checkState( regexMatcher.matches(), "Unexpected error description: %s", error.description); String namespace = regexMatcher.group(1); NodeMetadata metadata = new NodeMetadata(compiler); Match match = new Match(error.node, metadata); - return new SuggestedFix.Builder() - .attachMatchedNodeInfo(error.node, compiler) - .removeGoogRequire(match, namespace) - .build(); + if (error.node.isExprResult()) { + return new SuggestedFix.Builder() + .attachMatchedNodeInfo(error.node, compiler) + .removeGoogRequire(match, namespace) + .build(); + } else { + checkState(NodeUtil.isNameDeclaration(error.node), error.node); + if (error.node.getFirstChild().isName()) { + return null; + } + + checkState(error.node.getFirstChild().isDestructuringLhs(), error.node); + + SuggestedFix fix = + new SuggestedFix.Builder() + .attachMatchedNodeInfo(error.node, compiler) + .mergeGoogRequire(error.node, match.getMetadata(), namespace, compiler) + .build(); + if (!fix.isNoOp()) { + return fix; + } + return null; + } } private static SuggestedFix getFixForExtraRequire(JSError error, AbstractCompiler compiler) { diff --git a/src/com/google/javascript/refactoring/SuggestedFix.java b/src/com/google/javascript/refactoring/SuggestedFix.java index fa2e34a3532..bdcfc8f4883 100644 --- a/src/com/google/javascript/refactoring/SuggestedFix.java +++ b/src/com/google/javascript/refactoring/SuggestedFix.java @@ -109,8 +109,13 @@ public ImmutableList getNonDefaultAlternatives() { return alternatives.subList(1, alternatives.size()); } - @Override public String toString() { - if (replacements.isEmpty()) { + boolean isNoOp() { + return replacements.isEmpty(); + } + + @Override + public String toString() { + if (this.isNoOp()) { return ""; } StringBuilder sb = new StringBuilder(); @@ -795,6 +800,35 @@ private boolean usesConstGoogRequires(final NodeMetadata metadata, Node script) return callback.getUsesConstRequires(); } + /** + * Merge names from the lhs of |requireToMergeFrom|, which must be a destructuring require, if + * there is another destructuring require that its lhs can be merged into. If not, then no + * change is applied. + */ + Builder mergeGoogRequire( + Node requireToMergeFrom, NodeMetadata m, String namespace, AbstractCompiler compiler) { + checkArgument(requireToMergeFrom.getFirstChild().isDestructuringLhs(), requireToMergeFrom); + checkArgument(requireToMergeFrom.getFirstFirstChild().isObjectPattern(), requireToMergeFrom); + + Node googRequireNode = findGoogRequireNode(requireToMergeFrom, m, namespace); + if (googRequireNode == null) { + return this; + } + if (googRequireNode.isExprResult()) { + return this; + } + if (googRequireNode.getFirstChild().isDestructuringLhs()) { + Node objectPattern = googRequireNode.getFirstFirstChild(); + Node newObjectPattern = objectPattern.cloneTree(); + for (Node name : requireToMergeFrom.getFirstFirstChild().children()) { + newObjectPattern.addChildToBack(name.cloneTree()); + } + this.replace(objectPattern, newObjectPattern, compiler); + this.deleteWithoutRemovingWhitespace(requireToMergeFrom); + } + return this; + } + /** * Removes a goog.require for the given namespace to the file if it * already exists. @@ -808,8 +842,14 @@ public Builder removeGoogRequire(Match m, String namespace) { } /** - * 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. + * Find the goog.require node for the given namespace (or null if there isn't one). If there is + * more than one: + * + *
    + *
  • If there is at least one standalone goog.require, this will return the first standalone + * goog.require. + *
  • If not, this will return the first goog.require. + *
*/ @Nullable private static Node findGoogRequireNode(Node n, NodeMetadata metadata, String namespace) { diff --git a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java index 0beece2dbfc..bf80409d7a8 100644 --- a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java +++ b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java @@ -1600,18 +1600,40 @@ public void testDuplicateRequire_shorthand() { @Test public void testDuplicateRequire_destructuring1() { - // TODO(b/74166725): Can we merge the two requires to: "const {bar, foo} = goog.require(...)"? - assertNoChanges( + assertChanges( LINE_JOINER.join( "const {bar} = goog.require('goog.string');", "const {foo} = goog.require('goog.string');", "", "alert(bar('7'));", + "alert(foo('8'));"), + LINE_JOINER.join( + "const {bar, foo} = goog.require('goog.string');", + "", + "", + "alert(bar('7'));", "alert(foo('8'));")); } @Test public void testDuplicateRequire_destructuring2() { + assertChanges( + LINE_JOINER.join( + "const {bar} = goog.require('goog.string');", + "const {foo:x, qux:y} = goog.require('goog.string');", + "", + "alert(bar('7'));", + "alert(x(y));"), + LINE_JOINER.join( + "const {bar, foo:x, qux:y} = goog.require('goog.string');", + "", + "", + "alert(bar('7'));", + "alert(x(y));")); + } + + @Test + public void testDuplicateRequire_destructuring3() { assertChanges( LINE_JOINER.join( "const {bar} = goog.require('goog.util');", @@ -1619,8 +1641,39 @@ public void testDuplicateRequire_destructuring2() { "", "alert(bar('7'));"), LINE_JOINER.join( + "const {bar} = goog.require('goog.util');", + "alert(bar('7'));")); + } + + // In this case, only the standalone require gets removed. Then a second run would merge + // the two remaining ones. + @Test + public void testDuplicateRequire_destructuring4() { + assertChanges( + LINE_JOINER.join( + "const {bar} = goog.require('goog.util');", + "const {foo} = goog.require('goog.util');", + "goog.require('goog.util');", + "", + "alert(bar('7'));", + "alert(foo('8'));"), + LINE_JOINER.join( + "const {bar} = goog.require('goog.util');", + "const {foo} = goog.require('goog.util');", + "alert(bar('7'));", + "alert(foo('8'));")); + } + + @Test + public void testDuplicateRequire_destructuring5() { + // TODO(b/74166725): Here, we could remove the second require and add "const {bar} = util;". + assertNoChanges( + LINE_JOINER.join( + "const util = goog.require('goog.util');", "const {bar} = goog.require('goog.util');", - "alert(bar('7'));")); + "", + "alert(bar('7'));", + "alert(util.foo('8'));")); } private void assertChanges(String originalCode, String expectedCode) { @@ -1633,8 +1686,9 @@ private void assertChanges(String originalCode, String expectedCode) { assertThat(warningsAndErrors).named("warnings/errors").isNotEmpty(); Collection fixes = errorManager.getAllFixes(); assertThat(fixes).named("fixes").isNotEmpty(); - String newCode = ApplySuggestedFixes.applySuggestedFixesToCode( - fixes, ImmutableMap.of("test", originalCode)).get("test"); + String newCode = + ApplySuggestedFixes.applySuggestedFixesToCode(fixes, ImmutableMap.of("test", originalCode)) + .get("test"); assertThat(newCode).isEqualTo(expectedCode); }