Skip to content
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

Fix name resolution in typedef and allow defaults for template tags #45483

Merged
merged 5 commits into from
Sep 9, 2021

Conversation

rbuckton
Copy link
Member

This adds support for Type Parameter defaults in JavaScript files using the JSDoc @template tag. The implementation borrows from the syntax used for parameter defaults in JSDoc (namely [foo=1]):

/**
 * @template {string} [T="hello"]
 * @typedef {T} Foo
 */

This also fixes a name resolution bug when checking the constraint of a @template tag.

Fixes #45480
Fixes #45481

@rbuckton rbuckton requested review from sandersn and weswigham August 17, 2021 03:24
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 17, 2021
@rbuckton rbuckton force-pushed the jsDocTemplateTagDefault branch from 80fa780 to 3d9187f Compare August 17, 2021 22:25
@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 17, 2021
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one request in the parser and one in the tests.

src/compiler/parser.ts Outdated Show resolved Hide resolved
src/compiler/parser.ts Outdated Show resolved Hide resolved
@rbuckton
Copy link
Member Author

I'm debating whether [T] should be allowed, and if so, if it should default to the constraint or unknown. Given the changes necessary to support that, I may look at that in a separate PR instead.

@rbuckton
Copy link
Member Author

@sandersn can you take another look?

@rbuckton
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 28, 2021

Heya @rbuckton, I've started to run the parallelized community code test suite on this PR at 9131c0c. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 28, 2021

Heya @rbuckton, I've started to run the extended test suite on this PR at 9131c0c. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@rbuckton
Copy link
Member Author

@andrewbranch
Copy link
Member

@rbuckton FYI you can run @typescript-bot user test this inline to get a user test diff between this and current main, with results posted in the PR.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 30, 2021

Heya @andrewbranch, I've started to run the inline community code test suite on this PR at 9131c0c. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@andrewbranch
Great news! no new errors were found between main..refs/pull/45483/merge

@rbuckton
Copy link
Member Author

Wasn't aware of the inline, and I'm not sure why that's not the default behavior, tbh.

@rbuckton
Copy link
Member Author

rbuckton commented Sep 4, 2021

@DanielRosenwasser: Do we want to discuss the syntax for this in a design meeting, or is this ok to merge?

@rbuckton
Copy link
Member Author

rbuckton commented Sep 8, 2021

TODO:

  • Verify JS declaration emit.

@rbuckton rbuckton merged commit cf787e9 into main Sep 9, 2021
@rbuckton rbuckton deleted the jsDocTemplateTagDefault branch September 9, 2021 00:05
@DanielRosenwasser
Copy link
Member

So the syntax

@template {string} [T="hello"]

means

T extends string = "hello"

I can't wait to never get this right. 😂

@thw0rted
Copy link

thw0rted commented Sep 20, 2021

Hi @rbuckton , I noticed that none of the files changed in this PR seem to be documentation (HTML/MD). Is that hosted in a different repo? Is there a task / issue to track inclusion of this syntax in the handbook?

@orta
Copy link
Contributor

orta commented Nov 3, 2021

Just noting that the new JSDoc reference on the site includes this

robertknight added a commit to hypothesis/client that referenced this pull request Mar 24, 2022
Remove a cryptic piece of code that is no longer needed given the ability to
specify default values for template arguments [1]

[1] microsoft/TypeScript#45483
robertknight added a commit to hypothesis/client that referenced this pull request Mar 24, 2022
Remove a cryptic piece of code that is no longer needed given the ability to
specify default values for template arguments [1]

[1] microsoft/TypeScript#45483
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
8 participants