-
-
Notifications
You must be signed in to change notification settings - Fork 376
fix: large unions no longer erroneously fail to match later variants #3208
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
I think you can also link #3126 in your pr description, as they are essentially the same issue 👀
Close #3178 and close #3126 by adding ...
=> so both issues can be closed automatically when this pr is merged
I think you can also link #3126 in your pr description, as they are essentially the same issue 👀
Thanks, I've edited the description to include that.
@Copilot
Copilot
AI
left a comment
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 an issue where large unions were failing to match later variants by introducing a configurable limit on the number of union variants checked. Key changes include:
- Replacing the fixed union variant limit (100) with a dynamic check using the new config option Lua.type.maxUnionVariants.
- Adding the new configuration option to the config template.
- Updating the changelog to document these changes.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| script/vm/type.lua | Updated union variant iteration logic to honor the new Lua.type.maxUnionVariants setting. |
| script/config/template.lua | Added the new configuration option Lua.type.maxUnionVariants. |
| changelog.md | Documented the fix and the new configuration option related to union variant checking. |
Comments suppressed due to low confidence (1)
script/vm/type.lua:381
- [nitpick] A short inline comment explaining the intent behind the 'maxUnionVariants' check could improve clarity for future maintainers.
if maxUnionVariants > 0 and i > maxUnionVariants then
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.
[nitpick] Consider refactoring to reduce duplication by extracting the retrieval of 'maxUnionVariants' to a helper function or common code block if similar logic appears in multiple branches.
Copilot uses AI. Check for mistakes.
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.
should default 100?
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.
I decided to change to default to no limit, because I think that the language server should prioritise correctness over performance by default. I think it should be up to the user to decide that they can afford to sacrifice correctness for performance. It's a lot more confusing for a user to see the language server giving an incorrect diagnostic by default (and then needing to figure out there's a setting to change the behaviour) than it would be for a user to see poor performance.
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.
More pertinently, this pull request wouldn't actually resolve the linked issues if the maximum value was left configured to 100.
Uh oh!
There was an error while loading. Please reload this page.
Close #3178 and close #3126 by adding a config option
Lua.type.maxUnionVariantsto control how many union variants a value is checked against. By default, the setting is set to 0, meaning that the language server will check all union variants.Further work to improve ergonomics with this setting might include adding a warning when a union type is defined that has too many variants.