Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 25, 2025
Merged

fix: large unions no longer erroneously fail to match later variants #3208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
* `FIX` Prevent class methods from triggering missing-fields diagnostics
* `ADD` missing locale
* `FIX` updates the EmmyLuaCodeStyle submodule reference to a newer commit, ensuring compatibility with GCC 15
* `FIX` large unions will no longer erroneously fail to match later variants
* `ADD` Lua.type.maxUnionVariants which can be set to limit how many union variants are checked against

## 3.14.0
`2025-4-7`
Expand Down
1 change: 1 addition & 0 deletions script/config/template.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ local template = {
['Lua.language.completeAnnotation'] = Type.Boolean >> true,
['Lua.type.castNumberToInteger'] = Type.Boolean >> true,
['Lua.type.weakUnionCheck'] = Type.Boolean >> false,
['Lua.type.maxUnionVariants'] = Type.Integer >> 0,
['Lua.type.weakNilCheck'] = Type.Boolean >> false,
['Lua.type.inferParamType'] = Type.Boolean >> false,
['Lua.type.checkTableShape'] = Type.Boolean >> false,
Expand Down
9 changes: 6 additions & 3 deletions script/vm/type.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Copilot AI Jun 22, 2025

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.

Suggested change
local maxUnionVariants = config.get(uri, 'Lua.type.maxUnionVariants') or 0
local maxUnionVariants = getMaxUnionVariants(uri)

Copilot uses AI. Check for mistakes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should default 100?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading