Repository navigation
fix(type-check): accept compatible union assignments - #1297
Conversation
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.
There was a problem hiding this comment.
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) vsstring|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_compactwith union types on both sides? Thecheck_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
- Confirm the downstream union-to-union path is exercised and correct (add a focused unit test if not already covered).
- Clarify the comment to explain the deferral.
- Add the edge-case tests listed above.
- 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.
Valid union assignments can report
assign-type-mismatchwhen different source members match different target members. Both assignments below should be accepted: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.