-
Notifications
You must be signed in to change notification settings - Fork 763
Fix panic in completions within import attributes #2293
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
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
DanielRosenwasser
left a comment
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.
@copilot go
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
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 fixes a panic that occurred when requesting completions within import attributes by replacing a problematic direct call to Node.Text() with an explicit lambda function that properly extracts and converts the attribute name.
Key changes:
- Modified
completions.goto use an explicit lambda that callsel.AsImportAttribute().Name().Text()instead of directly calling(*ast.Node).Texton ImportAttribute nodes - Added comprehensive test coverage for all three problematic completion positions within import attributes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/ls/completions.go | Replaced direct (*ast.Node).Text call with explicit lambda function that properly handles ImportAttribute nodes by extracting the name first before calling .Text() |
| internal/fourslash/tests/completionImportAttributes_test.go | Added new test covering all three marker positions from the original issue to ensure completions work within import attribute syntax |
| "github.com/microsoft/typescript-go/internal/testutil" | ||
| ) | ||
|
|
||
| // !!! can delete, there are similar tests that haven't been ported yet. |
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.
What are the tests?
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.
There's a few other completions tests like completionListForImportAttributes.ts that already work on import specifiers via baselineCompletions but I don't think we have VerifyBaselineCompletions yet.
Plan to Fix Panic in Import Attributes Completions
.Text()onImportAttributenodes in completionsNode.Text()Summary
Fixed the panic in completions request within import attributes by updating the completions code to use an explicit lambda function instead of adding a case to
Node.Text().Changes made per review feedback:
KindImportAttributecase fromNode.Text()inast.gocompletions.goline 1372 to use explicit lambda:core.Map(elements, func(el *ast.Node) string { return el.AsImportAttribute().Name().Text() })GoToEachMarkerfor cleaner iteration over all markersAll tests pass including:
TestCompletionImportAttributescovering all three marker positions from the issueSecurity Summary
No security vulnerabilities were introduced or discovered during this change.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.