No error on unmatchable @param tags (#22510)

sandersn · web-flow · commit 677d860b4413 · 2018-03-14T10:17:54.000-07:00
* No errr on unmatchable `@param` tags

Such as when the initializer is not a function, or when the function
mentions `arguments` in its body.

* Do not require dummy param for JS uses of arguments

1. JS functions that use `arguments` do not require a dummy parameter in
order to get a type for the synthetic `args` parameter if there is an
`@param` with a `...` type.
2.JS functions that use `arguments` and have an `@param` must have a
type that is a `...` type.

* Check for array type instead of syntactic `...`

* Address PR comments

* Update baselines
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -6753,17 +6753,20 @@ namespace ts {
             return links.resolvedSignature;
         }
 
+        /**
+         * A JS function gets a synthetic rest parameter if it references `arguments` AND:
+         * 1. It has no parameters but at least one `@param` with a type that starts with `...`
+         * OR
+         * 2. It has at least one parameter, and the last parameter has a matching `@param` with a type that starts with `...`
+         */
         function maybeAddJsSyntheticRestParameter(declaration: SignatureDeclaration, parameters: Symbol[]): boolean {
-            // JS functions get a free rest parameter if:
-            // a) The last parameter has `...` preceding its type
-            // b) It references `arguments` somewhere
+            if (!containsArgumentsReference(declaration)) {
+                return false;
+            }
             const lastParam = lastOrUndefined(declaration.parameters);
-            const lastParamTags = lastParam && getJSDocParameterTags(lastParam);
+            const lastParamTags = lastParam ? getJSDocParameterTags(lastParam) : getJSDocTags(declaration).filter(isJSDocParameterTag);
             const lastParamVariadicType = firstDefined(lastParamTags, p =>
                 p.typeExpression && isJSDocVariadicType(p.typeExpression.type) ? p.typeExpression.type : undefined);
-            if (!lastParamVariadicType && !containsArgumentsReference(declaration)) {
-                return false;
-            }
 
             const syntheticArgsSymbol = createSymbol(SymbolFlags.Variable, "args" as __String);
             syntheticArgsSymbol.type = lastParamVariadicType ? createArrayType(getTypeFromTypeNode(lastParamVariadicType.type)) : anyArrayType;
@@ -21440,9 +21443,24 @@ namespace ts {
         function checkJSDocParameterTag(node: JSDocParameterTag) {
             checkSourceElement(node.typeExpression);
             if (!getParameterSymbolFromJSDoc(node)) {
-                error(node.name,
-                    Diagnostics.JSDoc_param_tag_has_name_0_but_there_is_no_parameter_with_that_name,
-                    idText(node.name.kind === SyntaxKind.QualifiedName ? node.name.right : node.name));
+                const decl = getHostSignatureFromJSDoc(node);
+                // don't issue an error for invalid hosts -- just functions --
+                // and give a better error message when the host function mentions `arguments`
+                // but the tag doesn't have an array type
+                if (decl) {
+                    if (!containsArgumentsReference(decl)) {
+                        error(node.name,
+                            Diagnostics.JSDoc_param_tag_has_name_0_but_there_is_no_parameter_with_that_name,
+                            idText(node.name.kind === SyntaxKind.QualifiedName ? node.name.right : node.name));
+                    }
+                    else if (findLast(getJSDocTags(decl), isJSDocParameterTag) === node &&
+                        node.typeExpression && node.typeExpression.type &&
+                        !isArrayType(getTypeFromTypeNode(node.typeExpression.type))) {
+                        error(node.name,
+                              Diagnostics.The_last_param_tag_of_a_function_that_uses_arguments_must_have_an_array_type,
+                              idText(node.name.kind === SyntaxKind.QualifiedName ? node.name.right : node.name));
+                    }
+                }
             }
         }
 
@@ -24470,18 +24488,19 @@ namespace ts {
             const paramTag = parent.parent;
             if (isJSDocTypeExpression(parent) && isJSDocParameterTag(paramTag)) {
                 // Else we will add a diagnostic, see `checkJSDocVariadicType`.
-                const param = getParameterSymbolFromJSDoc(paramTag);
-                if (param) {
-                    const host = getHostSignatureFromJSDoc(paramTag);
+                const host = getHostSignatureFromJSDoc(paramTag);
+                if (host) {
                     /*
-                    Only return an array type if the corresponding parameter is marked as a rest parameter.
+                    Only return an array type if the corresponding parameter is marked as a rest parameter, or if there are no parameters.
                     So in the following situation we will not create an array type:
                         /** @param {...number} a * /
                         function f(a) {}
                     Because `a` will just be of type `number | undefined`. A synthetic `...args` will also be added, which *will* get an array type.
                     */
-                    const lastParamDeclaration = host && last(host.parameters);
-                    if (lastParamDeclaration.symbol === param && isRestParameter(lastParamDeclaration)) {
+                    const lastParamDeclaration = lastOrUndefined(host.parameters);
+                    const symbol = getParameterSymbolFromJSDoc(paramTag);
+                    if (!lastParamDeclaration ||
+                        symbol && lastParamDeclaration.symbol === symbol && isRestParameter(lastParamDeclaration)) {
                         return createArrayType(type);
                     }
                 }
diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json
@@ -3720,6 +3720,10 @@
         "category": "Error",
         "code": 8028
     },
+    "The last @param tag of a function that uses 'arguments' must have an array type.": {
+        "category": "Error",
+        "code": 8029
+    },
     "Only identifiers/qualified-names with optional type arguments are currently supported in a class 'extends' clause.": {
         "category": "Error",
         "code": 9002
diff --git a/tests/baselines/reference/jsdocPrefixPostfixParsing.types b/tests/baselines/reference/jsdocPrefixPostfixParsing.types
@@ -16,7 +16,7 @@
  * @param {...number?[]!} k - (number[] | null)[]
  */
 function f(x, y, z, a, b, c, d, e, f, g, h, i, j, k) {
->f : (x: number[], y: number[], z: number[], a: (number | null)[], b: number[] | null, c: number[] | null, d: number | null | undefined, e: number | null | undefined, f: number | null | undefined, g: number | null | undefined, h: number | null | undefined, i: number[] | undefined, j: number[] | null | undefined, ...args: (number | null)[][]) => void
+>f : (x: number[], y: number[], z: number[], a: (number | null)[], b: number[] | null, c: number[] | null, d: number | null | undefined, e: number | null | undefined, f: number | null | undefined, g: number | null | undefined, h: number | null | undefined, i: number[] | undefined, j: number[] | null | undefined, k: (number | null)[] | undefined) => void
 >x : number[]
 >y : number[]
 >z : number[]
diff --git a/tests/baselines/reference/paramTagOnCallExpression.symbols b/tests/baselines/reference/paramTagOnCallExpression.symbols
@@ -0,0 +1,14 @@
+=== tests/cases/conformance/jsdoc/decls.d.ts ===
+declare function factory(type: string): {};
+>factory : Symbol(factory, Decl(decls.d.ts, 0, 0))
+>type : Symbol(type, Decl(decls.d.ts, 0, 25))
+
+=== tests/cases/conformance/jsdoc/a.js ===
+// from util
+/** @param {function} ctor - A big long explanation follows */
+exports.inherits = factory('inherits')
+>exports.inherits : Symbol(inherits, Decl(a.js, 0, 0))
+>exports : Symbol(inherits, Decl(a.js, 0, 0))
+>inherits : Symbol(inherits, Decl(a.js, 0, 0))
+>factory : Symbol(factory, Decl(decls.d.ts, 0, 0))
+
diff --git a/tests/baselines/reference/paramTagOnCallExpression.types b/tests/baselines/reference/paramTagOnCallExpression.types
@@ -0,0 +1,17 @@
+=== tests/cases/conformance/jsdoc/decls.d.ts ===
+declare function factory(type: string): {};
+>factory : (type: string) => {}
+>type : string
+
+=== tests/cases/conformance/jsdoc/a.js ===
+// from util
+/** @param {function} ctor - A big long explanation follows */
+exports.inherits = factory('inherits')
+>exports.inherits = factory('inherits') : {}
+>exports.inherits : {}
+>exports : typeof "tests/cases/conformance/jsdoc/a"
+>inherits : {}
+>factory('inherits') : {}
+>factory : (type: string) => {}
+>'inherits' : "inherits"
+
diff --git a/tests/baselines/reference/paramTagOnFunctionUsingArguments.errors.txt b/tests/baselines/reference/paramTagOnFunctionUsingArguments.errors.txt
@@ -0,0 +1,31 @@
+tests/cases/conformance/jsdoc/a.js(2,20): error TS8029: The last @param tag of a function that uses 'arguments' must have an array type.
+tests/cases/conformance/jsdoc/a.js(19,9): error TS2345: Argument of type '1' is not assignable to parameter of type 'string'.
+
+
+==== tests/cases/conformance/jsdoc/decls.d.ts (0 errors) ====
+    declare function factory(type: string): {};
+==== tests/cases/conformance/jsdoc/a.js (2 errors) ====
+    /**
+     * @param {string} first
+                       ~~~~~
+!!! error TS8029: The last @param tag of a function that uses 'arguments' must have an array type.
+     */
+    function concat(/* first, second, ... */) {
+      var s = ''
+      for (var i = 0, l = arguments.length; i < l; i++) {
+        s += arguments[i]
+      }
+      return s
+    }
+    
+    /**
+     * @param {...string} strings
+     */
+    function correct() {
+        arguments
+    }
+    
+    correct(1,2,3) // oh no
+            ~
+!!! error TS2345: Argument of type '1' is not assignable to parameter of type 'string'.
+    
diff --git a/tests/baselines/reference/paramTagOnFunctionUsingArguments.symbols b/tests/baselines/reference/paramTagOnFunctionUsingArguments.symbols
@@ -0,0 +1,47 @@
+=== tests/cases/conformance/jsdoc/decls.d.ts ===
+declare function factory(type: string): {};
+>factory : Symbol(factory, Decl(decls.d.ts, 0, 0))
+>type : Symbol(type, Decl(decls.d.ts, 0, 25))
+
+=== tests/cases/conformance/jsdoc/a.js ===
+/**
+ * @param {string} first
+ */
+function concat(/* first, second, ... */) {
+>concat : Symbol(concat, Decl(a.js, 0, 0))
+
+  var s = ''
+>s : Symbol(s, Decl(a.js, 4, 5))
+
+  for (var i = 0, l = arguments.length; i < l; i++) {
+>i : Symbol(i, Decl(a.js, 5, 10))
+>l : Symbol(l, Decl(a.js, 5, 17))
+>arguments.length : Symbol(IArguments.length, Decl(lib.d.ts, --, --))
+>arguments : Symbol(arguments)
+>length : Symbol(IArguments.length, Decl(lib.d.ts, --, --))
+>i : Symbol(i, Decl(a.js, 5, 10))
+>l : Symbol(l, Decl(a.js, 5, 17))
+>i : Symbol(i, Decl(a.js, 5, 10))
+
+    s += arguments[i]
+>s : Symbol(s, Decl(a.js, 4, 5))
+>arguments : Symbol(arguments)
+>i : Symbol(i, Decl(a.js, 5, 10))
+  }
+  return s
+>s : Symbol(s, Decl(a.js, 4, 5))
+}
+
+/**
+ * @param {...string} strings
+ */
+function correct() {
+>correct : Symbol(correct, Decl(a.js, 9, 1))
+
+    arguments
+>arguments : Symbol(arguments)
+}
+
+correct(1,2,3) // oh no
+>correct : Symbol(correct, Decl(a.js, 9, 1))
+
diff --git a/tests/baselines/reference/paramTagOnFunctionUsingArguments.types b/tests/baselines/reference/paramTagOnFunctionUsingArguments.types
@@ -0,0 +1,57 @@
+=== tests/cases/conformance/jsdoc/decls.d.ts ===
+declare function factory(type: string): {};
+>factory : (type: string) => {}
+>type : string
+
+=== tests/cases/conformance/jsdoc/a.js ===
+/**
+ * @param {string} first
+ */
+function concat(/* first, second, ... */) {
+>concat : (...args: any[]) => string
+
+  var s = ''
+>s : string
+>'' : ""
+
+  for (var i = 0, l = arguments.length; i < l; i++) {
+>i : number
+>0 : 0
+>l : number
+>arguments.length : number
+>arguments : IArguments
+>length : number
+>i < l : boolean
+>i : number
+>l : number
+>i++ : number
+>i : number
+
+    s += arguments[i]
+>s += arguments[i] : string
+>s : string
+>arguments[i] : any
+>arguments : IArguments
+>i : number
+  }
+  return s
+>s : string
+}
+
+/**
+ * @param {...string} strings
+ */
+function correct() {
+>correct : (...args: string[]) => void
+
+    arguments
+>arguments : IArguments
+}
+
+correct(1,2,3) // oh no
+>correct(1,2,3) : void
+>correct : (...args: string[]) => void
+>1 : 1
+>2 : 2
+>3 : 3
+
diff --git a/tests/cases/conformance/jsdoc/paramTagOnCallExpression.ts b/tests/cases/conformance/jsdoc/paramTagOnCallExpression.ts
@@ -0,0 +1,10 @@
+// @noEmit: true
+// @allowJs: true
+// @checkJs: true
+// @Filename: decls.d.ts
+declare function factory(type: string): {};
+// @Filename: a.js
+
+// from util
+/** @param {function} ctor - A big long explanation follows */
+exports.inherits = factory('inherits')
diff --git a/tests/cases/conformance/jsdoc/paramTagOnFunctionUsingArguments.ts b/tests/cases/conformance/jsdoc/paramTagOnFunctionUsingArguments.ts
@@ -0,0 +1,27 @@
+// @noEmit: true
+// @allowJs: true
+// @checkJs: true
+// @strict: true
+// @Filename: decls.d.ts
+declare function factory(type: string): {};
+// @Filename: a.js
+
+/**
+ * @param {string} first
+ */
+function concat(/* first, second, ... */) {
+  var s = ''
+  for (var i = 0, l = arguments.length; i < l; i++) {
+    s += arguments[i]
+  }
+  return s
+}
+
+/**
+ * @param {...string} strings
+ */
+function correct() {
+    arguments
+}
+
+correct(1,2,3) // oh no
