-
Notifications
You must be signed in to change notification settings - Fork 725
Fixed document highlight for reference directive #1951
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
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.
Seems right, but do you have a test that can show this?
| parent := node.Parent | ||
| if parent == nil { | ||
| return AccessKindRead | ||
| } |
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.
Ah yes, Strada said:
function accessKind(node: Node): AccessKind {
const { parent } = node;
switch (parent?.kind) {Love the ol "parent is never undefined except when it totally is"
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.
Test
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 crash and incorrect behavior in document highlighting for reference directives. The fix addresses issue #1892 where requesting highlights on a reference directive path would crash due to a nil parent in accessKind and fail to return proper highlight ranges.
Key changes:
- Added nil check in
accessKindto prevent crashes when processing reference directive nodes - Changed
getReferencedSymbolsForModuleto be a method onLanguageServiceto access LSP range creation utilities - Fixed reference entry creation for reference directives to return proper text ranges instead of node-based entries
- Removed unnecessary node nil check in
getSemanticDocumentHighlightsthat was preventing reference directive highlights
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ast/ast.go | Added nil parent guard in accessKind and fixed missing closing brace |
| internal/ls/findallreferences.go | Changed getReferencedSymbolsForModule to instance method and fixed reference entry creation for directives |
| internal/ls/documenthighlights.go | Removed unnecessary node nil check that blocked reference directive highlights |
| internal/fourslash/tests/documentHighlightReferenceDirective_test.go | Added test case for document highlighting on reference directives |
| testdata/baselines/reference/fourslash/documentHighlights/documentHighlightReferenceDirective.baseline.jsonc | Added baseline output for the new test |
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.
LGTM, ast package fix is sufficient to fix the crash but the other stuff appears to improve the output.
Fixes #1892
Fixed crash produced by nil parent in
accessKind, then fixed incorrect behavior infindAllReferences.goanddocumentHighlights.go. ThetextRangefrom the reference wasn't being returned bygetReferencedSymbolsForModule, andgetSemanticHighlightsunnecessarily checked the status of the reference's node.