Skip to content

fix(type-check): accept compatible union assignments - #1297

Merged
CppCXY merged 1 commit into
EmmyLuaLs:mainfrom
lewis6991:rewrite-union-assignments
Oct 16, 2026
Merged

CppCXY merged 1 commit into
EmmyLuaLs:mainfrom
lewis6991:rewrite-union-assignments

Conversation

@lewis6991

Copy link
Copy Markdown
Collaborator

Valid union assignments can report assign-type-mismatch when different source members match different target members. Both assignments below should be accepted:

---@param value true?
local function use(value)
    ---@type boolean?
    local result = value
    return result
end
---@param value string|integer
local function use(value)
    ---@type string|number|boolean
    local result = value
    return result
end

The check currently requires the whole source union to match one target member. Check each source member against the whole target union instead. This allows different source members to match different target members. Assignments still fail if any source member has no match.

These valid assignments report assign-type-mismatch:

```lua
---@PARAM value true?
local function use(value)
    ---@type boolean?
    local result = value
    return result
end
```

```lua
---@PARAM value string|integer
local function use(value)
    ---@type string|number|boolean
    local result = value
    return result
end
```

Each source member is valid for the target. The check fails because it
requires the whole source union to match one target member.

Check each source union member against the whole target union. Different
source members can then match different target members. Assignments
still fail if any source member has no match.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

Summary

The change modifies union type compatibility checking in assignment mode. Previously, a target union only needed one compatible member. Now, source union members are checked individually against target members, with a guard to avoid double-processing when the source is also a union.

Issues & Observations

1. Potential logic gap: source union vs. target union handling

The new guard && !matches!(source, LuaType::Union(_)) skips the target-union loop when the source is a union. This relies on the code below (the "check them below" path) to handle union-to-union matching. This is a significant behavioral assumption. Please verify:

  • Does the fall-through logic correctly handle partial overlaps? E.g. string|integer → string|number|boolean (accept) vs string|number → string|integer (reject). The tests cover these, but the correctness depends entirely on the downstream branch, which isn't shown in the diff.
  • Is there a risk of infinite recursion or redundant work if the downstream branch re-enters check_general_type_compact with union types on both sides? The check_guard.next_level() mechanism should bound this, but confirm the guard is applied on the new path too.

2. Comment accuracy

The comment // Source union members can match different target members; check them below. is placed on the branch that skips the loop. It would be clearer to state explicitly that union-to-union is deferred, e.g.:

// Union-to-union: defer to the general path below so each source
// member can match a different target member.

3. Test coverage gaps

The added tests are good, but consider adding:

  • Both sides union with nil: e.g. ("string|nil", "string?") — exercises optional handling on both sides.
  • Empty/never union edge cases: never → union, and union → never.
  • Nested unions: ("string|(integer|boolean)", "string|number") to ensure flattening/nesting doesn't break the new path.
  • Literal widening: ("'FILE'", "string") to confirm literal-to-base-type still works after the change.

4. Test assertion style inconsistency

test_union_assignment_accepts_each_source_member uses assert!(diagnostics.is_empty(), ...), while the reject test uses assert_eq! on codes. This is fine, but the accept test would benefit from also asserting the count is zero explicitly (it does via is_empty). No action required — just noting the asymmetry is intentional and acceptable.

5. Behavioral regression risk

The old behavior ("target union only needs one compatible member") is a widening of acceptance. The new behavior is stricter for union sources. This could surface new AssignTypeMismatch diagnostics in existing user code that previously passed. Ensure this is intended and documented in the changelog, as it may be a breaking change for downstream users.

Recommendations

  1. Confirm the downstream union-to-union path is exercised and correct (add a focused unit test if not already covered).
  2. Clarify the comment to explain the deferral.
  3. Add the edge-case tests listed above.
  4. Document the stricter union-assignment semantics as a potential breaking change.

Verdict

The change is reasonable and well-tested for the common cases, but the correctness hinges on the unshown downstream branch. Request verification of the union-to-union fall-through path before merging.

@CppCXY
CppCXY merged commit ca9bec3 into EmmyLuaLs:main Oct 16, 2026
17 checks passed
@lewis6991
lewis6991 deleted the rewrite-union-assignments branch October 16, 2026 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants