Fix document highlights crashes on missing tokens/nodes#2672
Fix document highlights crashes on missing tokens/nodes#2672DanielRosenwasser merged 9 commits intomainfrom
Conversation
DanielRosenwasser
requested review from
Copilot,
gabritto,
iisaduan and
johnfav03
There was a problem hiding this comment.
Pull request overview
This pull request fixes a crash in document highlights when encountering incomplete try-catch-finally statements and malformed if-else statements. The crash occurred when the language server attempted to find catch/finally keywords that didn't exist or when incorrectly casting else statements to if statements.
Changes:
- Fixed nil pointer dereference in
getTryCatchFinallyOccurrencesby adding nil checks for catch and finally tokens - Fixed unsafe type cast in
getIfElseKeywordsby checking if the else statement is actually an if statement before casting - Added comprehensive tests for broken/incomplete control flow structures
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ls/documenthighlights.go | Fixed nil checks in getTryCatchFinallyOccurrences and unsafe cast in getIfElseKeywords |
| internal/fourslash/tests/gen/*.go | Added test cases for broken try-catch-finally, if-else, switch-case, return, and other control flow structures |
| testdata/baselines/reference/fourslash/documentHighlights/*.baseline.jsonc | Generated baseline outputs for new test cases |
| internal/fourslash/_scripts/failingTests.txt | Updated to reflect new failing test |
| internal/fourslash/_scripts/crashingTests.txt | Updated to reflect new crashing test |
| internal/fourslash/_scripts/convertFourslash.mts | Added support for test.markers() in baseline document highlights |
|
|
||
| var keywords []*ast.Node | ||
| token := lsutil.GetFirstToken(node, sourceFile) | ||
| if token.Kind == ast.KindTryKeyword { |
There was a problem hiding this comment.
This seems to be a porting oopsie because the original pushKeywordIf dealt with nil and the Go version got rid of it and those occurrences weren't handling nil. I see we have other occurrences of pushKeywordIf in Strada. Do we need to also go and fix other occurrences other than those ones in this PR?
Fixes #2670.