-
Notifications
You must be signed in to change notification settings - Fork 764
fix: Crash for completions after JSDoc * on line following string-named property signature #2342
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
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
| if tokenStart <= position && (position < tokenEnd) { | ||
| if token == ast.KindIdentifier || !ast.IsTokenKind(token) { | ||
| if ast.IsJSDocKind(current.Kind) { | ||
| if ast.IsJSDocKind(current.Kind) || ast.IsTypeElement(current) { |
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 doesn't feel like the right fix- the code path here is meant to be extremely general, and I think you can repro this for any construct that
- Contains a list of nodes (e.g. a
NodeList) - Intersperses with a
*
/**
* @type {[
* /*$*/ SomeType,
* ]}
*/error] panic handling request textDocument/completion: did not expect KindTypeReference to have KindIdentifier in its trivia
goroutine 3162 [running]:
runtime/debug.Stack()
runtime/debug/stack.go:26 +0x5e
github.com/microsoft/typescript-go/internal/lsp.(*Server).recover(0xc0001d6008, 0xc00e95fda0)
github.com/microsoft/typescript-go/internal/lsp/server.go:701 +0x4c
panic({0xbb8320?, 0xc00df28a80?})
runtime/panic.go:783 +0x132
github.com/microsoft/typescript-go/internal/astnav.getTokenAtPosition(0xc01249e008, 0x13, 0x1, 0x0)
github.com/microsoft/typescript-go/internal/astnav/tokens.go:196 +0x5f9
github.com/microsoft/typescript-go/internal/astnav.GetTokenAtPosition(...)
github.com/microsoft/typescript-go/internal/astnav/tokens.go:40
github.com/microsoft/typescript-go/internal/ls.(*LanguageService).getCompletionData(0xc00dba6240, {0x109efa8, 0xc00dba61e0}, 0xc003df8c08, 0xc01249e008, 0x13, 0xc00371f400)
github.com/microsoft/typescript-go/internal/ls/completions.go:454 +0xce
github.com/microsoft/typescript-go/internal/ls.(*LanguageService).getCompletionsAtPosition(0xc00dba6240, {0x109efa8, 0xc00dba61e0}, 0xc01249e008, 0x13, 0x0)
github.com/microsoft/typescript-go/internal/ls/completions.go:383 +0x2cf
github.com/microsoft/typescript-go/internal/ls.(*LanguageService).ProvideCompletion(0xc00dba6240, {0x109efa8, 0xc00dba61e0}, {0xc003d82390?, 0xc00dba61e0?}, {0x3d82390?, 0xc0?}, 0xc0151180f0)
github.com/microsoft/typescript-go/internal/ls/completions.go:44 +0xc8
github.com/microsoft/typescript-go/internal/lsp.(*Server).handleCompletion(0xc00110e008?, {0x109efa8?, 0xc00dba61e0?}, 0xc003d82390?, 0x40b92c?)
github.com/microsoft/typescript-go/internal/lsp/server.go:1011 +0x39
github.com/microsoft/typescript-go/internal/lsp.init.func1.registerLanguageServiceDocumentRequestHandler[...].16({0x109efa8, 0xc00dba61e0}, 0xc00e95fda0)
github.com/microsoft/typescript-go/internal/lsp/server.go:617 +0x130
github.com/microsoft/typescript-go/internal/lsp.(*Server).handleRequestOrNotification(0xc0001d6008, {0x109efe0?, 0xc00f516410?}, 0xc00e95fda0)
github.com/microsoft/typescript-go/internal/lsp/server.go:501 +0x14b
github.com/microsoft/typescript-go/internal/lsp.(*Server).dispatchLoop.func1()
github.com/microsoft/typescript-go/internal/lsp/server.go:404 +0x3a
created by github.com/microsoft/typescript-go/internal/lsp.(*Server).dispatchLoop in goroutine 7
github.com/microsoft/typescript-go/internal/lsp/server.go:424 +0x9ad
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.
Another example:
/**
* @type {(
* a: string
* /**/ b
* ) => string}
*/
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 in general the panic we had before was correct: if we're walking trivia for a non-JSDoc node, we shouldn't find a dangling identifier. Maybe the root problem is that if we're inside a JSDoc, but current is a non-JSDoc node, we run into problems when scanning for the in-between tokens because we're also inside JSDoc?
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 probably should have asked some preliminary question in the ticket, but I'll ask them now 😅
The panic would cause the build to fail, as I understand the code, which seems like the desired behavior if there is an additional comment, such as /*$*/ inside of a JSDoc. So what @gabritto sounds right to me. But what is the expected behavior given an example such as the on below?
/**
* @type {(
* a: string
* /**/ b
* ) => string}
*/Or is my understanding off the mark?
This is a fix for #2290.
It would seem that type elements can also be included in JSDoc comments, so a check was added in addition to
ast.ISJSDocKindto allow for property signatures among other type elements.Note: This is my first contribution to the TypeScript compiler, so if I'm totally off the mark, feel free to be blunt about it.