-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Add typedef declaration space, unify typedef name gathering #18172
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
Conversation
src/compiler/core.ts
Outdated
@@ -2458,6 +2458,11 @@ namespace ts { | |||
throw e; | |||
} | |||
|
|||
export function assertNever(_member: never, message?: string, stackCrawlMark?: Function): never { | |||
fail(message, stackCrawlMark || assertNever); | |||
throw "Unreachable"; |
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.
You could make fail
return never
and write return fail(message, stackCrawlMark || assertNever)
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.
It's nice to check that the type of the member is never
though. Or did you just mean to change the implementation of assertNever
instead of to replace it?
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.
I think you just mean that throw "Unreachable";
is not required if fail
just returns never
- which seems fine to me; so I've done that.
@@ -2098,6 +2138,7 @@ namespace ts { | |||
|
|||
export interface JSDoc extends Node { | |||
kind: SyntaxKind.JSDocComment; | |||
parent?: HasJSDoc; |
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.
& Node
?
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.
All members of HasJSDoc
are already nodes. The interface HasJSDocNodes
would require that. Is that confusing? Should I rename the second to something like JSDocContainer
?
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.
JSDocContainer seems like a better name
…en errorUnusedLocal
…row to satisfy checker
01acb14
to
ca8d298
Compare
And strengthen
errorUnusedLocal
to no longer error when a declaration name cannot be found.Fixes #18097
There were actually two separate but related crashes in that issue - one because
getDeclarationSpaces
did not contain a clause for jsdoc typedefs, and one becausegetNameOfDeclaration
did not know how to find the name for a jsdoc typedef when its own.name
field was not provided - I've unified the codepath used by the binder to get the name with the path used ingetNameOfDeclaration
, so minimally if the symbol is bound, then it should be nameable now. In doing so I found that we had some test cases which were unnameable but bound or nameable but unbound, and added clauses to the newgetNameOfJSDocTypedef
based on where I saw either naming or binding occur prior to this change, so both should always be possible in both cases now. To do this, I wanted to ensure I was being exhaustive with the nodes checked; so I've also separated the jsdoc node properties from theNode
interface and created aHasJSDoc
union which represents all nodes which the parser applies JSDoc to. In doing so, I actually found a few types where we could have been applying jsdoc, but were not - which we now are.