-
Notifications
You must be signed in to change notification settings - Fork 759
Fix nil pointer dereference in completion at property start after JSDoc #2259
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
Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
…SDoc Fixed two issues: 1. Variable shadowing bug in getRelevantTokens where contextToken was declared with := instead of assigned with =, causing it to remain nil 2. Added nil checks in tryGetClassLikeCompletionSymbols to handle cases where contextToken is nil These changes prevent the nil pointer dereference crash reported in the issue. 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 nil pointer dereference panic that occurred when requesting completions at the beginning of a property name preceded by JSDoc in a class or interface. The crash was caused by variable shadowing in getRelevantTokens that prevented contextToken from being assigned to the return parameter, combined with missing nil checks in tryGetClassLikeCompletionSymbols.
Key changes:
- Fixed variable shadowing by changing
:=to=ingetRelevantTokensto properly assign to the return parameter - Added defensive nil checks before accessing
contextToken.KindandcontextToken.Parentto prevent panics when contextToken is nil
internal/ls/completions.go
Outdated
| // Declaring new property/method/accessor | ||
| isNewIdentifierLocation = true | ||
| if contextToken.Kind == ast.KindAsteriskToken { | ||
| if contextToken != nil && contextToken.Kind == ast.KindAsteriskToken { |
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.
We don't need to do all of those defensive checks in Strada, so we should not need them here, just need the actual fix that makes the contextToken not nil.
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 obey.
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.
Removed the defensive nil checks. The fix now only includes the variable shadowing correction in getRelevantTokens (commit 4c877f7).
Per code review feedback, removed defensive nil checks in tryGetClassLikeCompletionSymbols since the actual fix (variable shadowing in getRelevantTokens) ensures contextToken is properly set. Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
| previousToken = astnav.FindPrecedingToken(file, position) | ||
| if previousToken != nil && position <= previousToken.End() && (ast.IsMemberName(previousToken) || ast.IsKeywordKind(previousToken.Kind)) { | ||
| contextToken := astnav.FindPrecedingToken(file, previousToken.Pos()) | ||
| contextToken = astnav.FindPrecedingToken(file, previousToken.Pos()) |
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 can't possibly do anything anymore...
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're right - the variable shadowing fix alone wasn't sufficient. The issue was that tryGetObjectTypeDeclarationCompletionContainer could return a non-nil declaration even when contextToken was nil (specifically in the Identifier case at line 4166). I've added a nil check there: if contextToken != nil && isFromObjectTypeDeclaration(location) (commit 27b3371).
|
@copilot Where are the tests? Also it looks like the PR may need to merge from upstream and run |
- Fixed variable shadowing in getRelevantTokens (contextToken := to contextToken =) - Added nil check in tryGetObjectTypeDeclarationCompletionContainer for Identifier case - Added test to verify no crash when completing at property start after JSDoc - Merged from main and ran hereby generate Co-authored-by: DanielRosenwasser <972891+DanielRosenwasser@users.noreply.github.com>
Requesting completions at the beginning of a property name preceded by JSDoc caused a nil pointer panic when accessing
contextToken.Kind.Root Causes
Variable shadowing in
getRelevantTokens: Used:=instead of=, creating a local variable that shadowed the return parameter, causingcontextTokento return nil.Missing nil check in
tryGetObjectTypeDeclarationCompletionContainer: The function could return a non-nil declaration even whencontextTokenwas nil (specifically in theKindIdentifiercase whenisFromObjectTypeDeclaration(location)is true), leading to a crash when the caller tried to accesscontextToken.Kind.Changes
getRelevantTokens(line 2756) by changingcontextToken :=tocontextToken =to properly assign to the return parametertryGetObjectTypeDeclarationCompletionContainer(line 4166) by addingcontextToken != nil &&condition beforeisFromObjectTypeDeclaration(location)TestCompletionJSDocBeforePropertyNoCrashto verify the crash is fixedTesting
The fixes ensure
contextTokenis correctly set and prevent accessing it when nil, resolving the crash while maintaining correct completion behavior.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.