-
Notifications
You must be signed in to change notification settings - Fork 763
Fix completions in JSX tag with unstructured spreads #2335
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
Fix completions in JSX tag with unstructured spreads #2335
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.
Pull request overview
This PR fixes a bug where completions in JSX tags with unstructured spread elements would cause a crash. The fix adds a type check before attempting to access properties on spread types, preventing nil pointer dereferences when the spread type is primitive (e.g., any, unknown, never, number).
Key Changes
- Added a
TypeFlagsStructuredTypecheck before callingAsStructuredType().Properties()to prevent crashes on primitive types - Added comprehensive test coverage for various spread element types in JSX tags, including both structured types (objects, unions) and primitive types (any, unknown, never, undefined, number, boolean)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/ls/completions.go |
Adds type flag validation to ensure only structured types (Object, Union, Intersection) attempt property access, preventing nil pointer dereferences |
internal/fourslash/tests/completionsInJsxTagDifferentSpreadElementTypes_test.go |
New test file verifying completions work correctly for JSX spread elements with various type categories |
internal/fourslash/tests/completionsInJsxTagDifferentSpreadElementTypes_test.go
Show resolved
Hide resolved
| } | ||
|
|
||
| // Very unexpected, but still structured (union) types. | ||
| // `boolean` is `true | false` and an optional `null` is really `null | undefined`. |
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 can't put ` in the test content, unless you concatenate the pieces (it's very annoying) 🙁
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.
Oof!
Fixes #2332.