-
-
Notifications
You must be signed in to change notification settings - Fork 380
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. Choose a reason for hiding this commentThe 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.
Suggested change
local maxUnionVariants = config.get(uri, 'Lua.type.maxUnionVariants') or0
local maxUnionVariants = getMaxUnionVariants(uri)
Copilot uses AI. Check for mistakes. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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. |
||
| local i = 0 | ||
| for n in child:eachObject() do | ||
| i = i + 1 | ||
| if i > 100 then | ||
| if maxUnionVariants > 0 and i > maxUnionVariants then | ||
| break | ||
| end | ||
| if vm.getNodeName(n) then | ||
|
|
@@ -403,10 +404,11 @@ function vm.isSubType(uri, child, parent, mark, errs) | |
| else | ||
| local weakNil = config.get(uri, 'Lua.type.weakNilCheck') | ||
| local skipTable | ||
| local maxUnionVariants = config.get(uri, 'Lua.type.maxUnionVariants') or 0 | ||
| local i = 0 | ||
| for n in child:eachObject() do | ||
| i = i + 1 | ||
| if i > 100 then | ||
| if maxUnionVariants > 0 and i > maxUnionVariants then | ||
| break | ||
| end | ||
| if skipTable == nil and n.type == "table" and parent.type == "vm.node" then -- skip table type check if child has class | ||
|
|
@@ -473,10 +475,11 @@ function vm.isSubType(uri, child, parent, mark, errs) | |
| parent = global | ||
| elseif parent.type == 'vm.node' then | ||
| local hasKnownType = 0 | ||
| local maxUnionVariants = config.get(uri, 'Lua.type.maxUnionVariants') or 0 | ||
| local i = 0 | ||
| for n in parent:eachObject() do | ||
| i = i + 1 | ||
| if i > 100 then | ||
| if maxUnionVariants > 0 and i > maxUnionVariants then | ||
| break | ||
| end | ||
| if vm.getNodeName(n) then | ||
|
|
||