fix: large unions no longer erroneously fail to match later variants #3208
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 |
Thanks, I've edited the description to include that. |
There was a problem hiding this comment.
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
| @@ -374,10 +374,11 @@ function vm.isSubType(uri, child, parent, mark, errs) | |||
| elseif child.type == 'vm.node' then | |||
| if config.get(uri, 'Lua.type.weakUnionCheck') then | |||
| local hasKnownType = 0 | |||
| local maxUnionVariants = config.get(uri, 'Lua.type.maxUnionVariants') or 0 | |||
There was a problem hiding this comment.
[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.
| local maxUnionVariants = config.get(uri, 'Lua.type.maxUnionVariants') or 0 | |
| local maxUnionVariants = getMaxUnionVariants(uri) |
Copilot uses AI. Check for mistakes.
| @@ -374,10 +374,11 @@ function vm.isSubType(uri, child, parent, mark, errs) | |||
| elseif child.type == 'vm.node' then | |||
| if config.get(uri, 'Lua.type.weakUnionCheck') then | |||
| local hasKnownType = 0 | |||
| local maxUnionVariants = config.get(uri, 'Lua.type.maxUnionVariants') or 0 | |||
There was a problem hiding this comment.
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.
More pertinently, this pull request wouldn't actually resolve the linked issues if the maximum value was left configured to 100.
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.