-
Notifications
You must be signed in to change notification settings - Fork 745
Fix nil pointer dereference in getAdjustedLocation for type-only exports #2090
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: jakebailey <5341706+jakebailey@users.noreply.github.com>
|
@copilot run |
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Done. The fix allowed Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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 nil pointer dereference bug in the getAdjustedLocation function that caused panics when handling textDocument/documentHighlight requests on TypeScript files with type-only export statements (e.g., export type * from "./module").
Key Changes:
- Fixed incorrect parent node traversal for ExportDeclaration handling in
getAdjustedLocation - Enabled and updated the
TestReferencesForStatementKeywordstest which now passes
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
internal/ls/utilities.go |
Corrected line 917 to use parent instead of parent.Parent when parent is already an ExportDeclaration, fixing the nil pointer dereference |
internal/fourslash/tests/gen/referencesForStatementKeywords_test.go |
Removed t.Skip() to enable the test that now passes with the fix |
internal/fourslash/_scripts/failingTests.txt |
Removed TestReferencesForStatementKeywords from the failing tests list |
testdata/baselines/reference/fourslash/findAllReferences/referencesForStatementKeywords.baseline.jsonc |
Added baseline test output showing correct behavior for finding references in type-only import/export statements |
Fix nil pointer dereference in getAdjustedLocation when handling textDocument/documentHighlight
Summary
Fixed a bug in the
getAdjustedLocationfunction that caused nil pointer dereference panics when processingtextDocument/documentHighlightrequests on TypeScript files with type-only export statements.Changes Made
parent.Parent.AsExportDeclaration()toparent.AsExportDeclaration()for ExportDeclaration nodesRoot Cause
The panic occurred when ExportDeclaration handling incorrectly accessed
parent.Parentinstead ofparentitself. Whenparentis already anExportDeclarationwithisTypeOnly, we should passparentdirectly togetAdjustedLocationForExportDeclaration, notparent.Parent.Solution
Applied minimal, surgical fix that aligns with the TypeScript reference implementation:
For the ImportClause case, the TypeScript AST definition guarantees that an
ImportClause's parent is always eitherImportDeclarationorJSDocImportTag(never nil), so no nil check is needed there.Test Results
The fix allowed
TestReferencesForStatementKeywordsto pass, which was previously failing and has been removed from the failing tests list.Testing
Security Summary
No security vulnerabilities introduced or fixed by 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.