Make the lint warning for sorting goog.requires, and the suggested fi…

…x that adds new ones, understand the sorting rules for goog.modules, which have "shorthand" goog.requires.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=136210403

src/com/google/javascript/jscomp/NodeUtil.java

@@ -2291,7 +2291,7 @@ static boolean isBlockScopedDeclaration(Node n) {
    * @return True if {@code n} is VAR, LET or CONST
    */
   public static boolean isNameDeclaration(Node n) {
-    return n.isVar() || n.isLet() || n.isConst();
+    return n != null && (n.isVar() || n.isLet() || n.isConst());
   }
 
   /**

src/com/google/javascript/jscomp/lint/CheckRequiresAndProvidesSorted.java

@@ -53,6 +53,7 @@ public final class CheckRequiresAndProvidesSorted extends AbstractShallowCallbac
 
   private List<Node> requires;
   private List<Node> provides;
+  private boolean containsStandaloneRequire = false;
   private boolean containsShorthandRequire = false;
 
   private final AbstractCompiler compiler;
@@ -73,30 +74,45 @@ public void hotSwapScript(Node scriptRoot, Node originalRoot) {
     NodeTraversal.traverseEs6(compiler, scriptRoot, this);
   }
 
-  private final Function<Node, String> getNamespace =
+  public static final Function<Node, String> getSortKey =
       new Function<Node, String>() {
         @Override
         public String apply(Node n) {
-          Preconditions.checkState(n.isCall(), n);
-          return n.getLastChild().getString();
+          if (n.isExprResult()) {
+            // Case 1: goog.require('a.b.c');
+            return n.getFirstChild().getLastChild().getString();
+          } else if (NodeUtil.isNameDeclaration(n)) {
+            if (n.getFirstChild().isName()) {
+              // Case 2: var x = goog.require('w.x');
+              return n.getFirstChild().getString();
+            } else if (n.getFirstChild().isDestructuringLhs()) {
+              // Case 3: var {y} = goog.require('w.x');
+              // All case 3 nodes should come after all case 2 nodes.
+              Node pattern = n.getFirstFirstChild();
+              Preconditions.checkState(pattern.isObjectPattern(), pattern);
+              return "{" + pattern.getFirstChild().getString();
+            }
+          }
+          throw new IllegalArgumentException("Unexpected node " + n);
         }
       };
 
-  private final Ordering<Node> alphabetical = Ordering.natural().onResultOf(getNamespace);
+  private final Ordering<Node> alphabetical = Ordering.natural().onResultOf(getSortKey);
 
   @Override
   public void visit(NodeTraversal t, Node n, Node parent) {
     switch (n.getToken()) {
       case SCRIPT:
-        // For now, don't report for requires being out of order if there are
-        // "var x = goog.require('goog.x');" style requires.
-        if (!containsShorthandRequire) {
-          reportIfOutOfOrder(requires, REQUIRES_NOT_SORTED);
+        // For now, don't report for requires being out of order if there both
+        // "const x = goog.require('goog.x');" and "goog.require('goog.y')" style requires.
+        // TODO(tbreisacher): Put all the shorthand ones first, and then all the standalone ones.
+        if (containsShorthandRequire && containsStandaloneRequire) {
+          return;
         }
+
+        reportIfOutOfOrder(requires, REQUIRES_NOT_SORTED);
         reportIfOutOfOrder(provides, PROVIDES_NOT_SORTED);
-        requires.clear();
-        provides.clear();
-        containsShorthandRequire = false;
+        reset();
         break;
       case CALL:
         Node callee = n.getFirstChild();
@@ -112,18 +128,20 @@ public void visit(NodeTraversal t, Node n, Node parent) {
             return;
           }
           if (callee.matchesQualifiedName("goog.require")) {
-            requires.add(n);
+            requires.add(parent);
+            containsStandaloneRequire = true;
           } else {
             if (!requires.isEmpty()) {
               t.report(n, PROVIDES_AFTER_REQUIRES);
             }
             if (callee.matchesQualifiedName("goog.provide")) {
-              provides.add(n);
+              provides.add(parent);
             }
           }
         } else if (NodeUtil.isNameDeclaration(parent.getParent())
             && callee.matchesQualifiedName("goog.require")) {
           containsShorthandRequire = true;
+          requires.add(parent.getParent());
         }
         break;
       default:
@@ -146,4 +164,11 @@ private void reportIfOutOfOrder(List<Node> requiresOrProvides, DiagnosticType wa
           JSError.make(requiresOrProvides.get(0), warning, Joiner.on('\n').join(correctOrder)));
     }
   }
+
+  private void reset() {
+    requires.clear();
+    provides.clear();
+    containsShorthandRequire = false;
+    containsStandaloneRequire = false;
+  }
 }

src/com/google/javascript/refactoring/ErrorToFixMapper.java

@@ -22,6 +22,7 @@
 import com.google.javascript.jscomp.JSError;
 import com.google.javascript.jscomp.NodeTraversal;
 import com.google.javascript.jscomp.NodeUtil;
+import com.google.javascript.jscomp.lint.CheckRequiresAndProvidesSorted;
 import com.google.javascript.rhino.IR;
 import com.google.javascript.rhino.JSDocInfo;
 import com.google.javascript.rhino.Node;
@@ -310,11 +311,6 @@ private static SuggestedFix getFixForUnsortedRequiresOrProvides(
     return fix.replaceRange(first, last, newContent).build();
   }
 
-  private static String getNamespaceFromClosureNode(Node exprResult) {
-    Preconditions.checkState(exprResult.isExprResult());
-    return exprResult.getFirstChild().getLastChild().getString();
-  }
-
   private static class RequireProvideSorter extends NodeTraversal.AbstractShallowCallback
       implements Comparator<Node> {
     private final String closureFunction;
@@ -330,6 +326,9 @@ public final void visit(NodeTraversal nodeTraversal, Node n, Node parent) {
           && parent.isExprResult()
           && n.getFirstChild().matchesQualifiedName(closureFunction)) {
         calls.add(parent);
+      } else if (NodeUtil.isNameDeclaration(parent)
+          && n.getFirstFirstChild().matchesQualifiedName(closureFunction)) {
+        calls.add(parent);
       }
     }
 
@@ -339,8 +338,8 @@ public void sortCallsAlphabetically() {
 
     @Override
     public int compare(Node n1, Node n2) {
-      String namespace1 = getNamespaceFromClosureNode(n1);
-      String namespace2 = getNamespaceFromClosureNode(n2);
+      String namespace1 = CheckRequiresAndProvidesSorted.getSortKey.apply(n1);
+      String namespace2 = CheckRequiresAndProvidesSorted.getSortKey.apply(n2);
       return namespace1.compareTo(namespace2);
     }
   }

src/com/google/javascript/refactoring/Matchers.java

@@ -210,6 +210,17 @@ public static Matcher functionCall(final String name) {
     };
   }
 
+  public static Matcher googRequire(final String namespace) {
+    return new Matcher() {
+      @Override
+      public boolean matches(Node node, NodeMetadata metadata) {
+        return functionCall("goog.require").matches(node, metadata)
+            && node.getSecondChild().isString()
+            && node.getSecondChild().getString().equals(namespace);
+      }
+    };
+  }
+
   public static Matcher googRequire() {
     return functionCall("goog.require");
   }

src/com/google/javascript/refactoring/SuggestedFix.java

@@ -553,9 +553,6 @@ public Builder addGoogRequire(Match m, String namespace) {
       if (existingNode != null) {
         return this;
       }
-      Node googRequireNode = IR.exprResult(IR.call(
-          IR.getprop(IR.name("goog"), IR.string("require")),
-          IR.string(namespace)));
 
       // Find the right goog.require node to insert this after.
       Node script = NodeUtil.getEnclosingScript(node);
@@ -565,6 +562,21 @@ public Builder addGoogRequire(Match m, String namespace) {
       if (script.getFirstChild().isModuleBody()) {
         script = script.getFirstChild();
       }
+
+      Node googRequireNode = IR.call(
+          IR.getprop(IR.name("goog"), IR.string("require")),
+          IR.string(namespace));
+
+      // The name that will be used on the LHS, if the require is added using the shorthand form.
+      String shortName = namespace.substring(namespace.lastIndexOf('.') + 1);
+
+      if (script.isModuleBody()) {
+        // TODO(tbreisacher): Switch to IR.const() once ES6+ is on by default everywhere.
+        googRequireNode = IR.var(IR.name(shortName), googRequireNode);
+      } else {
+        googRequireNode = IR.exprResult(googRequireNode);
+      }
+
       Node lastModuleOrProvideNode = null;
       Node lastGoogRequireNode = null;
       Node nodeToInsertBefore = null;
@@ -584,6 +596,12 @@ public Builder addGoogRequire(Match m, String namespace) {
               break;
             }
           }
+        } else if (NodeUtil.isNameDeclaration(child)
+            && Matchers.googRequire().matches(child.getFirstFirstChild(), metadata)) {
+          if (shortName.compareTo(child.getFirstChild().getString()) < 0) {
+            nodeToInsertBefore = child;
+            break;
+          }
         }
         child = child.getNext();
       }
@@ -630,19 +648,19 @@ public Builder removeGoogRequire(Match m, String namespace) {
 
     private 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 (child.isExprResult() && child.getFirstChild().isCall()) {
-            // TODO(mknichel): Replace this logic with a function argument
-            // Matcher when it exists.
-            Node grandchild = child.getFirstChild();
-            if (Matchers.googRequire().matches(child.getFirstChild(), metadata)
-                && grandchild.getLastChild().isString()
-                && namespace.equals(grandchild.getLastChild().getString())) {
-              return child;
-            }
+          if ((NodeUtil.isExprCall(child)
+                  && Matchers.googRequire(namespace).matches(child.getFirstChild(), metadata))
+              || (NodeUtil.isNameDeclaration(child)
+                  && Matchers.googRequire(namespace)
+                      .matches(child.getFirstFirstChild(), metadata))) {
+            return child;
           }
           child = child.getNext();
         }
@@ -687,7 +705,7 @@ private static boolean isInClosurizedFile(Node node, NodeMetadata metadata) {
 
       Node child = script.getFirstChild();
       while (child != null) {
-        if (child.isExprResult() && child.getFirstChild().isCall()) {
+        if (NodeUtil.isExprCall(child)) {
           if (Matchers.googRequire().matches(child.getFirstChild(), metadata)) {
             return true;
           }

test/com/google/javascript/jscomp/lint/CheckRequiresAndProvidesSortedTest.java

@@ -106,30 +106,47 @@ public void testB3473189() {
   }
 
   /**
-   * If a goog.module uses the "var x = goog.require('x')" form, don't warn.
+   * Contains both the standalone and the shorthand style of goog.require: No warning.
    */
-  public void testGoogModuleWithShorthand() {
-    testSame(
+  public void testGoogModule_bothStyles() {
+    testSameEs6(
         LINE_JOINER.join(
             "goog.module('m');",
             "",
             "goog.require('a.c');",
-            "var d = goog.require('a.b.d');",
+            "const d = goog.require('a.b.d');",
             "goog.require('a.b.c');",
             "",
             "alert(1);"));
 
-    testSame(
+  }
+
+  public void testGoogModule_shorthand() {
+    testWarning(
         LINE_JOINER.join(
             "goog.module('m');",
             "",
             "var d = goog.require('a.b.d');",
             "var c = goog.require('a.c');",
             "",
-            "alert(1);"));
+            "alert(1);"),
+        REQUIRES_NOT_SORTED);
+  }
+
+  public void testGoogModule_shorthand_destructuring() {
+    testWarningEs6(
+        LINE_JOINER.join(
+            "goog.module('m');",
+            "",
+            "var a = goog.require('a.b.d');",
+            "var {b} = goog.require('a.a.a');",
+            "var c = goog.require('a.c');",
+            "",
+            "alert(1);"),
+        REQUIRES_NOT_SORTED);
   }
 
-  public void testGoogModuleNoShorthand() {
+  public void testGoogModule_standalone() {
     testWarning(
         LINE_JOINER.join(
             "goog.module('m');",