When sorting goog.require's, stop when encountering a non-goog.requir…

…e statement. If other statements are interleaved within the goog.requires, then this avoids removing those statements. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=164639505
google · Aug 8, 2017 · 3462da4 · 3462da4
1 parent 501a3f1
commit 3462da4
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 3 deletions.
diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java
@@ -2620,7 +2620,7 @@ static boolean isValidCfgRoot(Node n) {
    * @return Whether the node is used as a statement.
    */
   public static boolean isStatement(Node n) {
-    return isStatementParent(n.getParent());
+    return !n.isModuleBody() && isStatementParent(n.getParent());
   }
 
   private static final Predicate<Node> isStatement = new Predicate<Node>() {

diff --git a/src/com/google/javascript/refactoring/ErrorToFixMapper.java b/src/com/google/javascript/refactoring/ErrorToFixMapper.java
@@ -380,15 +380,20 @@ private static SuggestedFix getFixForUnsortedRequiresOrProvides(
     return fix.replaceRange(first, last, newContent).build();
   }
 
-  private static class RequireProvideSorter extends NodeTraversal.AbstractShallowCallback
-      implements Comparator<Node> {
+  private static class RequireProvideSorter implements NodeTraversal.Callback, Comparator<Node> {
     private final ImmutableSet<String> closureFunctions;
     private final List<Node> calls = new ArrayList<>();
+    private boolean finished = false;
 
     RequireProvideSorter(String... closureFunctions) {
       this.closureFunctions = ImmutableSet.copyOf(closureFunctions);
     }
 
+    @Override
+    public final boolean shouldTraverse(NodeTraversal nodeTraversal, Node n, Node parent) {
+      return !finished;
+    }
+
     @Override
     public final void visit(NodeTraversal nodeTraversal, Node n, Node parent) {
       if (n.isCall()
@@ -401,6 +406,9 @@ && matchName(n.getFirstChild())) {
           && matchName(n.getLastChild().getFirstChild())) {
         checkState(n.isName() || n.isDestructuringLhs(), n);
         calls.add(parent);
+      } else if (!calls.isEmpty() && parent != null && NodeUtil.isStatement(parent)) {
+        // Reached a non-goog.(require|provide|forwardDeclare) statement, so stop.
+        finished = true;
       }
     }
 

diff --git a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java
@@ -585,6 +585,32 @@ public void testSortRequiresInGoogModule_withFwdDeclare() {
             "alert(r, s);"));
   }
 
+  @Test
+  public void testSortRequiresInGoogModule_withOtherStatements() {
+    // The requires after "const {Bar} = bar;" are not sorted.
+    assertChanges(
+        LINE_JOINER.join(
+            "goog.module('x');",
+            "",
+            "const foo = goog.require('foo');",
+            "const bar = goog.require('bar');",
+            "const {Bar} = bar;",
+            "const util = goog.require('util');",
+            "const {doCoolThings} = util;",
+            "",
+            "doCoolThings(foo, Bar);"),
+        LINE_JOINER.join(
+            "goog.module('x');",
+            "",
+            "const bar = goog.require('bar');",
+            "const foo = goog.require('foo');",
+            "const {Bar} = bar;",
+            "const util = goog.require('util');",
+            "const {doCoolThings} = util;",
+            "",
+            "doCoolThings(foo, Bar);"));
+  }
+
   @Test
   public void testSortRequiresAndForwardDeclares() {
     assertChanges(