JSDoc @type tag optional parameters

[jablko]

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.

• https://github.com/remarkjs/remark-lint/blob/96e8c4f543a0b13d14470819b2a7278d11b4afe3/packages/unified-lint-rule/index.d.ts#L22
• https://github.com/remarkjs/remark-validate-links/blob/0489d132d2354af0ca29e37f4e88c6ce660587fe/lib/index.js#L54

🧐 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]

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]

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]

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.

[jablko]

@jakebailey Thanks for reviewing!

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

[jablko]

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]

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]

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.

[sandersn]

Is this ready to review again?

[jablko]

@sandersn Yes please.

[sandersn]

Looks good except for a couple of comment suggestions.

[sandersn]

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]

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.

[jablko]

@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.