JSDoc @type tag optional parameters #48132

jablko · 2022-03-05T16:22:15Z

TS obeys JSDoc @type tag signatures, but not parameter optionality. Projects work around this by adding | void to parameters in @type tags, but this isn't strictly correct, and leads to problems later using those values, without Exclude<param, void>. e.g.

🧐 Investigation

The signatures passed to resolveCall() ignore @type tags entirely, so they have the wrong minArgumentCount, so hasCorrectArity() fails. I think they obey @param tags?

Otherwise, the applicability checks obey @type tags, because getSignatureApplicabilityError() calls getTypeAtPosition(), which ... calls getTypeForVariableLikeDeclaration(), which contains:

                if (isInJSFile(declaration)) {
                    const type = getParameterTypeOfTypeTag(func, declaration);
                    if (type) return type;
                }

since #26694 (or #22692?).

Backing up, resolveCallExpression() calls resolveCall() on callSignatures returned by getSignaturesOfType() (with the wrong minArgumentCount).

getSignaturesOfType() calls ... getSignatureFromDeclaration(), which obeys @param tags, but not @type tags?

So maybe the solution is to (roughly) copy the @type tag logic from getTypeForVariableLikeDeclaration() -> getSignatureFromDeclaration()? That's what I've tried to do in this PR.

🔍 Search terms

jsdoc type tag optional parameters signature arity

🕗 Version & regression information

2.8.0-dev.20180320 - 4.7.0-dev.20220305

⏯️ Playground link

Workbench repro

🧑‍💻 Code

// @checkJs
// @filename: input.js
// @errors: 2345

/**
 * @type {(options?: 'optional') => void}
 */
export function plugin(options) {}

plugin("optional"); // ✔️ No errors, as expected
plugin("bogus"); // ✔️ Argument type error, as expected
plugin(); // ❌ Unexpected arity error

🙁 Actual behavior

$ tsc --noEmit --checkJs input.js 
input.js:11:8 - error TS2345: Argument of type '"bogus"' is not assignable to parameter of type '"optional"'.

11 plugin("bogus"); // ✔️ Argument type error, as expected
          ~~~~~~~

input.js:12:1 - error TS2554: Expected 1 arguments, but got 0.

12 plugin(); // ❌ Unexpected arity error
   ~~~~~~~~

  input.js:8:24
    8 export function plugin(options) {}
                             ~~~~~~~
    An argument for 'options' was not provided.


Found 2 errors in the same file, starting at: input.js:11

🙂 Expected behavior

With this PR:

$ tsc --noEmit --checkJs input.js 
input.js:11:8 - error TS2345: Argument of type '"bogus"' is not assignable to parameter of type '"optional"'.

11 plugin("bogus"); // ✔️ Argument type error, as expected
          ~~~~~~~


Found 1 error in input.js:11

typescript-bot · 2022-03-05T16:22:20Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

sandersn

I think this basic approach is good enough, although it would be slightly more accurate to copy over the properties from signature to a fresh signature whose symbol is the function value instead of the @type annotation.

Why are there so many deleted baseline files? After that I think this will be ready to merge.

sandersn · 2022-03-14T16:39:34Z

tests/baselines/reference/jsdocTypeTagRequiredParameters.errors.txt

@@ -21,9 +21,9 @@ tests/cases/conformance/jsdoc/a.js(13,1): error TS2554: Expected 1 arguments, bu
    g() // should error
    ~~~
 !!! error TS2554: Expected 1 arguments, but got 0.
-!!! related TS6210 tests/cases/conformance/jsdoc/a.js:5:12: An argument for 's' was not provided.
+!!! related TS6210 tests/cases/conformance/jsdoc/a.js:4:13: An argument for 's' was not provided.


This error span change shows that re-using the signature isn't precisely correct. It's probably close enough, though, and it would take much more code to copy over the properties of the declared signature to the new signature.

jakebailey · 2022-03-14T21:06:06Z

This seems okay to fix the bug, but I'm not sure exactly if the changes in the diff are "good" or not. It seems like what's happening is that the declared parameter name (in the JS) is getting totally overridden by the jsdoc in the symbols output, which feels odd. I'm not sure what practical effect is has, though.

tests/baselines/reference/callbackTag1.types

jablko

@jakebailey Thanks for reviewing!

I rebased, which eliminated the unrelated, deleted baselines (they were for tests that no longer exist).

jablko · 2022-03-15T19:10:16Z

src/compiler/checker.ts

@@ -12762,10 +12760,8 @@ namespace ts {
        }

        function getSignatureFromDeclaration(declaration: SignatureDeclaration | JSDocSignature): Signature {
-            if (isInJSFile(declaration)) {


I noticed getSignatureOfTypeTag() already calls isInJSFile(), so I dropped that condition from this change, and from the other getSignatureOfTypeTag()/getParameterTypeOfTypeTag() call sites. It's a cosmetic change I'm happy to skip though, if you prefer?

jablko · 2022-03-15T19:18:04Z

tests/baselines/reference/callbackTag1.types

@@ -20,7 +20,7 @@ var sid = s => s + "!";
 /** @type {NoReturn} */
 var noreturn = obj => void obj.title
 >noreturn : NoReturn
->obj => void obj.title : (obj: { e: number; m: number; title: string; }) => any
+>obj => void obj.title : (s: { e: number; m: number; title: string; }) => any


These .types baselines are okay and expected, I think? We're now obeying the @type tag, so the output is, e.g. the NoReturn type, and the NoReturn parameter name is s.

jablko · 2022-03-19T15:17:05Z

I noticed that there's currently an error if an initializer isn't compatible with it's target, and that this PR was suppressing that, by applying @type tags to both the target and the inializer.

Excluding isFunctionExpressionOrArrowFunction() and isObjectLiteralMethod() restored the error.
There's already a separate check that function and method declarations are @type tag compatible, in checkFunctionOrMethodDeclaration(). That one's unaffected by this PR.
I added on to tests/cases/conformance/jsdoc/checkJsdocTypeTag6.ts, to confirm initializer ≠ @type incompatibility is reported.

⏯️ Playground link

Workbench repro.

🧑‍💻 Code

// @checkJs
// @filename: input.js
// @errors: 2322 8030

// Confirm initializers are compatible.
// They can't have more parameters than the target.

/** @type {() => void} */
function func(more) {}

/** @type {() => void} */
const variable = function (more) {};

/** @type {() => void} */
const arrow = (more) => {};

({
  /** @type {() => void} */
  method(more) {},

  /** @type {() => void} */
  prop: function (more) {},

  /** @type {() => void} */
  arrow: (more) => {},
});

📓 TIL

Some notes from exploring the code (not necessarily of benefit to anyone but myself):

@type tags are applied to variables and properties in getTypeForVariableLikeDeclaration():

// Use type from type annotation if one is present
const declaredType = tryGetTypeFromEffectiveTypeNode(declaration);

applied to object literal methods in getTypeOfVariableOrParameterOrPropertyWorker():

else if (isObjectLiteralMethod(declaration)) {
  type = tryGetTypeFromEffectiveTypeNode(declaration) || checkObjectLiteralMethod(declaration, CheckMode.Normal);
}

Q: Why aren't they analogously applied to functions and methods in getTypeOfFuncClassEnumModule()?
A: Because, unlike variables, functions can have multiple, merged declarations. It instead creates an anonymous object type:
```
const type = createObjectType(ObjectFlags.Anonymous, symbol);
```
which is later resolved by resolveAnonymousTypeMembers() -> getSignaturesOfSymbol(), but this was ignoring @type tags.

src/compiler/checker.ts

tests/cases/conformance/jsdoc/checkJsdocTypeTag6.ts

sandersn · 2022-05-10T21:22:51Z

Is this ready to review again?

jablko · 2022-05-10T21:52:23Z

@sandersn Yes please.

sandersn

Looks good except for a couple of comment suggestions.

sandersn · 2022-06-02T22:41:18Z

src/compiler/checker.ts

@@ -12941,7 +12939,15 @@ namespace ts {
                        continue;
                    }
                }
-                result.push(getSignatureFromDeclaration(decl));
+                // If this is a function or method declaration, get the accurate minArgumentCount from the @type tag, if present.


Suggested change

// If this is a function or method declaration, get the accurate minArgumentCount from the @type tag, if present.

// If this is a function or method declaration, get the signature from the @type tag, if present.

sandersn · 2022-06-02T22:45:16Z

src/compiler/checker.ts

+                // If this is a variable or property declaration, apply the @type tag to it
+                // (getTypeForVariableLikeDeclaration()), not to the initializer.


I would delete this. I tried rewording and I got

Suggested change

// If this is a variable or property declaration, apply the @type tag to it

// (getTypeForVariableLikeDeclaration()), not to the initializer.

// other kinds are handled through contextual typing

which is OK, but not that valuable to state explicitly. I'll leave it up to you whether you want to keep this, since I'm probably over-familiar with the code.

@sandersn Thanks! I agree, as written the whole comment isn't that valuable ... I guess I was aiming to answer:

Q: Why are we doing this for function and method declarations?
A: For optional parameters.

Q: Why are we excluding contextually-typed kinds?
A: Because they're already handled elsewhere, and not excluding them would suppress compatibility checks.

I've added a commit with an alternative rewording (based on your suggestions), but I'm very open to going with your preference!

sandersn

Sorry I missed this until now. It's late enough in the 4.8 beta that I want to hold this for 4.9, so I'll merge it next week.

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Mar 5, 2022

jablko mentioned this pull request Mar 5, 2022

Add type for rules' callback parameter remarkjs/remark-lint#283

Closed

5 tasks

sandersn requested review from sandersn and jakebailey March 14, 2022 16:29

sandersn self-assigned this Mar 14, 2022

sandersn requested changes Mar 14, 2022

View reviewed changes

jablko force-pushed the patch-48 branch 3 times, most recently from 111a593 to 7b85698 Compare March 15, 2022 17:17

jakebailey reviewed Mar 15, 2022

View reviewed changes

tests/baselines/reference/callbackTag1.types Outdated Show resolved Hide resolved

jablko force-pushed the patch-48 branch from 13c4538 to 6d3217a Compare March 15, 2022 18:34

jablko commented Mar 15, 2022

View reviewed changes

jablko force-pushed the patch-48 branch 5 times, most recently from 5645d0d to eab90ca Compare March 17, 2022 16:54

jablko added 2 commits March 17, 2022 11:45

JSDoc @type tag optional parameters

f81bf97

Don't repeat isInJSFile() condition

e8d2570

jablko force-pushed the patch-48 branch 2 times, most recently from 5dd9a1b to f12b97f Compare March 17, 2022 20:07

typescript-bot added the Has Repro This issue has compiler-backed repros: https://aka.ms/ts-repros label Mar 19, 2022

sandersn requested changes Mar 23, 2022

View reviewed changes

src/compiler/checker.ts Show resolved Hide resolved

src/compiler/checker.ts Show resolved Hide resolved

tests/cases/conformance/jsdoc/checkJsdocTypeTag6.ts Outdated Show resolved Hide resolved

jablko force-pushed the patch-48 branch 3 times, most recently from efb13ef to 5bb7079 Compare May 7, 2022 21:39

Exclude variable initializers

7e3e4e5

jablko added 2 commits May 7, 2022 15:02

Add tests for class methods

45558c5

Don't contextually type JS function declarations

9cffbfa

jablko force-pushed the patch-48 branch from b312d59 to 9cffbfa Compare May 7, 2022 22:03

Update Baselines and/or Applied Lint Fixes

1e84bc9

sandersn requested changes Jun 2, 2022

View reviewed changes

Reword comment

8d705b7

sandersn approved these changes Aug 1, 2022

View reviewed changes

sandersn merged commit 35c6fbf into microsoft:main Aug 10, 2022

	// If this is a function or method declaration, get the accurate minArgumentCount from the @type tag, if present.
	// If this is a function or method declaration, get the signature from the @type tag, if present.

		// If this is a variable or property declaration, apply the @type tag to it
		// (getTypeForVariableLikeDeclaration()), not to the initializer.

	// If this is a variable or property declaration, apply the @type tag to it
	// (getTypeForVariableLikeDeclaration()), not to the initializer.
	// other kinds are handled through contextual typing

JSDoc @type tag optional parameters #48132

JSDoc @type tag optional parameters #48132

Uh oh!

Conversation

jablko commented Mar 5, 2022

🧐 Investigation

🔍 Search terms

🕗 Version & regression information

⏯️ Playground link

🧑‍💻 Code

🙁 Actual behavior

🙂 Expected behavior

Uh oh!

typescript-bot commented Mar 5, 2022

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn Mar 14, 2022

Choose a reason for hiding this comment

Uh oh!

jakebailey commented Mar 14, 2022

Uh oh!

Uh oh!

jablko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jablko Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

jablko Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

jablko commented Mar 19, 2022

⏯️ Playground link

🧑‍💻 Code

📓 TIL

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandersn commented May 10, 2022

Uh oh!

jablko commented May 10, 2022

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

sandersn Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

jablko Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jablko left a comment •

edited

Loading

jablko Jun 20, 2022 •

edited

Loading