diff --git a/src/com/google/javascript/jscomp/lint/RequiresFixer.java b/src/com/google/javascript/jscomp/lint/RequiresFixer.java new file mode 100644 index 00000000000..d8a80efeb13 --- /dev/null +++ b/src/com/google/javascript/jscomp/lint/RequiresFixer.java @@ -0,0 +1,462 @@ +/* + * Copyright 2019 The Closure Compiler Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.javascript.jscomp.lint; + +import static com.google.common.collect.ImmutableList.toImmutableList; + +import com.google.auto.value.AutoValue; +import com.google.common.base.Preconditions; +import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ComparisonChain; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.common.collect.Multimap; +import com.google.errorprone.annotations.Immutable; +import com.google.javascript.jscomp.AbstractCompiler; +import com.google.javascript.jscomp.NodeTraversal; +import com.google.javascript.jscomp.NodeUtil; +import com.google.javascript.rhino.JSDocInfo; +import com.google.javascript.rhino.Node; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import javax.annotation.Nullable; + +/** + * Canonicalizes Closure import statements (goog.require, goog.requireType, and goog.forwardDeclare) + * by merging and deduplicating them. + */ +public final class RequiresFixer { + + /** Primitives that may be called in an import statement. */ + enum ImportPrimitive { + REQUIRE("goog.require"), + REQUIRE_TYPE("goog.requireType"), + FORWARD_DECLARE("goog.forwardDeclare"); + + private final String name; + + private ImportPrimitive(String name) { + this.name = name; + } + + /** Returns the primitive with the given name. */ + static ImportPrimitive fromName(String name) { + for (ImportPrimitive primitive : values()) { + if (primitive.name.equals(name)) { + return primitive; + } + } + throw new IllegalArgumentException("Invalid primitive name " + name); + } + + static ImportPrimitive WEAKEST = FORWARD_DECLARE; + + /** + * Returns the stronger of two primitives. + * + *

`goog.require` is stronger than `goog.requireType`, which is stronger than + * `goog.forwardDeclare`. + */ + @Nullable + static ImportPrimitive stronger(ImportPrimitive p1, ImportPrimitive p2) { + return p1.ordinal() < p2.ordinal() ? p1 : p2; + } + + @Override + public String toString() { + return name; + } + } + + /** + * One of the bindings of a destructuring pattern. + * + *

{@code exportedName} and {@code localName} are equal in the case where the binding does not + * explicitly specify a local name. + */ + @AutoValue + @Immutable + abstract static class DestructuringBinding implements Comparable { + abstract String exportedName(); + + abstract String localName(); + + static DestructuringBinding of(String exportedName, String localName) { + return new AutoValue_RequiresFixer_DestructuringBinding(exportedName, localName); + } + + /** Compares two bindings according to the style guide sort order. */ + @Override + public int compareTo(DestructuringBinding other) { + return ComparisonChain.start() + .compare(this.exportedName(), other.exportedName()) + .compare(this.localName(), other.localName()) + .result(); + } + } + + /** + * An import statement, which may have been merged from several import statements for the same + * namespace in the original code. + * + *

An import statement has exactly one of three shapes: + * + *

+ */ + @AutoValue + abstract static class ImportStatement implements Comparable { + /** Returns the nodes this import statement was merged from, in source order. */ + abstract ImmutableList nodes(); + + /** Returns the import primitive being called. */ + abstract ImportPrimitive primitive(); + + /** Returns the namespace being imported. */ + abstract String namespace(); + + /** Returns the alias for an aliasing import, or null if the import isn't aliasing. */ + abstract @Nullable String alias(); + + /** + * Returns the destructures for a destructuring import in source order, or null if the import + * isn't destructuring. + * + *

If the import is destructuring but the pattern is empty, the value is non-null but empty. + */ + abstract @Nullable ImmutableList destructures(); + + /** Creates a new import statement. */ + static ImportStatement of( + ImmutableList nodes, + ImportPrimitive primitive, + String namespace, + @Nullable String alias, + @Nullable ImmutableList destructures) { + Preconditions.checkArgument( + alias == null || destructures == null, + "Import statement cannot be simultaneously aliasing and destructuring"); + return new AutoValue_RequiresFixer_ImportStatement( + nodes, primitive, namespace, alias, destructures); + } + + /** Returns whether the import is standalone. */ + boolean isStandalone() { + return !isAliasing() && !isDestructuring(); + } + + /** Returns whether the import is aliasing. */ + boolean isAliasing() { + return alias() != null; + } + + /** Returns whether the import is destructuring. */ + boolean isDestructuring() { + return destructures() != null; + } + + /** + * Returns an import statement identical to the current one, except for its primitive, which is + * upgraded to the given one if stronger. + */ + ImportStatement upgrade(ImportPrimitive otherPrimitive) { + if (ImportPrimitive.stronger(primitive(), otherPrimitive) != primitive()) { + return new AutoValue_RequiresFixer_ImportStatement( + nodes(), otherPrimitive, namespace(), alias(), destructures()); + } + return this; + } + + private String formatWithoutDoc() { + StringBuilder sb = new StringBuilder(); + + if (!isStandalone()) { + sb.append("const "); + } + if (alias() != null) { + sb.append(alias()); + } + if (destructures() != null) { + sb.append("{"); + boolean first = true; + for (DestructuringBinding binding : destructures()) { + String exportedName = binding.exportedName(); + String localName = binding.localName(); + if (first) { + first = false; + } else { + sb.append(", "); + } + sb.append(exportedName); + if (!exportedName.equals(localName)) { + sb.append(": "); + sb.append(localName); + } + } + sb.append("}"); + } + if (!isStandalone()) { + sb.append(" = "); + } + sb.append(primitive().toString()); + sb.append("('"); + sb.append(namespace()); + sb.append("');"); + + return sb.toString(); + } + + /** Formats the import statement into code. */ + public String format() { + StringBuilder sb = new StringBuilder(); + + for (Node node : nodes()) { + JSDocInfo jsDoc = NodeUtil.getBestJSDocInfo(node); + if (jsDoc != null) { + sb.append(jsDoc.getOriginalCommentString() + "\n"); + } + } + + sb.append(formatWithoutDoc()); + + return sb.toString(); + } + + /** Compares two import statements according to the style guide sort order. */ + @Override + public int compareTo(ImportStatement other) { + return ComparisonChain.start() + .compare(this.formatWithoutDoc(), other.formatWithoutDoc()) + .result(); + } + } + + /** A traversal collecting existing import statements. */ + private static class ImportsCallback implements NodeTraversal.Callback { + // The import statements in the order they appear. + private final List originalImports = new ArrayList<>(); + + // Maps each namespace into the existing import statements for that namespace. + // Use an ArrayListMultimap so that values for a key are iterated in a deterministic order. + private final Multimap importsByNamespace = ArrayListMultimap.create(); + + private Node firstNode = null; + private Node lastNode = null; + private boolean finished = false; + + @Override + public final boolean shouldTraverse(NodeTraversal nodeTraversal, Node n, Node parent) { + // Traverse top-level statements until a block of contiguous requires is found. + return !finished + && (parent == null || parent.isRoot() || parent.isScript() || parent.isModuleBody()); + } + + @Override + public final void visit(NodeTraversal nodeTraversal, Node n, Node parent) { + Node callNode = null; + if (n.isExprResult()) { + callNode = n.getFirstChild(); + } else if (NodeUtil.isNameDeclaration(n)) { + callNode = n.getFirstChild().getLastChild(); + } + if (callNode != null && isValidImportCall(callNode)) { + ImportStatement stmt = parseImport(callNode); + originalImports.add(stmt); + importsByNamespace.put(stmt.namespace(), stmt); + if (firstNode == null) { + firstNode = lastNode = n; + } else { + lastNode = n; + } + } else if (!importsByNamespace.isEmpty()) { + finished = true; + } + } + + private static boolean isValidImportCall(Node n) { + return n.isCall() + && n.hasTwoChildren() + && (n.getFirstChild().matchesQualifiedName("goog.require") + || n.getFirstChild().matchesQualifiedName("goog.requireType") + || n.getFirstChild().matchesQualifiedName("goog.forwardDeclare")) + && n.getSecondChild().isString(); + } + + private static ImportStatement parseImport(Node callNode) { + ImportPrimitive primitive = + ImportPrimitive.fromName(callNode.getFirstChild().getQualifiedName()); + String namespace = callNode.getSecondChild().getString(); + + Node parent = callNode.getParent(); + if (parent.isExprResult()) { + // goog.require('a'); + return ImportStatement.of( + ImmutableList.of(parent), + primitive, + namespace, + /* alias= */ null, + /* destructures= */ null); + } + + Node grandparent = parent.getParent(); + if (parent.isName()) { + // const a = goog.require('a'); + String alias = parent.getString(); + return ImportStatement.of( + ImmutableList.of(grandparent), primitive, namespace, alias, /* destructures= */ null); + } + // const {a: b, c} = goog.require('a'); + ImmutableList.Builder destructures = ImmutableList.builder(); + for (Node name : parent.getFirstChild().children()) { + String exportedName = name.getString(); + String localName = name.getFirstChild().getString(); + destructures.add(DestructuringBinding.of(exportedName, localName)); + } + return ImportStatement.of( + ImmutableList.of(grandparent), + primitive, + namespace, + /* alias= */ null, + destructures.build()); + } + } + + private final Node firstNode; + private final Node lastNode; + private final String replacement; + private final boolean needsFix; + + public RequiresFixer(AbstractCompiler compiler, Node scriptRoot) { + ImportsCallback callback = new ImportsCallback(); + NodeTraversal.traverse(compiler, scriptRoot, callback); + + firstNode = callback.firstNode; + lastNode = callback.lastNode; + + List originalImports = callback.originalImports; + List canonicalImports = canonicalizeImports(callback.importsByNamespace); + + needsFix = !originalImports.equals(canonicalImports); + replacement = String.join("\n", Iterables.transform(canonicalImports, ImportStatement::format)); + } + + /** Returns the node for the first recognized import statement. */ + public Node getFirstNode() { + return firstNode; + } + + /** Returns the node for the last recognized import statement. */ + public Node getLastNode() { + return lastNode; + } + + /** Returns a textual replacement yielding a canonical version of the imports. */ + public String getReplacement() { + return replacement; + } + + /** + * Returns whether the imports need to be fixed, i.e., whether they are *not* already canonical. + */ + public boolean needsFix() { + return needsFix; + } + + /** + * Canonicalizes a list of import statements by deduplicating and merging imports for the same + * namespace, and sorting the result. + */ + private static List canonicalizeImports( + Multimap importsByNamespace) { + List canonicalImports = new ArrayList<>(); + + for (String namespace : importsByNamespace.keySet()) { + Collection allImports = importsByNamespace.get(namespace); + + // Find the strongest primitive across all existing imports. Every emitted import for this + // namespace will use this primitive. This makes the logic simpler and cannot change runtime + // behavior, but may produce spurious changes when multiple aliasing imports of differing + // strength exist (which are already in violation of the style guide). + ImportPrimitive strongestPrimitive = + allImports.stream() + .map(ImportStatement::primitive) + .reduce(ImportPrimitive.WEAKEST, ImportPrimitive::stronger); + + // Emit each aliasing import separately, as deduplicating them would require code references + // to be rewritten. + boolean hasAliasing = false; + for (ImportStatement stmt : Iterables.filter(allImports, ImportStatement::isAliasing)) { + canonicalImports.add(stmt.upgrade(strongestPrimitive)); + hasAliasing = true; + } + + // Emit a single destructuring import with a non-empty pattern, merged from the existing + // destructuring imports. + boolean hasDestructuring = false; + ImmutableList destructuringNodes = + allImports.stream() + .filter(ImportStatement::isDestructuring) + .flatMap(i -> i.nodes().stream()) + .collect(toImmutableList()); + ImmutableList destructures = + allImports.stream() + .filter(ImportStatement::isDestructuring) + .flatMap(i -> i.destructures().stream()) + .distinct() + .sorted() + .collect(toImmutableList()); + if (!destructures.isEmpty()) { + canonicalImports.add( + ImportStatement.of( + destructuringNodes, + strongestPrimitive, + namespace, + /* alias= */ null, + destructures)); + hasDestructuring = true; + } + + // Emit a standalone import unless an aliasing or destructuring one already exists. + if (!hasAliasing && !hasDestructuring) { + ImmutableList standaloneNodes = + allImports.stream() + .filter(ImportStatement::isStandalone) + .flatMap(i -> i.nodes().stream()) + .collect(toImmutableList()); + canonicalImports.add( + ImportStatement.of( + standaloneNodes, + strongestPrimitive, + namespace, + /* alias= */ null, + /* destructures= */ null)); + } + } + + // Sorting by natural order yields the correct result due to the implementation of + // ImportStatement#compareTo. + Collections.sort(canonicalImports); + + return canonicalImports; + } +} diff --git a/src/com/google/javascript/refactoring/ErrorToFixMapper.java b/src/com/google/javascript/refactoring/ErrorToFixMapper.java index 8755bf8e768..9212c8d6494 100644 --- a/src/com/google/javascript/refactoring/ErrorToFixMapper.java +++ b/src/com/google/javascript/refactoring/ErrorToFixMapper.java @@ -26,6 +26,7 @@ import com.google.javascript.jscomp.NodeTraversal; import com.google.javascript.jscomp.NodeUtil; import com.google.javascript.jscomp.lint.CheckRequiresAndProvidesSorted; +import com.google.javascript.jscomp.lint.RequiresFixer; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.JSDocInfo; import com.google.javascript.rhino.Node; @@ -84,10 +85,9 @@ public static SuggestedFix getFixForJsError(JSError error, AbstractCompiler comp case "JSC_MISSING_SEMICOLON": return getFixForMissingSemicolon(error, compiler); case "JSC_REQUIRES_NOT_SORTED": - return getFixForUnsortedRequiresOrProvides( - error, compiler, "goog.require", "goog.requireType", "goog.forwardDeclare"); + return getFixForUnsortedRequires(error, compiler); case "JSC_PROVIDES_NOT_SORTED": - return getFixForUnsortedRequiresOrProvides(error, compiler, "goog.provide"); + return getFixForUnsortedProvides(error, compiler); case "JSC_DEBUGGER_STATEMENT_PRESENT": return removeNode(error, compiler); case "JSC_USELESS_EMPTY_STATEMENT": @@ -352,12 +352,26 @@ private static SuggestedFix getFixForExtraRequire(JSError error, AbstractCompile return fix.build(); } - private static SuggestedFix getFixForUnsortedRequiresOrProvides( - JSError error, AbstractCompiler compiler, String... closureFunctions) { + private static SuggestedFix getFixForUnsortedRequires(JSError error, AbstractCompiler compiler) { + Node script = NodeUtil.getEnclosingScript(error.node); + + RequiresFixer fixer = new RequiresFixer(compiler, script); + + if (!fixer.needsFix()) { + return null; + } + + return new SuggestedFix.Builder() + .attachMatchedNodeInfo(fixer.getFirstNode(), compiler) + .replaceRange(fixer.getFirstNode(), fixer.getLastNode(), fixer.getReplacement()) + .build(); + } + + private static SuggestedFix getFixForUnsortedProvides(JSError error, AbstractCompiler compiler) { SuggestedFix.Builder fix = new SuggestedFix.Builder(); fix.attachMatchedNodeInfo(error.node, compiler); Node script = NodeUtil.getEnclosingScript(error.node); - RequireProvideSorter cb = new RequireProvideSorter(closureFunctions); + RequireProvideSorter cb = new RequireProvideSorter("goog.provide"); NodeTraversal.traverse(compiler, script, cb); Node first = cb.calls.get(0); Node last = Iterables.getLast(cb.calls); @@ -385,6 +399,7 @@ private static SuggestedFix getFixForRedundantNullabilityModifierJsDoc( .build(); } + // TODO(tjgq): Rewrite this into a simpler ProvideFixer now that RequireFixer has been split off. private static class RequireProvideSorter implements NodeTraversal.Callback, Comparator { private final ImmutableSet closureFunctions; private final List calls = new ArrayList<>(); diff --git a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java index 80005e2255f..400e241c1c5 100644 --- a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java +++ b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java @@ -29,13 +29,16 @@ import com.google.javascript.jscomp.SourceFile; import java.util.Collection; import org.junit.Before; +import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** - * Test case for {@link ErrorToFixMapper}. - */ +// TODO(tjgq): Re-enable the disabled tests once CheckRequiresAndProvidesSorted is updated to use +// RequiresFixer. Currently, CheckRequiresAndProvidesSorted and ErrorToFixMapper sometimes disagree +// on whether a fix is required, which causes no fix to be suggested when it should. + +/** Test case for {@link ErrorToFixMapper}. */ @RunWith(JUnit4.class) public class ErrorToFixMapperTest { @@ -365,334 +368,364 @@ public void testInsertSemicolon2() { } @Test - public void testRequiresSorted_standalone() { + public void testFixRequires_sortStandaloneOnly() { assertChanges( - lines( - "/**", - " * @fileoverview", - " * @suppress {extraRequire}", - " */", - "", - "", + fileWithImports( "goog.require('b');", - "goog.requireType('a');", "goog.requireType('d');", - "goog.require('c');", - "", - "", - "alert(1);"), - lines( - "/**", - " * @fileoverview", - " * @suppress {extraRequire}", - " */", - "", - "", - "goog.requireType('a');", + "goog.requireType('c');", + "goog.forwardDeclare('f');", + "goog.require('a');", + "goog.forwardDeclare('e');", + useInCode("a", "b"), + useInType("c", "d")), + fileWithImports( + "goog.forwardDeclare('e');", + "goog.forwardDeclare('f');", + "goog.require('a');", "goog.require('b');", - "goog.require('c');", + "goog.requireType('c');", "goog.requireType('d');", - "", - "", - "alert(1);")); + useInCode("a", "b"), + useInType("c", "d"))); } @Test - public void testRequiresSorted_suppressExtra() { + public void testFixRequires_sortAllTypes() { assertChanges( - lines( - "/**", - " * @fileoverview", - " * @suppress {extraRequire}", - " */", - "goog.provide('x');", - "", - "/** @suppress {extraRequire} */", - "goog.requireType('c');", - "/** @suppress {extraRequire} */", + fileWithImports( + "goog.requireType('a');", "goog.require('b');", + "const f = goog.require('f');", + "const {d} = goog.require('d');", + "const e = goog.requireType('e');", + "const {c} = goog.requireType('c');", + "goog.forwardDeclare('g');", + useInCode("a", "d", "f"), + useInType("b", "c", "e")), + fileWithImports( + "const e = goog.requireType('e');", + "const f = goog.require('f');", + "const {c} = goog.requireType('c');", + "const {d} = goog.require('d');", + "goog.forwardDeclare('g');", + "goog.require('b');", + "goog.requireType('a');", + useInCode("a", "d", "f"), + useInType("b", "c", "e"))); + } + + @Ignore + @Test + public void testFixRequires_deduplicate_standalone() { + assertChanges( + fileWithImports( "goog.require('a');", - "", - "alert(1);"), - lines( - "/**", - " * @fileoverview", - " * @suppress {extraRequire}", - " */", - "goog.provide('x');", - "", "goog.require('a');", - "/** @suppress {extraRequire} */", "goog.require('b');", - "/** @suppress {extraRequire} */", + "goog.requireType('b');", "goog.requireType('c');", - "", - "alert(1);")); + "goog.requireType('c');", + "goog.forwardDeclare('b');", + "goog.forwardDeclare('c');", + "goog.forwardDeclare('d');", + useInCode("a", "b"), + useInType("c")), + fileWithImports( + "goog.forwardDeclare('d');", + "goog.require('a');", + "goog.require('b');", + "goog.requireType('c');", + useInCode("a", "b"), + useInType("c"))); } @Test - public void testSortRequiresInGoogModule_let() { + public void testFixRequires_preserveMultipleAliases() { + // a3 changes from goog.requireType to goog.require because we enforce all imports for the same + // namespace to have the same strength when rewriting. assertChanges( - lines( - "goog.module('m');", - "", - "/** @suppress {extraRequire} */", - "goog.require('a.c');", - "/** @suppress {extraRequire} */", - "goog.require('a.b');", - "", - "let localVar;"), - lines( - "goog.module('m');", - "", - "/** @suppress {extraRequire} */", - "goog.require('a.b');", - "/** @suppress {extraRequire} */", - "goog.require('a.c');", - "", - "let localVar;")); + fileWithImports( + "const a3 = goog.requireType('a');", + "const a2 = goog.require('a');", + "const a1 = goog.require('a');", + useInCode("a1", "a2"), + useInType("a3")), + fileWithImports( + "const a1 = goog.require('a');", + "const a2 = goog.require('a');", + "const a3 = goog.require('a');", + useInCode("a1", "a2"), + useInType("a3"))); } @Test - public void testSortRequiresInGoogModule_const() { + public void testFixRequires_mergeDestructuring() { assertChanges( - lines( - "goog.module('m');", - "", - "/** @suppress {extraRequire} */", - "goog.require('a.c');", - "/** @suppress {extraRequire} */", - "goog.require('a.b');", - "", - "const FOO = 0;"), - lines( - "goog.module('m');", - "", - "/** @suppress {extraRequire} */", - "goog.require('a.b');", - "/** @suppress {extraRequire} */", - "goog.require('a.c');", - "", - "const FOO = 0;")); + fileWithImports( + "const {c, a: a2} = goog.require('a');", + "const {b, a: a1} = goog.require('a');", + "const {a} = goog.require('a');", + "const {} = goog.require('a');", + useInCode("a", "a1", "a2", "b", "c")), + fileWithImports( + "const {a, a: a1, a: a2, b, c} = goog.require('a');", + useInCode("a", "a1", "a2", "b", "c"))); } - /** - * Using this form in a goog.module is a violation of the style guide, but still fairly common. - */ @Test - public void testSortRequiresInGoogModule_standalone() { + public void testFixRequires_mergeDestructuring_multiplePrimitives() { assertChanges( - lines( - "/** @fileoverview @suppress {strictModuleChecks} */", - "goog.module('m');", - "", - "goog.require('a.c');", - "goog.require('a.b.d');", - "goog.require('a.b.c');", - "", - "alert(a.c());", - "alert(a.b.d());", - "alert(a.b.c());"), - lines( - "/** @fileoverview @suppress {strictModuleChecks} */", - "goog.module('m');", - "", - "goog.require('a.b.c');", - "goog.require('a.b.d');", - "goog.require('a.c');", - "", - "alert(a.c());", - "alert(a.b.d());", - "alert(a.b.c());")); + fileWithImports( + "const {c, a} = goog.require('a');", + "const {b, a} = goog.requireType('a');", + useInCode("a", "c"), + useInType("a", "b")), + fileWithImports( + "const {a, b, c} = goog.require('a');", useInCode("a", "c"), useInType("a", "b"))); } + @Ignore @Test - public void testSortRequiresInGoogModule_shorthand() { + public void testFixRequires_emptyDestructuring_alone() { assertChanges( - lines( - "goog.module('m');", - "", - "var c2 = goog.require('a.c');", - "var d = goog.require('a.b.d');", - "var c1 = goog.require('a.b.c');", - "", - "alert(c1());", - "alert(d());", - "alert(c2());"), - lines( - "goog.module('m');", - "", - "var c1 = goog.require('a.b.c');", - "var c2 = goog.require('a.c');", - "var d = goog.require('a.b.d');", - "", - "alert(c1());", - "alert(d());", - "alert(c2());")); + fileWithImports("const {} = goog.require('a');", useInCode("a")), + fileWithImports("goog.require('a');", useInCode("a"))); } + @Ignore @Test - public void testSortRequiresInGoogModule_destructuring() { + public void testFixRequires_emptyDestructuringStandaloneBySamePrimitive() { assertChanges( - lines( - "/** @fileoverview @suppress {extraRequire} */", - "goog.module('m');", - "", - "const {fooBar} = goog.require('x');", - "const {foo, bar} = goog.requireType('y');"), - lines( - "/** @fileoverview @suppress {extraRequire} */", - "goog.module('m');", - "", - "const {foo, bar} = goog.requireType('y');", - "const {fooBar} = goog.require('x');")); + fileWithImports("const {} = goog.require('a');", "goog.require('a');", useInCode("a")), + fileWithImports("goog.require('a');", useInCode("a"))); } + @Ignore @Test - public void testSortRequiresInGoogModule_shorthandAndStandalone() { + public void testFixRequires_emptyDestructuringStandaloneByStrongerPrimitive() { assertChanges( - lines( - "/** @fileoverview @suppress {extraRequire} */", - "goog.module('m');", - "", - "const shorthand2 = goog.requireType('a');", - "goog.require('standalone.two');", - "goog.requireType('standalone.one');", - "const shorthand1 = goog.require('b');"), - lines( - "/** @fileoverview @suppress {extraRequire} */", - "goog.module('m');", - "", - "const shorthand1 = goog.require('b');", - "const shorthand2 = goog.requireType('a');", - "goog.requireType('standalone.one');", - "goog.require('standalone.two');")); + fileWithImports("const {} = goog.requireType('a');", "goog.require('a');", useInCode("a")), + fileWithImports("goog.require('a');", useInCode("a"))); } + @Ignore @Test - public void testSortRequiresInGoogModule_allThreeStyles() { + public void testFixRequires_emptyDestructuringStandaloneByWeakerPrimitive() { assertChanges( - lines( - "/** @fileoverview @suppress {extraRequire} */", - "goog.module('m');", - "", - "const shorthand2 = goog.requireType('a');", - "goog.require('standalone.two');", - "const {destructuring2} = goog.requireType('c');", - "const {destructuring1} = goog.require('d');", - "goog.requireType('standalone.one');", - "const shorthand1 = goog.require('b');"), - lines( - "/** @fileoverview @suppress {extraRequire} */", - "goog.module('m');", - "", - "const shorthand1 = goog.require('b');", - "const shorthand2 = goog.requireType('a');", - "const {destructuring1} = goog.require('d');", - "const {destructuring2} = goog.requireType('c');", - "goog.requireType('standalone.one');", - "goog.require('standalone.two');")); + fileWithImports("const {} = goog.require('a');", "goog.requireType('a');", useInCode("a")), + fileWithImports("goog.require('a');", useInCode("a"))); } @Test - public void testSortRequiresInGoogModule_withFwdDeclare() { + public void testFixRequires_emptyDestructuringAliasedBySamePrimitive() { assertChanges( - lines( - "goog.module('x');", - "", - "const s = goog.require('s');", - "const g = goog.forwardDeclare('g');", - "const f = goog.forwardDeclare('f');", - "const r = goog.require('r');", - "", - "alert(r, s);"), - lines( - "goog.module('x');", - "", - "const r = goog.require('r');", - "const s = goog.require('s');", - "const f = goog.forwardDeclare('f');", - "const g = goog.forwardDeclare('g');", - "", - "alert(r, s);")); + fileWithImports( + "const {} = goog.require('a');", "const a = goog.require('a');", useInCode("a")), + fileWithImports("const a = goog.require('a');", useInCode("a"))); } @Test - public void testSortRequiresInGoogModule_withOtherStatements() { - // The requires after "const {Bar} = bar;" are not sorted. + public void testFixRequires_emptyDestructuringAliasedByStrongerPrimitive() { assertChanges( - lines( - "goog.module('x');", - "", - "const foo = goog.require('foo');", - "const bar = goog.require('bar');", - "const {Bar} = bar;", - "const util = goog.require('util');", - "const type = goog.requireType('type');", - "const {doCoolThings} = util;", - "", - "/** @type {!type} */ let x;", - "doCoolThings(foo, Bar);"), - lines( - "goog.module('x');", - "", - "const bar = goog.require('bar');", - "const foo = goog.require('foo');", - "const {Bar} = bar;", - "const util = goog.require('util');", - "const type = goog.requireType('type');", - "const {doCoolThings} = util;", - "", - "/** @type {!type} */ let x;", - "doCoolThings(foo, Bar);")); + fileWithImports( + "const {} = goog.requireType('a');", "const a = goog.require('a');", useInCode("a")), + fileWithImports("const a = goog.require('a');", useInCode("a"))); } @Test - public void testSortRequiresInGoogModule_veryLongRequire() { + public void testFixRequires_emptyDestructuringAliasedByWeakerPrimitive() { assertChanges( - lines( - "goog.module('m');", - "", - "const {veryLongDestructuringStatementSoLongThatWeGoPast80CharactersBeforeGettingToTheClosingCurlyBrace} = goog.require('other');", - "const {anotherVeryLongDestructuringStatementSoLongThatWeGoPast80CharactersBeforeGettingToTheClosingCurlyBrace} = goog.requireType('type');", - "const shorter = goog.require('shorter');", - "", - "/** @type {!anotherVeryLongDestructuringStatementSoLongThatWeGoPast80CharactersBeforeGettingToTheClosingCurlyBrace} */ var x;", - "use(veryLongDestructuringStatementSoLongThatWeGoPast80CharactersBeforeGettingToTheClosingCurlyBrace);", - "use(shorter);"), - lines( - "goog.module('m');", - "", - "const shorter = goog.require('shorter');", - "const {anotherVeryLongDestructuringStatementSoLongThatWeGoPast80CharactersBeforeGettingToTheClosingCurlyBrace} = goog.requireType('type');", - "const {veryLongDestructuringStatementSoLongThatWeGoPast80CharactersBeforeGettingToTheClosingCurlyBrace} = goog.require('other');", + fileWithImports( + "const {} = goog.require('a');", "const a = goog.requireType('a');", useInCode("a")), + fileWithImports("const a = goog.require('a');", useInCode("a"))); + } + + @Test + public void testFixRequires_aliasPreservedWhenDestructuring() { + assertNoChanges( + fileWithImports( + "const a = goog.require('a');", + "const b = goog.require('b');", + "const {a1, a2} = goog.require('a');", + "const {b1, b2} = goog.require('b');", "", - "/** @type {!anotherVeryLongDestructuringStatementSoLongThatWeGoPast80CharactersBeforeGettingToTheClosingCurlyBrace} */ var x;", - "use(veryLongDestructuringStatementSoLongThatWeGoPast80CharactersBeforeGettingToTheClosingCurlyBrace);", - "use(shorter);")); + useInCode("a1", "a2", "b", "b1", "b2"), + useInType("a"))); } + @Ignore @Test - public void testSortRequiresAndForwardDeclares() { + public void testFixRequires_standaloneAliasedBySamePrimitive() { assertChanges( - lines( - "goog.provide('x');", - "", - "goog.require('s');", - "goog.forwardDeclare('g');", - "goog.forwardDeclare('f');", - "goog.require('r');", - "", - "alert(r, s);"), - lines( - "goog.provide('x');", - "", - "goog.require('r');", - "goog.require('s');", - "goog.forwardDeclare('f');", - "goog.forwardDeclare('g');", + fileWithImports("const a = goog.require('a');", "goog.require('a');", useInCode("a")), + fileWithImports("const a = goog.require('a');", useInCode("a"))); + } + + @Ignore + @Test + public void testFixRequires_standaloneAliasedByStrongerPrimitive() { + assertChanges( + fileWithImports("const a = goog.require('a');", "goog.requireType('a');", useInCode("a")), + fileWithImports("const a = goog.require('a');", useInCode("a"))); + } + + @Ignore + @Test + public void testFixRequires_standaloneAliasedByWeakerPrimitive() { + assertChanges( + fileWithImports("const a = goog.requireType('a');", "goog.require('a');", useInCode("a")), + fileWithImports("const a = goog.require('a');", useInCode("a"))); + } + + @Ignore + @Test + public void testFixRequires_standaloneDestructuredBySamePrimitive() { + assertChanges( + fileWithImports("const {a} = goog.require('a');", "goog.require('a');", useInCode("a")), + fileWithImports("const {a} = goog.require('a');", useInCode("a"))); + } + + @Ignore + @Test + public void testFixRequires_standaloneDestructuredByStrongerPrimitive() { + assertChanges( + fileWithImports("const {a} = goog.require('a');", "goog.requireType('a');", useInCode("a")), + fileWithImports("const {a} = goog.require('a');", useInCode("a"))); + } + + @Ignore + @Test + public void testFixRequires_standaloneDestructuredByWeakerPrimitive() { + assertChanges( + fileWithImports("const {a} = goog.requireType('a');", "goog.require('a');", useInCode("a")), + fileWithImports("const {a} = goog.require('a');", useInCode("a"))); + } + + @Test + public void testFixRequires_varAndLetBecomeConstIfUnsorted() { + assertChanges( + fileWithImports( + "var b = goog.require('b');", "let a = goog.require('a');", useInCode("a", "b")), + fileWithImports( + "const a = goog.require('a');", "const b = goog.require('b');", useInCode("a", "b"))); + } + + @Test + public void testFixRequires_varAndLetDoNotBecomeConstIfAlreadySorted() { + // TODO(tjgq): Consider rewriting to const even when already sorted. + assertNoChanges( + fileWithImports( + "let a = goog.require('a');", "var b = goog.require('b');", useInCode("a", "b"))); + } + + @Test + public void testFixRequires_preserveJsDoc_whenSorting() { + assertChanges( + fileWithImports( + "const c = goog.require('c');", + "/**", + " * @suppress {extraRequire} Because I said so.", + " */", + "const b = goog.require('b');", + "const a = goog.require('a');", + useInCode("a", "b", "c")), + fileWithImports( + "const a = goog.require('a');", + "/**", + " * @suppress {extraRequire} Because I said so.", + " */", + "const b = goog.require('b');", + "const c = goog.require('c');", + useInCode("a", "b", "c"))); + } + + @Ignore + @Test + public void testFixRequires_preserveJsDoc_whenMergingStandalone() { + assertChanges( + fileWithImports( + "/**", + " * @suppress {extraRequire} Because I said so.", + " */", + "goog.require('a');", + "goog.requireType('a');", + useInCode("a")), + fileWithImports( + "/**", + " * @suppress {extraRequire} Because I said so.", + " */", + "goog.require('a');", + useInCode("a"))); + } + + @Ignore + @Test + public void testFixRequires_preserveJsDoc_whenMergingDestructures_single() { + assertChanges( + fileWithImports( + "/**", + " * @suppress {extraRequire} Because I said so.", + " */", + "const {b} = goog.require('a');", + "const {c} = goog.require('a');", + useInCode("b", "c")), + fileWithImports( + "/**", + " * @suppress {extraRequire} Because I said so.", + " */", + "const {b, c} = goog.require('a');", + useInCode("b", "c"))); + } + + @Ignore + @Test + public void testFixRequires_preserveJsDoc_whenMergingDestructures_multiple() { + // TODO(tjgq): Consider merging multiple @suppress annotations into a single comment. + assertChanges( + fileWithImports( + "/**", + " * @suppress {extraRequire} Because I said so.", + " */", + "const {b} = goog.require('a');", + "/**", + " * @suppress {extraRequire} Because I rule.", + " */", + "const {c} = goog.require('a');", + useInCode("b", "c")), + fileWithImports( + "/**", + " * @suppress {extraRequire} Because I said so.", + " */", + "/**", + " * @suppress {extraRequire} Because I rule.", + " */", + "const {b, c} = goog.require('a');", + useInCode("b", "c"))); + } + + @Test + public void testFixRequires_veryLongNames() { + assertNoChanges( + fileWithImports( + "const veryLongIdentifierSoLongThatItGoesPastThe80CharactersLimitAndYetWeShouldNotLineBreak = goog.require('b');", + "const {anotherVeryLongIdentifierSoLongThatItGoesPastThe80CharactersLimitAndYetWeShouldNotLineBreak} = goog.require('a');", "", - "alert(r, s);")); + useInCode( + "veryLongIdentifierSoLongThatItGoesPastThe80CharactersLimitAndYetWeShouldNotLineBreak", + "anotherVeryLongIdentifierSoLongThatItGoesPastThe80CharactersLimitAndYetWeShouldNotLineBreak"))); + } + + @Test + public void testFixRequires_veryLongNames_whenMergingDestructures() { + assertChanges( + fileWithImports( + "const {veryLongSymbolThatMapsTo: veryLongLocalNameForIt} = goog.require('a');", + "const {anotherVeryLongSymbolThatMapsTo: veryLongLocalNameForItAlso} = goog.require('a');", + useInCode("veryLongLocalNameForIt", "veryLongLocalNameForItAlso")), + fileWithImports( + "const {anotherVeryLongSymbolThatMapsTo: veryLongLocalNameForItAlso, veryLongSymbolThatMapsTo: veryLongLocalNameForIt} = goog.require('a');", + useInCode("veryLongLocalNameForIt", "veryLongLocalNameForItAlso"))); } @Test @@ -837,31 +870,6 @@ public void testMissingRequireInGoogModule_atExtends() { "function Cat() {}")); } - @Test - public void testBothFormsOfRequire() { - assertChanges( - lines( - "goog.module('example');", - "", - "goog.require('foo.bar.SoyRenderer');", - "const SoyRenderer = goog.require('foo.bar.SoyRenderer');", - "", - "function setUp() {", - " const soyService = new foo.bar.SoyRenderer();", - "}", - ""), - lines( - "goog.module('example');", - "", - "const SoyRenderer = goog.require('foo.bar.SoyRenderer');", - "goog.require('foo.bar.SoyRenderer');", - "", - "function setUp() {", - " const soyService = new SoyRenderer();", - "}", - "")); - } - @Test public void testStandaloneVarDoesntCrashMissingRequire() { assertChanges( @@ -1152,49 +1160,6 @@ public void testMissingRequireInGoogModule_alwaysInsertsConst() { "alert(new A(new B(new C())));")); } - @Test - public void testSortShorthandRequiresInGoogModule() { - assertChanges( - lines( - "goog.module('m');", - "", - "var B = goog.require('x.B');", - "var A = goog.require('a.A');", - "var C = goog.require('c.C');", - "", - "alert(new A(new B(new C())));"), - lines( - "goog.module('m');", - "", - // Requires are sorted by the short name, not the full namespace. - "var A = goog.require('a.A');", - "var B = goog.require('x.B');", - "var C = goog.require('c.C');", - "", - "alert(new A(new B(new C())));")); - } - - @Test - public void testUnsortedAndMissingLhs() { - assertChanges( - lines( - "goog.module('foo');", - "", - "goog.require('example.controller');", - "const Bar = goog.require('example.Bar');", - "", - "alert(example.controller.SOME_CONSTANT);", - "alert(Bar.doThings);"), - lines( - "goog.module('foo');", - "", - "const Bar = goog.require('example.Bar');", - "goog.require('example.controller');", - "", - "alert(example.controller.SOME_CONSTANT);", - "alert(Bar.doThings);")); - } - @Test public void testShortRequireInGoogModule1() { assertChanges( @@ -1613,6 +1578,40 @@ public void testExtraRequire_unsorted() { "alert(goog.dom.createElement('div'));")); } + // TODO(tjgq): Make this not crash on ClosureRewriteModule#updateGoogRequire. + @Ignore + @Test + public void testNoCrashOnInvalidMultiRequireStatement() { + assertNoChanges( + fileWithImports( + "const a = goog.require('a'), b = goog.require('b');", useInCode("a", "b"))); + } + + private String fileWithImports(String... imports) { + return lines( + lines("/*", " * @fileoverview", " */", "goog.module('x');", ""), + lines(imports), + lines("", "module.exports = function() {};")); + } + + private String useInCode(String... names) { + StringBuilder sb = new StringBuilder(); + sb.append("\n"); + for (String name : names) { + sb.append("use(").append(name).append(");\n"); + } + return sb.toString(); + } + + private String useInType(String... names) { + StringBuilder sb = new StringBuilder(); + sb.append("\n"); + for (String name : names) { + sb.append("/** @type {!").append(name).append("} */ var _").append(name).append(";\n"); + } + return sb.toString(); + } + private void assertChanges(String originalCode, String expectedCode) { compiler.compile( ImmutableList.of(), // Externs