Bind object type literals in JSDoc #51066
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DanielRosenwasser
force-pushed
the
bindObjectTypesInJSDoc
branch
from
01cbedf
to
1d901bf
Compare
|
@typescript-bot run dt slower |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at 1d901bf. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at 1d901bf. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at 1d901bf. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the Definitely Typed test suite on this PR at 1d901bf. You can monitor the build here. |
|
I'm somewhat nervous about binding more things in JSDoc just because we seem to have existing issues with JSDoc nodes being bound more than once; see #50069. If we already bind JSDoc nodes like this, then I'm less worried, but if we normally only set the parent and such, I don't quite know what'll happen if we visit these nodes more than once. |
|
@DanielRosenwasser Here are the results of running the user test suite comparing Everything looks good! |
|
We already bind these nodes in JS files - so I think we should be good. If it were up to me, I would just do a full bind and decide do be more selective on what JSDoc does in TS files, but this is a more conservative first step. |
sandersn
requested changes
Oct 5, 2022
| setParent(node, parent); | ||
| const saveParent = parent; | ||
| parent = node; | ||
| if (node.kind === SyntaxKind.TypeLiteral) { |
There was a problem hiding this comment.
won't this also bind type literals inside a @typedef? Maybe it won't matter because there's some required intermediate binding needed for the checker to check the type.
| @@ -0,0 +1,47 @@ | |||
| // === /a.ts === | |||
| // /** | |||
| // * @type {"[|foo|]" | "bar"} | |||
There was a problem hiding this comment.
I think find-all-refs should ignore @type in ts files and not offer completions either.
|
@DanielRosenwasser Here they are:Comparison Report - main..51066
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsapache/echarts
|
weswigham
reviewed
Mar 15, 2023
There was a problem hiding this comment.
I think, to pair with current work to try to parse jsdoc maybe a bit more lazily, we'd want to think of an approach where we could bind these lazily - maybe on demand in the services layer, since for normal compiler error-checking operation, we don't really want to consider them. (But we do want references in them to be available in find-all-refs in services, I assume!)
But, ignoring that, I think binding these is a pretty required step to get decent ls results that involve them. I guess the interesting part is that we want to be able to know these reference other things, but not let them make new references in TS code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Object type literals are a specific instance of types that really need to have their own symbol table for language service operations.
We could try binding everything from JSDoc, but that does "extra" work like declaring types from JSDoc, which is undesirable. Instead, this PR just adds a lightweight walk to perform local binding within type literals in JSDoc, rather than parameterizing
bindWorker.It might be the case that we have to account for function types, constructor types, and mapped types, which all implicitly create their own symbol tables, and that might be fine too.
Fixes #51054
Alternative fix for #50750
Ref #51057, #51055
CC @a-tarasyuk who's been working on these issues.