No error on unmatchable `@param` tags by sandersn · Pull Request #22510 · microsoft/TypeScript

sandersn · 2018-03-13T17:45:13Z

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

This will need to be ported to 2.8 as well.

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

ghost · 2018-03-13T18:28:02Z

+// @noEmit: true
+// @allowJs: true
+// @checkJs: true
+// @strict: true


Does this matter?

No. It was part of my template when generating a bunch of these similar JSDoc bugs. I removed it.

ghost · 2018-03-13T18:28:05Z

        function checkJSDocParameterTag(node: JSDocParameterTag) {
            checkSourceElement(node.typeExpression);
-            if (!getParameterSymbolFromJSDoc(node)) {
+            const decl = getHostSignatureFromJSDoc(node);


getHostSignatureFromJSDoc looks expensive and we won't use it if the param has a symbol; better to put it in an inner if.

Yeah, good idea. It's also called twice -- once in getParameterSymbolFromJSDoc -- but I don't know that it's worthwhile to fix that.

ghost · 2018-03-13T18:30:03Z

+// @Filename: a.js
+
+/**
+ * @param {string} first


This case I'm not so sure about supporting since people will think they're getting typing, when we just throw the type definition away without telling them and accept any. Instead we could add a better message telling them to use a rest parameter instead.

Rest parameters don't exist in ES5, so the error would only apply to ES2015 and later. And we don't usually know whether a .js file targets ES2015 or later.

As for throwing away typing, I don't think there's much we can do -- maybe a noImplicitAny error instead?

I think it would be easier to error (to info-error?) whenever there's a reference to arguments. That error would be a lot easier to write, and would apply to both ES2015+ and TS, and should come with an associated quick fix. I'll open a separate issue for that.

Currently we require the user to place a dummy-parameter and type it as a rest parameter:

/** @param {...string} x */ function f(x) { return arguments[0]; } f(1); // error, 1 is not a string

We could make a codefix to convert @param {string} first to @param {...string} args and add the dummy parameter -- we could also stop requiring the dumy parameter.

I like that. I'll see how hard it is to get the error for the {string} x case and then remove it when it's changed to {...string} x. We can do the code fix later, since @mhegazy wants to get this into 2.8.1.

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.

sandersn · 2018-03-13T22:12:36Z

OK, I improved the error message and type for functions that use arguments. They no longer require a dummy parameter to match the jsdoc @param. So I added the error you suggested. Let me know what you think of the wording.

ghost · 2018-03-14T15:43:22Z

+            }
            const lastParam = lastOrUndefined(declaration.parameters);
-            const lastParamTags = lastParam && getJSDocParameterTags(lastParam);
+            const lastParamTags = lastParam ? getJSDocParameterTags(lastParam) : getJSDocTags(declaration).filter((tag): tag is JSDocParameterTag => isJSDocParameterTag(tag));


(tag): tag is JSDocParameterTag => isJSDocParameterTag(tag) === isJSDocParameterTag

ghost · 2018-03-14T15:44:09Z


+        /**
+         * A JS function gets a synthetic rest parameter if it references `arguments` AND:
+         * 1. It has no parameters and at least one `@param` and the first `@param` has a type and the type starts with `...`


Comment says the first @param, but we are looping over all of them.

Good catch. I decided to change the comment in order to continue sharing most of the code here.

ghost · 2018-03-14T15:45:01Z

-                    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);
+                if (decl) {


Might want to explicitly comment that we decided not to issue errors on @param tags in invalid locations.

ghost · 2018-03-14T15:48:27Z

        "category": "Error",
        "code": 8028
    },
+    "JSDoc '...' must appear on the last @param tag of a function that uses 'arguments'.": {


This sounds like it's complaining about ... in an invalid location. Would write this backwards: the last tag must have ....

ghost · 2018-03-14T15:48:55Z

+                            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 (lastOrUndefined(getJSDocTags(decl).filter((tag): tag is JSDocParameterTag => isJSDocParameterTag(tag))) === node &&


(tag): tag is JSDocParameterTag => isJSDocParameterTag(tag) === isJSDocParameterTag. Also, inefficient to filter entire array then take the last element, instead of findLast.

ghost · 2018-03-14T16:44:20Z

                    */
-                    const lastParamDeclaration = host && last(host.parameters);
-                    if (lastParamDeclaration.symbol === param && isRestParameter(lastParamDeclaration)) {
+                    const lastParamDeclaration = host.parameters.length > 0 && last(host.parameters);


lastOrUndefined

I guess so. lastOrUndefined is surprisingly complicated though.

We could just make the implementation simpler? #22572

sandersn · 2018-03-14T17:17:48Z

No errors and maxOf2: (a: number, b: number, ...args: any[]) => any.

The error reporting in checkJSDocParameterTag only happens if there is no matching symbol for each @param tag, which there is in this case. And the code in getSignatureFromDeclaration notices the use of arguments with no matching @param, so it adds the synthetic rest parameter.

ghost · 2018-03-14T17:55:18Z

The error message currently says The last @param tag of a function that uses 'arguments' must have an array type., which isn't true if it could match the last parameter. Maybe it should say "@param tag has no matching parameter. It could match 'arguments' if it had an array type." or something.

sandersn · 2018-03-14T18:09:33Z

Or maybe just "The last unmatched @param ..." ? I think that makes the conditions too complex to read easily.

I like "JSDoc @param tag has name {0} but there is no parameter with that name. It would match arguments if it had an array type." That way the normal error is a substring of the special-case one.

* 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

sandersn · 2018-03-14T18:09:57Z

This is now in release 2.8.

ghost · 2018-03-14T18:11:55Z

JSDoc @param tag has name {0} but there is no parameter with that name. It would match arguments if it had an array type.

👍

sandersn · 2018-03-14T18:22:25Z

#22577 improves the message.

From #22510 and #22514, which remove lots of bogus `@param` errors and add lots of `Object is possibly undefined` errors, respectively. There are quite a few effects of the correct addition of undefined to types based on postfix= in jsdoc, actually.

No errr on unmatchable @param tags

0dd9a99

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

sandersn requested review from a user and mhegazy March 13, 2018 17:45

sandersn changed the title ~~No errr on unmatchable @param tags~~ No error on unmatchable @param tags Mar 13, 2018

ghost suggested changes Mar 13, 2018

View reviewed changes

ghost reviewed Mar 14, 2018

View reviewed changes

sandersn added 2 commits March 14, 2018 09:31

Check for array type instead of syntactic ...

43e20d0

Merge branch 'master' into no-error-on-unmatchable-param-tags

b252e1c

ghost reviewed Mar 14, 2018

View reviewed changes

sandersn added 2 commits March 14, 2018 09:49

Address PR comments

24e014d

Update baselines

70af333

ghost approved these changes Mar 14, 2018

View reviewed changes

sandersn merged commit 677d860 into master Mar 14, 2018

sandersn deleted the no-error-on-unmatchable-param-tags branch March 14, 2018 17:18

sandersn mentioned this pull request Mar 14, 2018

Update user tests for new jsdoc fixes #22582

Merged

microsoft locked and limited conversation to collaborators Jul 25, 2018

Conversation

sandersn commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Mar 13, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn commented Mar 14, 2018

Uh oh!

ghost commented Mar 14, 2018

Uh oh!

sandersn commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn commented Mar 14, 2018

Uh oh!

ghost commented Mar 14, 2018

Uh oh!

sandersn commented Mar 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ghost Mar 13, 2018 •

edited by ghost

Loading

sandersn commented Mar 14, 2018 •

edited

Loading