-
Notifications
You must be signed in to change notification settings - Fork 708
Special-case JSDocVariadicType in JSDocSignature #1444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Special-case JSDocVariadicType in JSDocSignature #1444
Conversation
Currently in Corsa, `...T` is always interpreted as `T[]`, which is considerably simpler than in Strada. However, there is no way to express rest parameters in a `@callback` or `@overload` tag. This PR special-cases `...` in `@param` tags in JSDocSignature, which is how these tags parse, such that instead of `...T --> T[]`, it reparses as `...T --> `...paramName: T`. Notes: 1. This is how it used in unifier, which is a heavily jsdoc'ed codebase checked with Strada. 2. This PR *removes* the variadic type in reparsing, meaning that the author needs to provide an array type, like `...[number, number]` for example. This is still a subset of the Strada semantics. 3. Code that relies on Strada treating `...` both as rest *and* as array type in this position gets an error that an array type is required. This should help other uses update to Corsa semantics. The new semantics are inconsistent but they are still fairly simple and should match with how people use `...T` in Strada. See how the new test based on example code from unifiedjs uses `...T` to mean rest parameter on `@overload` and to mean `T[]` on a `@param` of the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements special handling for JSDoc variadic types (...T
) in JSDoc signatures (used in @callback
and @overload
tags). Instead of always converting ...T
to T[]
, the change enables rest parameter syntax (...paramName: T
) when variadic types appear in JSDoc signatures, while maintaining the array conversion behavior in other contexts.
Key changes:
- Modified JSDoc signature reparsing to detect variadic types and convert them to rest parameters
- Added test coverage for
@overload
scenarios with variadic parameters - Updated baseline expectations to reflect the new rest parameter semantics
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
File | Description |
---|---|
testdata/tests/cases/conformance/jsdoc/jsdocVariadicInOverload.ts |
New test case demonstrating variadic types in @overload tags, based on unifiedjs code |
testdata/baselines/reference/submodule*/callbackTagVariadicType.* |
Updated baselines showing variadic types now generate rest parameters with different error messages |
testdata/baselines/reference/conformance/jsdocVariadicInOverload.* |
New baseline files showing expected output for the new test case |
internal/parser/reparser.go |
Core implementation detecting JSDocVariadicType and converting to rest parameter syntax |
Comments suppressed due to low confidence (1)
testdata/tests/cases/conformance/jsdoc/jsdocVariadicInOverload.ts:47
- The test uses undefined variables 'x', 'y', 'z' which will cause TypeScript errors unrelated to the feature being tested. Consider defining these variables with appropriate types or using literal values to focus the test on variadic parameter behavior.
p.use(x, y, z);
testdata/baselines/reference/submodule/conformance/callbackTagVariadicType.js.diff
Show resolved
Hide resolved
/** | ||
* @callback Foo | ||
* @param {...string} args | ||
~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shows the old, still unsupported semantics where the parameter args
becomes a rest parameter and the type string
becomes string[]
. Only the first happens now, based on seeing how a real person (Titus Woormer) wrote overloads.
testdata/baselines/reference/submodule/conformance/callbackTagVariadicType.js.diff
Show resolved
Hide resolved
I am testing Typescript 7's JS support, which I've largely rewritten during the switch to Go. The new code processes tags by converting them into synthetic nodes just after parsing their host node. For unified, that means that `@template` tags can't be shared between multiple `@overload` tags like before. This PR also has to convert function declarations with `@type` on them to variables initialised with a function expression. TS7 doesn't currently support this feature on function declarations--but both unified and svelte use this feature a lot, so I think we'll need to add it before TS7's JS features are ready. At that point the shuffling of functions in the test files won't be needed anymore. In addition, microsoft/typescript-go#1444 makes it possible to have rest parameters in `@overload`, and is needed for this change to work in TS7, so it won't compile with the nightly until that PR is merged. Because TS7 is quite a way off, I don't know whether you'll want to take this PR. I created it to see how hard it would be to update popular JS code that uses TS for checking.
Currently in Corsa,
...T
is always interpreted asT[]
, which is considerablysimpler than in Strada. However, there is no
way to express rest parameters in a
@callback
or@overload
tag. ThisPR special-cases
...
in@param
tags in JSDocSignature, which is how these tags parse,such that instead of
...T --> T[]
,it reparses as
...T --> ...paramName: T
.Notes:
codebase checked with Strada.
author needs to provide an array type, like
...[number, number]
forexample. This is still a subset of the Strada semantics.
...
both as rest and asarray type in this position gets an error that an array type is
required. This should help other uses update to Corsa semantics.
The new semantics are inconsistent but they are still fairly simple and
should match with how people use
...T
in Strada. See how the new testbased on example code from unifiedjs uses
...T
to mean rest parameteron
@overload
and to meanT[]
on a@param
of the implementation.