Update typechecker passes to understand REST parameters

Also adds some features to ScopeSubject to aid testing.

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

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

@@ -533,14 +533,20 @@ FunctionTypeBuilder inferParameterTypes(@Nullable Node argsParent,
         (info == null) ? new HashSet<>() : new HashSet<>(info.getParameterNames());
     boolean isVarArgs = false;
     for (Node arg : argsParent.children()) {
+      boolean isOptionalParam = false;
+      if (arg.isRest()) {
+        isVarArgs = true;
+        arg = arg.getOnlyChild();
+      } else {
+        isVarArgs = isVarArgsParameter(arg, info);
+        isOptionalParam = isOptionalParameter(arg, info);
+      }
+
       String argumentName = arg.getString();
       allJsDocParams.remove(argumentName);
 
       // type from JSDocInfo
       JSType parameterType = null;
-      boolean isOptionalParam = isOptionalParameter(arg, info);
-      isVarArgs = isVarArgsParameter(arg, info);
-
       if (info != null && info.hasParameterType(argumentName)) {
         parameterType =
             info.getParameterType(argumentName).evaluate(scope, typeRegistry);

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

@@ -873,6 +873,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
       case WHILE:
       case FOR:
       case TEMPLATELIT_SUB:
+      case REST:
         typeable = false;
         break;
 

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

@@ -155,6 +155,12 @@ private void inferArguments(TypedScope functionScope) {
       if (parameterTypes != null) {
         Node parameterTypeNode = parameterTypes.getFirstChild();
         for (Node astParameter : astParameters.children()) {
+          if (astParameter.isRest()) {
+            // e.g. `function f(p1, ...restParamName) {}`
+            // set astParameter = restParamName
+            astParameter = astParameter.getOnlyChild();
+          }
+          checkState(astParameter.isName(), astParameter);
           TypedVar var = functionScope.getVar(astParameter.getString());
           checkNotNull(var);
           if (var.isTypeInferred() && var.getType() == unknownType) {

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

@@ -1255,6 +1255,7 @@ void defineSlot(Node n, JSType type, boolean inferred) {
             parent.isFunction()
                 || NodeUtil.isNameDeclaration(parent)
                 || parent.isParamList()
+                || (parent.isRest() && parent.getParent().isParamList())
                 || parent.isCatch());
       } else {
         checkArgument(n.isGetProp() && (parent.isAssign() || parent.isExprResult()));
@@ -1459,7 +1460,8 @@ private TypedScope getLValueRootScope(Node n) {
       Node root = NodeUtil.getBestLValueRoot(n);
       if (root != null) {
         if (root.isName()) {
-          switch (root.getParent().getToken()) {
+          Node nameParent = root.getParent();
+          switch (nameParent.getToken()) {
             case VAR:
               return currentHoistScope;
             case LET:
@@ -1469,6 +1471,12 @@ private TypedScope getLValueRootScope(Node n) {
             case PARAM_LIST:
             case CATCH:
               return currentScope;
+
+            case REST:
+              // TODO(bradfordcsmith): Handle array destructuring REST
+              checkState(nameParent.getParent().isParamList(), nameParent);
+              return currentScope;
+
             default:
               TypedVar var = currentScope.getVar(root.getString());
               if (var != null) {
@@ -2206,8 +2214,21 @@ private void declareArguments(Node functionNode) {
         if (jsDocParameters != null) {
           Node jsDocParameter = jsDocParameters.getFirstChild();
           for (Node astParameter : astParameters.children()) {
+            boolean isRestParameter = false;
+            if (astParameter.isRest()) {
+              // e.g. `function f(p1, ...restParamName) {}`
+              // set astParameter = restParamName
+              astParameter = astParameter.getOnlyChild();
+              isRestParameter = true;
+            }
+            checkState(astParameter.isName(), astParameter);
             JSType paramType = jsDocParameter == null ? unknownType : jsDocParameter.getJSType();
             boolean inferred = paramType == null || paramType.equals(unknownType);
+            if (isRestParameter) {
+              // rest parameter is actually an array of the type specified in the JSDoc
+              ObjectType arrayType = typeRegistry.getNativeObjectType(ARRAY_TYPE);
+              paramType = typeRegistry.createTemplatizedType(arrayType, paramType);
+            }
 
             if (iifeArgumentNode != null && inferred) {
               String argumentName = iifeArgumentNode.getQualifiedName();

test/com/google/javascript/jscomp/ScopeSubject.java

@@ -24,6 +24,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.truth.FailureMetadata;
 import com.google.common.truth.Subject;
+import com.google.javascript.jscomp.testing.TypeSubject;
 import com.google.javascript.rhino.Node;
 import javax.annotation.CheckReturnValue;
 import javax.annotation.Nullable;
@@ -103,43 +104,49 @@ private void expectScope(String preposition, Object expected, AbstractScope<?, ?
     }
 
     /** Expects the declared variable to be declared on the subject scope. */
-    public void directly() {
+    public DeclarationSubject directly() {
       expectScope("", "directly", actual());
+      return this;
     }
 
     /** Expects the declared variable to be declared on the given scope. */
-    public void on(AbstractScope<?, ?> scope) {
+    public DeclarationSubject on(AbstractScope<?, ?> scope) {
       checkState(
           scope != actual(),
           "It doesn't make sense to pass the scope already being asserted about. Use .directly()");
       expectScope("on", scope, scope);
+      return this;
     }
 
     /** Expects the declared variable to be declared on the closest container scope. */
-    public void onClosestContainerScope() {
+    public DeclarationSubject onClosestContainerScope() {
       expectScope("on", "the closest container scope", actual().getClosestContainerScope());
+      return this;
     }
 
     /** Expects the declared variable to be declared on the closest hoist scope. */
-    public void onClosestHoistScope() {
+    public DeclarationSubject onClosestHoistScope() {
       expectScope("on", "the closest hoist scope", actual().getClosestHoistScope());
+      return this;
     }
 
     /** Expects the declared variable to be declared on the global scope. */
-    public void globally() {
+    public DeclarationSubject globally() {
       expectScope("", "globally", actual().getGlobalScope());
+      return this;
     }
 
     /** Expects the declared variable to be declared on any scope other than the subject. */
-    public void onSomeParent() {
+    public DeclarationSubject onSomeParent() {
       if (var != null && var.getScope() == actual()) {
         failWithBadResults(
             "declares " + var.getName(), "on a parent scope", "declares it", "directly");
       }
+      return this;
     }
 
     /** Expects the declared variable to be declared on some scope with the given label. */
-    public void onScopeLabeled(String expectedLabel) {
+    public DeclarationSubject onScopeLabeled(String expectedLabel) {
       checkNotNull(expectedLabel);
       String actualLabel = getLabel(var.getScopeRoot());
       if (actualLabel == null) {
@@ -155,6 +162,12 @@ public void onScopeLabeled(String expectedLabel) {
             "declares it",
             "on a scope labeled \"" + actualLabel + "\"");
       }
+      return this;
+    }
+
+    public TypeSubject withTypeThat() {
+      TypedVar typedVar = (TypedVar) var.getSymbol();
+      return TypeSubject.assertType(typedVar.getType());
     }
   }
 

test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java

@@ -368,6 +368,126 @@ public void testInferTypesFromExpressionInArgumentSpread() {
             "required: null"));
   }
 
+  public void testOnlyRestParameterWithoutJSDocCalledWithNoArgs() {
+    testTypes(
+        lines(
+            "", // input lines
+            "function use(...numbers) {}",
+            "use();", // no args provided in call - should be OK
+            ""));
+  }
+
+  public void testOnlyRestParameterWithoutJSDocCalledWithArgs() {
+    testTypes(
+        lines(
+            "", // input lines
+            "function use(...numbers) {}",
+            "use(1, 'hi', {});",
+            ""));
+  }
+
+  public void testOnlyRestParameterWithJSDocCalledWithNoArgs() {
+    testTypes(
+        lines(
+            "/**", // input lines
+            " * @param {...number} numbers",
+            " */",
+            "function use(...numbers) {}",
+            "use();", // no args provided in call - should be OK
+            ""));
+  }
+
+  public void testOnlyRestParameterWithJSDocCalledWithGoodArgs() {
+    testTypes(
+        lines(
+            "/**", // input lines
+            " * @param {...number} numbers",
+            " */",
+            "function use(...numbers) {}",
+            "use(1, 2, 3);",
+            ""));
+  }
+
+  public void testOnlyRestParameterWithJSDocCalledWithBadArg() {
+    testTypes(
+        lines(
+            "/**", // input lines
+            " * @param {...number} numbers",
+            " */",
+            "function use(...numbers) {}",
+            "use(1, 'hi', 3);",
+            ""),
+        lines(
+            "actual parameter 2 of use does not match formal parameter",
+            "found   : string",
+            // TODO(bradfordcsmith): should not allow undefined
+            // This is consistent with pre-ES6 var_args behavior.
+            // See https://github.com/google/closure-compiler/issues/2561
+            "required: (number|undefined)"
+        ));
+  }
+
+  public void testNormalAndRestParameterWithJSDocCalledWithOneArg() {
+    testTypes(
+        lines(
+            "/**", // input lines
+            " * @param {string} str",
+            " * @param {...number} numbers",
+            " */",
+            "function use(str, ...numbers) {}",
+            "use('hi');", // no rest args provided in call - should be OK
+            ""));
+  }
+
+  public void testNormalAndRestParameterWithJSDocCalledWithGoodArgs() {
+    testTypes(
+        lines(
+            "/**", // input lines
+            " * @param {string} str",
+            " * @param {...number} numbers",
+            " */",
+            "function use(str, ...numbers) {}",
+            "use('hi', 2, 3);",
+            ""));
+  }
+
+  public void testOnlyRestParameterWithJSDocCalledWithBadNormalArg() {
+    testTypes(
+        lines(
+            "/**", // input lines
+            " * @param {string} str",
+            " * @param {...number} numbers",
+            " */",
+            "function use(str, ...numbers) {}",
+            "use(1, 2, 3);",
+            ""),
+        lines(
+            "actual parameter 1 of use does not match formal parameter",
+            "found   : number",
+            "required: string"
+        ));
+  }
+
+  public void testOnlyRestParameterWithJSDocCalledWithBadRestArg() {
+    testTypes(
+        lines(
+            "/**", // input lines
+            " * @param {string} str",
+            " * @param {...number} numbers",
+            " */",
+            "function use(str, ...numbers) {}",
+            "use('hi', 'there', 3);",
+            ""),
+        lines(
+            "actual parameter 2 of use does not match formal parameter",
+            "found   : string",
+            // TODO(bradfordcsmith): should not allow undefined
+            // This is consistent with pre-ES6 var_args behavior.
+            // See https://github.com/google/closure-compiler/issues/2561
+            "required: (number|undefined)"
+        ));
+  }
+
   public void testExponent1() {
     testTypes(
         lines(

test/com/google/javascript/jscomp/TypeCheckTest.java

@@ -2382,12 +2382,29 @@ public void testFunctionArguments18() {
         "parameter y does not appear in <anonymous>'s parameter list");
   }
 
-  public void testRestParameters() {
+  public void testVarArgParameterWithTemplateType() {
     testTypesWithExterns(
         new TestExternsBuilder().addArray().build(),
         lines(
             "/**",
             " * @template T",
+            " * @param {...T} var_args",
+            " * @return {T}",
+            " */",
+            "function firstOf(var_args) { return arguments[0]; }",
+            "/** @type {null} */ var a = firstOf('hi', 1);",
+            ""),
+        lines(
+            "initializing variable", // preserve newlinues
+            "found   : (number|string)",
+            "required: null"));
+  }
+
+  public void testRestParameters() {
+    testTypesWithExterns(
+        new TestExternsBuilder().addArray().build(),
+        lines(
+            "/**",
             " * @param {...number} x",
             " */",
             "function f(...x) {",
@@ -2399,6 +2416,31 @@ public void testRestParameters() {
             "required: string"));
   }
 
+  public void testRestParameterWithTemplateType() {
+    testTypesWithExterns(
+        new TestExternsBuilder().addArray().build(),
+        lines(
+            "/**",
+            " * @template T",
+            " * @param {...T} rest",
+            " * @return {T}",
+            " */",
+            "function firstOf(...rest) {",
+            "  return rest[0];",
+            "}",
+            "/** @type {null} */ var a = firstOf('hi', 1);",
+            ""),
+        ImmutableList.of(
+            // TODO(b/79707793): This will be fixed when transpilation of REST parameters moves
+            // after type checking.
+            "Bad type annotation. Unknown type T",
+            lines(
+                "initializing variable", // preserve newlinues
+                "found   : (number|string)",
+                "required: null")),
+        /* isError= */ false);
+  }
+
   // Test that when transpiling we don't use T in the body of f; it would cause a spurious
   // unknown-type warning.
   public void testRestParametersWithGenericsNoWarning() {