[hgschmie]

Project missing or optional non-nil union members as nullable values through argument checks, flow, hover and callable consumers. For example, reading an integer field absent from another union arm now yields integer|nil: integer parameters reject it, while integer? and any parameters accept it.

Covers UP-123 and UP-125, refreshed onto 1b6df85942be5b76c6717b84ede386a4d48df38c. Follow-up regressions cover chained/recursive aliases, cold and warm queries, unresolved definitions, receiver nil checks, synthetic optional intersections, arrays, completion and signature help. Complete component projections are reused through the existing model cache.

Current source validation: 2,713 all-feature workspace tests pass, with 32 ignored; formatting and all-target/all-feature Clippy pass. One newly retained ignored test records a baseline-proven recursive-generic inline-object limitation; that limitation is not claimed fixed.

Separate USER approval is required before signed squash, maintained-branch synchronization, closure or integration.

[github-actions]

AI-generated review by OpenRouter Review.
Verify findings before applying changes. Feedback · Report to GitHub.

Code Review

Summary

The change removes the special-case member_missing_in_union diagnostic from param_type_check.rs and instead bakes "missing union member" semantics into the type system: member_info_impl now expands aliases before lookup and appends LuaType::Nil when a union arm lacks the accessed member. flow.rs and hover handling were updated accordingly. Tests are extensive.

Overall direction is good — moving the logic into type resolution avoids duplicated policy in the checker. However, several correctness/robustness concerns should be addressed before/after merging.

---

Significant Concerns

1. Unbounded recursion risk in the new alias fast-path (member.rs)

if alias_id
    .and_then(|id| model.type_def_of(&id))
    .is_some_and(|def| def.kind == TypeDefKind::Alias)
{
    return with_expanded_alias(model, prefix_type, |expanded| {
        member_info(model, expanded, key)
    })
    .flatten();
}

This unconditionally expands and recurses before any other resolution. A self-referential alias such as ---@alias A A|B would re-enter member_info(A, key) unless with_expanded_alias has an explicit cycle guard and returns something that terminates the recursion. The recursive_alias_member_queries_keep_concrete_alternatives test suggests a guard exists, but please confirm the case where expansion is an identity (e.g. ---@alias Alias Alias or ---@alias A A). If not guarded, this will stack-overflow for user-supplied alias definitions.

Recommendation: verify with_expanded_alias short-circuits on identity expansion, and add a regression test for a self-referential alias that resolves to itself.

2. Exponential blow-up potential in the union absence scan (member.rs)

let component_info = member_info(model, component, key);

This is called for every component of a union on every member query. If a union arm is itself an alias to another union, this causes the whole component tree to be re-walked (with nested alias expansion and nested union scans again). For deeply nested unions/aliases this is at least super-linear in the worst case.

Recommendation: consider flattening unions (or at least iterating only leaf components) before querying, and/or memoize per-(component, key) results. Even short-circuiting on the first absence would help.

3. is_complete(revision) heuristic is fragile (member.rs)

let revision = model.cache.borrow().revision();
let component_info = member_info(model, component, key);
let optional = ...;
optional || (component_info.is_none() && model.cache.borrow().is_complete(revision))

This treats any None as "truly absent" only if no cache entries were added while resolving. Because unrelated lazy type resolution can bump the revision, the "missing member" nil may be omitted on first evaluation and re-added on a subsequent one, making the reported type non-deterministic across passes. Diagnostics (and hover) may flap depending on cache warmness.

Recommendation: use an explicit cycle/recursion guard flag (or return a "resolved vs. incomplete" indicator from member_info) rather than inferring from a global revision counter. Also add a test that first evaluates the type cold (no prior caching) and then warm, asserting identical results.

4. Diagnostic message regression (union_member_argument_test.rs)

assert_that!(diagnostics[0].message, contains_substring("`uint32?`"));

The old message explicitly mentioned nil ("missing union member counts as nil"); the new message reports the surface type `uint32?`. This loses the reason for the failure. Users will find expected `integer`, got `uint32?` less actionable than the previous explanation.

Recommendation: keep surfacing the fact that a union arm lacks the member — e.g. augment mismatch_message to append "(missing member in one union alternative)".

5. flow.rs only checks expand_alias_head (one level) for union detection

matches!(
    super::type_eval::expand_alias_head(model, &ty),
    Some(LuaType::Union(_))
)

If expand_alias_head resolves only one alias step, a chain like A = B, B = C|D will not be recognized as a union prefix here, so guarded_index_type narrowing will silently be skipped. If expand_alias_head iterates until non-alias, this is fine — please confirm and add a test:

---@alias Middle ProjectionPresent|ProjectionAbsent
---@alias Outer Middle
---@param event Outer

6. flow.rs: perf and fallback interaction

For every LuaExpr::IndexExpr in every file, this now calls model.type_of_expr_at on the prefix and expand_alias_head. Then:

if union_prefix.is_none()
    && let Some(resolved) = model.resolve_member(index_expr)

When union_prefix.is_some() but member_type returns None (e.g. key genuinely absent), resolve_member is skipped entirely. If downstream code relied on member_id from resolve_member (documentation, method resolution, etc.), that path is now bypassed for union-typed prefixes. Please double-check the fall-through below the diff handles all union_prefix = Some cases, or fall back to resolve_member when member_type returns None.

---

Minor Issues

7. Test placement outside the mod test block (param_type_check_test.rs)

     }
 }
+#[test]
+fn test_nullable_union_projection_argument_contracts() {

The new tests sit outside the existing mod test { ... }, unlike the rest of the file. They still compile, but this is inconsistent and makes it easy to miss gating (#[cfg(test)]) conventions if the file changes. Move them inside mod test.

8. Confusing local name

let accepts = ws.has_no_diagnostic(DiagnosticCode::ParamTypeMismatch, &source);
...
assert!(!accepts, "{annotation}");

accepts actually stores "no diagnostics", and the assertion negates it. Rename to no_diagnostics (or assert positively).

9. Repeated imports inside tests

Every new test repeats use crate::{DiagnosticCode, VirtualWorkspace}; (and others). Prefer hoisting these to the module top (or to the mod test header) for readability.

10. Unconditional origin_typ computation in hover (build_hover.rs)

let origin_typ = semantic_type_of(&origin_model, &origin);

This runs even when &origin == decl or origin_is_param, where the result isn't used. Likely cached, but a cheap let origin_typ = ... inside the else branch would avoid the unnecessary work and keep the intended invariant obvious.

11. member_missing_in_union possibly dead code

Since the checker no longer calls semantic_model.member_missing_in_union(index_expr), verify the method is either still used or removed. If unused, delete it to prevent future readers from trusting an outdated path.

---

Positive Notes

• Moving "missing union member ⇒ nil" into type resolution is the right architectural direction; it ensures all consumers (hover, type_of_expr, checker) see a consistent type.
• The new tests are thoughtful: they cover plain unions, aliases, generics, guarded narrowing, optional fields, unresolved nominals, and copy-then-hover projection. The test_copied_union_member_hover_keeps_projected_nil around declaration-vs-use-site hover is particularly valuable.
• The check !matches!(&origin_typ, Function | DocFunction | Signature) in the projected-member decision correctly avoids new nil-ness bleeding into call signatures.
• Using guarded_index_type when the prefix is a union preserves narrowing for if event.player_index then accept(event.player_index) end, which is the subtle case that would previously have required the removed isinstance fallback.

---

Recommended Test Additions

1. Chained alias (A = B, B = C|D) to lock in #5.
2. Self-referential alias (---@alias A A|B) to lock in #1.
3. Cold vs. warm cache evaluating the same expression twice to verify #3.
4. Union with a Ref to an unloadable type def (to exercise the unresolved_nominal branch) — assert nil is not appended.
5. Union with a LuaType::Nil arm and a member lookup — verify no nil is double-added and that NeedCheckNil is not spuriously reported.

[hgschmie]

Assessed every point in review 5721585190 on exact head 525da4f95213a9a3e2bf9ff3ffb8ff7cca2d399e. No demonstrated source blocker remains.

1. with_expanded_alias uses AliasPath and the shared evaluation budget; entering an already active exact alias marks the query incomplete and terminates. Identity self-alias, recursive union-alias and concrete-alternative paths do not recurse without a guard. An additional exact-head probe verifies the pure self-alias case terminates.
2. The union scan already uses any(), so it stops at the first proven absence. Recursive work is bounded by the shared alias/evaluation machinery. Flattening or memoizing every projection is an optional performance change; no measured regression was supplied.
3. revision() is the incomplete_revision counter, incremented by mark_incomplete(), not by ordinary cache insertion. Successful lazy queries do not make a cold result differ from a warm one. The extra probe checks repeated cold/warm results for chained aliases, unresolved nominals and nil alternatives.
4. The nullable surface type is intentional: the normal mismatch now reports the same projected type as inference and hover. This preserves the reason nil is incompatible without a checker-specific alternative type policy. Adding diagnostic provenance is a separate presentation enhancement.
5. expand_alias_head resolves outer computations with an explicit work queue. It does not stop at one alias. The extra Outer -> Middle -> Present|Absent probe retains integer|nil.
6. A union follows its complete member projection; falling back to an individual resolved member would erase absent-arm nilness. Existing callable/imported-member tests retain function identity, optional markers, generic arguments and explicit broad signatures. Unresolved names are not treated as proven absence.
   7–9. These tests live in a test-only module; file-level placement does not enable production code. Names/import placement are optional style changes.
7. The existing cached origin query is harmless; moving it into a branch is optional optimization.
8. member_missing_in_union is a public API, now unused inside this repository. Removing it would be a separate API cleanup; it no longer decides these diagnostics.

The additional probe passed for self-cycle, chained union aliases, unknown nominal alternatives and nil arms, with repeated queries agreeing; it was restored cleanly. Existing exact-source validation remains 24 focused and 2,702 workspace tests passed (31 ignored), plus formatting and all-target/all-feature Clippy. I sign off this head for final user review; approval remains separate.

[hgschmie]

Revisited the five recommended test additions from review 5721585190. NEW signed commit b554a5fcecc55348feab8b3f49f91657d3898171 retains five permanent regression tests instead of the earlier temporary probes:

1. Chained Outer -> Middle -> Present|Absent aliases: unguarded integer arguments are rejected and guarded arguments accepted.
2. Identity self-alias and recursive alias with a concrete alternative: queries terminate, and the unresolved cycle does not manufacture absent-member nil.
3. The same expression is evaluated cold and warm, with the exact integer|nil result required both times.
4. A Ref whose type definition is absent is tested in both union orders, with a genuinely absent member as a contrasting control.
5. Required and optional fields on a nil-containing owner union retain the exact member type, with only one nil in nullable results. NeedCheckNil is explicitly absent after a receiver guard and present for the unsafe unguarded access.

All five tests pass on the existing production implementation; this commit adds regression coverage only. All 2,707 workspace tests pass (31 ignored), along with formatting and all-target/all-feature Clippy. My earlier assessment verified behavior with disposable probes but did not provide the requested lasting coverage; this follow-up closes that gap. The PR remains in draft for review of this new commit; final USER approval stays separate.

[github-actions]

AI-generated review by OpenRouter Review.
Verify findings before applying changes. Feedback · Report to GitHub.

Code Review

The change fixes a real semantic gap: member access on a union (A|C where only A has handle) now yields T? rather than silently projecting T. The old ad-hoc member_missing_in_union check in param_type_check.rs is replaced by making the type itself nullable, which is the right layering. Overall the direction is good, tests are extensive, and the inline comments explain the non-obvious parts. Below are the issues worth addressing.

---

High priority

1. member_missing_in_union is likely dead code now

The only consumer of semantic_model.member_missing_in_union(...) was the deleted block in param_type_check.rs. If it isn't used elsewhere it should be removed, otherwise you will get a dead_code warning (or silently leave a stale helper). Worth grepping.

2. Behavioral change in a very central function (member_info_impl)

member_info_impl is used by completion, hover, signature help, flow, and diagnostics. Injecting Nil into the returned info.typ for all union member lookups is a broad change. Only two pre-existing tests were updated (union_member_argument_test.rs), but consumers that previously relied on the non-null member type may subtly change behavior. Consider:

• Auditing callers of member_type / member_info for code paths that assume non-null.
• Adding a regression test for signature help and completion on event.player_index for a union receiver.

3. Alias branch in member_info_impl swallows failures

if alias_id...is_some_and(|def| def.kind == TypeDefKind::Alias) {
    return with_expanded_alias(model, prefix_type, |expanded| {
        member_info(model, expanded, key)
    })
    .flatten();
}

Once an alias head is detected, the function always returns, even if with_expanded_alias returns None (e.g. cycle-limits, unresolvable alias body). That means the earlier fast path (direct_member_info) and later fallbacks are bypassed for aliases. The SelfCycle test asserts None, so it is intentional; but for a truly unresolvable alias you lose the previous fallback. Please confirm this is desired and add a test for a missing (undeclared) alias body.

4. Cycle safety for self-referential generic aliases is untested

---@alias Gen<T> Gen<T> recursing through Generic(get_base_type_id()) is not covered by self_alias_member_projection_terminates_without_false_absence (which only tests nominal self-aliases). If with_expanded_alias keys on LuaTypeDeclId, this could stack overflow. Worth a test.

5. cache.is_complete(revision) is fragile

let revision = model.cache.borrow().revision();
let component_info = member_info(model, component, key);
optional || (component_info.is_none() && model.cache.borrow().is_complete(revision))

This is the "cycle-limited query is unresolved, not absent" heuristic. Two concerns:

• If is_complete returns false for any reason other than a cycle (e.g. pending derivation elsewhere in the project), a genuinely missing alternative silently stops contributing Nil → under-detection.
• The revision is captured before member_info, but member_info may push revisions; behavior depends on is_complete semantics without seeing it here. This deserves a doc-comment on is_complete explaining the invariant, or a more explicit "hit-cycle" flag on the return value.

---

Medium priority

6. resolve_member is now bypassed for any union prefix

if union_prefix.is_none() && let Some(resolved) = model.resolve_member(index_expr) { ... }

If union_prefix.is_some() but member_type(prefix, &key) returns None, we skip resolve_member and fall out of this block entirely. Cases to consider:

• Numeric indexing into a union with an array arm, e.g. ---@param x (integer[]|nil); x[1]. If array::last_element_type doesn't unwrap the union, the union branch runs, member_type on key 1 may or may not resolve for the array component, and resolve_member is no longer consulted.
• Aliases that expose a "raw" member without going through member_type.

Please add a test for (integer[]|nil)[1] and for (Foo|Bar)[key] where key is a non-literal.

7. expand_alias_head on a direct union

union_prefix is only Some when expand_alias_head returns Some(Union). If expand_alias_head returns None for a non-alias direct union, then the direct-union case in test_nullable_union_projection_argument_contracts ("ProjectionPresent|ProjectionAbsent") would fall through to the old path, and the new test would fail for the ("integer", "integer", false) case. The tests presumably pass, so expand_alias_head must return Some(Union) for direct unions — but this dependency is implicit and worth a one-line comment.

8. Performance on hot paths

• The alias branch does a type_def_of lookup on every Ref/Def/Generic member query.
• The union branch calls member_info(model, component, key) for each non-nil, resolvable alternative on every lookup — O(N) extra work per union access. For large generated unions this can compound with caching misses.
• build_hover.rs: origin_typ is now computed unconditionally (let origin_typ = semantic_type_of(&origin_model, &origin);) for every member hover, even if unused. Prefer computing lazily in the else branch.

9. RefCell borrow inner-loop

model.cache.borrow() is invoked inside the any() closure. It currently works because component_info is evaluated before the final is_complete check, but this is fragile — if file_facts_of or member_info is ever changed to hold a cache borrow across the call, this becomes a runtime BorrowMutError. Consider hoisting the revision once outside the closure (per iteration vs per member query) and forbidding cache.borrow() inside the callback.

10. Optional detection relies on SemanticId::Member shape

let Some(member_id @ SemanticId::Member(key)) = info.id.as_ref() else {
    return false;
};

If a component exposes its handle through a non-Member SemanticId (e.g. a synthesized/inherited member), the optional marker is silently ignored. Add an assertion or comment clarifying that inherited members are handled elsewhere.

---

Minor

11. Test file organization

param_type_check_test.rs has existing tests inside mod test { ... }, but the four new tests (test_nullable_union_projection_*) are appended at file scope. Functionally fine but inconsistent. Prefer keeping all tests inside mod test for readability.

12. Fragile hover assertion

test_copied_union_member_hover_keeps_projected_nil asserts the exact rendered string "(field) player_index: integer?\n". Consider matching with contains("player_index: integer?") or contains("integer?"), to avoid coupling to hover-format details.

13. Redundant find on member_infos_with_key

if let Some(mut info) = member_infos_with_key(model, prefix_type, key)
    .into_iter()
    .find(|info| &info.key == key)

_with_key should already restrict to the key, so .find(...) is redundant. Harmless, but worth cleaning up if member_infos_with_key guarantees filtering.

14. Unknown interaction with Nil injection

info.typ = LuaType::from_vec(vec![info.typ, LuaType::Nil]);

If info.typ is Unknown, the result may still be Unknown, which is the desired conservative behavior; but if from_vec normalizes differently, this could produce an odd union. Add a ("Unknown", ...) case to test_nullable_union_projection_keeps_optional_generic_and_unresolved_alternatives.

15. Diagnostics message change

The removal of the explicit member_missing_in_union check means the diagnostic no longer says "nil" but uint32?. This is more accurate given the new type, but it changes user-facing messages. If there are downstream docs/snapshots not updated, they should be checked.

---

Summary

The core direction is correct and well-tested. The main things to resolve before merging:

1. Confirm/remove the now-dead member_missing_in_union.
2. Double-check is_complete semantics and the RefCell borrow pattern inside the union loop (#5, #9).
3. Verify resolve_member bypass doesn't regress array/union indexing (#6), including a test.
4. Audit consumers of member_info since Nil is now injected globally (#2).
5. Add cycle tests for generic self-aliases (#4).

[hgschmie]

Assessed all fifteen points in review 5724838879. NEW signed commit da095166a79d94173f8c715a22655cc95c4070e4 retains additional consumer/alias regressions and fixes the demonstrated unresolved-alias absence defect.

1. member_missing_in_union is a public API. It is no longer used internally, but removing an exported API is separate cleanup; all-target Clippy confirms no dead-code warning.
2. Added actual completion and signature-help regressions for union receivers. Available fields and callable signatures survive projection. Existing copied-hover, imported callable, optional/generic signature and diagnostic controls also pass.
3. Confirmed an alias-specific gap: an alias to an unloaded definition was treated as proven member absence when combined with a concrete class. The union scan now checks the expanded nominal head, while retaining the original member-query path and recursion guard. The new regression failed before this correction and now retains the concrete member type without manufacturing nil.
4. Added identity generic self-alias termination and cold/warm checks. Existing named Leaf recursive alternatives remain covered. An additional recursive-generic union with an inline object arm fails even with the upstream-base member implementation; its failing test is retained with an explicit ignore reason as a separate pre-existing limitation, not claimed fixed or counted as passing here.
   5,9. Documented the completeness invariant: only incomplete work advances the revision, not successful cache insertion. The component snapshot now also covers alias-head resolution. Cache borrows end at their statements before recursive work; none is held across member_info or file-fact queries. Hoisting one revision outside all components would conflate unrelated alternatives.
5. Added guarded array|nil numeric-index and nonliteral union-key expression controls. Array entries remain integer|nil; guarding the receiver avoids a spurious NeedCheckNil. The existing dynamic-member Unknown limitation remains explicit.
6. Direct unions already pass through expand_alias_head unchanged; direct, named, generic and chained union controls pass.
7. Moved hover-origin type computation into the branch that needs it. The absence scan still short-circuits with any(); per-alternative queries are necessary to distinguish real absence from unresolved types.
8. Added a source comment distinguishing inherited declaration optional markers from nullability already carried by synthetic member types; existing optional callable controls pass.
   11–13. The diagnostic test file is test-only through its parent module. Exact hover rendering is an intentional output contract, and the keyed-result check is a cheap defensive check. These are optional organization/style changes, not failing behavior.
9. Added an Unknown-field union control; its conservative projected union remains intact.
10. Existing diagnostic and hover assertions cover the intentional nullable surface-type message. No separate message policy is introduced.

All 14 focused projection/consumer tests pass (one explicitly ignored), all 2,712 workspace tests pass (32 ignored, including the newly recorded pre-existing limitation), and formatting plus all-target/all-feature Clippy pass. These receipts bind the exact source. The PR remains draft pending current-head review; final USER approval remains separate.

[github-actions]

AI-generated review by OpenRouter Review.
Verify findings before applying changes. Feedback · Report to GitHub.

Review Summary

The change replaces a special‑cased member_missing_in_union check in param_type_check.rs with a more principled fix upstream: union member projections (event.field where only some arms declare field) now evaluate to T | nil, and the normal argument check reports the mismatch. This is cleaner and also fixes the hover/completion/signature-help paths. Tests are extensive and clearly structured. A few concerns below.

Potential issues / suggestions

1. member.rs – redundant work and per-iteration cache reads
   The new union branch iterates every non‑nil component and calls member_info(model, component, key) even though member_infos_with_key already computed per‑component member info for the same key. This duplicates the work (and recursion), which can be noticeable for wide/deep unions. Also model.cache.borrow().revision() is re‑read on every iteration.

   • Hoist the revision snapshot outside the loop.
   • If feasible, reuse the per‑component results from member_infos_with_key instead of calling member_info again.

2. member.rs – optional detection only covers SemanticId::Member
member_by_id(...).is_nullable only works when info.id is a real SemanticId::Member. For synthetic members (intersection/generic instantiation, etc.) this returns false. The comment notes this ("Synthetic members carry nullability directly in their projected type"), but there is no explicit test for an intersection arm that is optional. Consider either extending detection or adding a dedicated test/comment that intersection components are intentionally handled by the nullability already in typ.

3. flow.rs – resolve_member is now skipped for any union prefix
   The resolve_member path is gated by union_prefix.is_none(). If a union prefix is detected but member_type returns None (e.g., member missing on all arms, or all arms unresolved), resolve_member is no longer attempted and execution falls through to the general fallback. Please confirm this is desired (e.g., for method resolution on union-typed receivers) and that the fallback still produces the expected diagnostic rather than silently yielding Unknown/nil.

4. hover/build_hover.rs – heuristic for projected_member is fragile

   • It only applies when decl is SemanticId::Decl(_), so locals and params get projected rendering, but globals/other decl kinds won't.
   • Function/DocFunction/Signature are explicitly excluded, so an optional callable member (e.g., round? fun(): integer) hovered via a union will not show the projected ?. This may be a display regression worth a comment/test.
   • It depends on info.typ already holding the projected value; if a decl re‑annotates its type, the projected display could be inconsistent. Consider resolving the projected type explicitly from the member origin rather than relying on info.typ.

5. #[ignore] test – please add a tracking issue reference and, if possible, a rough plan for the recursive‑generic + inline‑object arm limitation so it isn’t forgotten.

6. type_eval/tests.rs – the updated expectations (String | Nil for array arms) confirm that member_type and member_infos intentionally diverge (nil projection applied only in member_type). Worth a brief doc comment on member_type explaining that union projections may insert nil, since callers may otherwise be surprised.

Minor

• The is_complete doc comment is a helpful clarification; no action needed.
• The removed special case in param_type_check.rs also previously fired even when the parameter accepted nil (e.g., integer?); the new behavior correctly only reports real mismatches. Good improvement, and the test_nullable_union_projection_argument_contracts matrix covers it.

Overall the direction is sound and well tested; the main things to address are the redundant recomputation in the union loop and the robustness/documentation of the hover heuristic and resolve_member gating.

[hgschmie]

Assessed all six points in review 5724903773. NEW signed commit b376b592b7f33b5a7efcaa0c074a73534873efdd adds the requested synthetic-intersection regression and documents the projection API.

1. Retained the per-component completeness snapshot. A failed alias/cycle lookup in one alternative must not make a later, genuinely absent alternative look unresolved. Hoisting the snapshot across the loop would lose that distinction. member_infos_with_key collects and deduplicates declarations; it does not return each alternative's absence/completeness or the projected intersection member. Reusing that flattened vector as an absence proof would be incorrect. A richer collector result could remove repeated traversal, but that is a separate optimization requiring preservation of those per-arm facts; no performance improvement is claimed here. The scan still short-circuits at the first proven absent/optional arm.
2. Added a permanent synthetic optional intersection test, in both union orders and with cold/warm member queries. Its nil comes from the projected synthetic type, without a declaration ID. Existing declared-optional and generic callable cases remain covered.
3. Union lookup intentionally avoids selecting one declaration as the whole value. If no arm supplies a member, existing fallback inference remains in control; missing/unresolved members are not fabricated as nil. The retained unresolved-nominal/alias, dynamic-key, array, Unknown-field and completion/signature-help controls exercise these boundaries. This change does not claim to repair the existing dynamic-key Unknown limitation.
4. SemanticId::Decl includes globals as well as local declarations and parameters (DeclKind::Global). The projected data value comes from the existing use-site info, so following its origin for documentation does not replace an annotation or narrowing with the origin's type. Callable declarations retain the existing specialized signature renderer and declaration optional-marker handling; unifying callable-value display is a separate presentation change, not part of this data-alias fix. Existing callable/imported signature tests remain passing.
5. The ignored recursive-generic inline-object test is deliberately retained and tracked in the workspace finding F-20261031-pr34-pr38-regression-followup/generic-inline-object-preexisting.md, with its failing baseline receipt. The next investigation should compare alias generic substitution and inline-object collection against the passing named Leaf alternative, then replace the ignore only when that baseline defect is fixed. No passing claim or public issue is fabricated.
6. Added the member_type API comment explaining nil insertion for known absent/optional non-nil union arms and the distinction from declaration enumeration.

The new intersection test and the existing projection regressions pass. Formatting, all-target/all-feature Clippy and full all-feature workspace validation pass; the recursive-generic inline-object case remains explicitly ignored. The PR remains draft for current-head review; USER approval is separate.

[github-actions]

AI-generated review by OpenRouter Review.
Verify findings before applying changes. Feedback · Report to GitHub.

Code Review

Summary

The change reworks union-member projection: instead of a bespoke ParamTypeMismatch in the argument checker, it produces a properly projected member type (adding nil for absent/optional arms) in member_info_impl, wires it through flow.rs, and adapts hover/completion/signature help. The approach is reasonable and the tests are extensive, but there are several concerns.

Issues

1. Likely dead code: member_missing_in_union

param_type_check.rs removed the only call site of semantic_model.member_missing_in_union(index_expr). Unless it is used elsewhere, this now becomes dead code and should be removed (or have its remaining callers confirmed). The removal is correct behaviourally, but stray dead code should be cleaned up in the same PR.

2. Test placement / style inconsistency

In crates/emmylua_code_analysis/src/check/test/param_type_check_test.rs, the diff appends the new tests after the closing } of mod test:

}
#[test]
fn test_nullable_union_projection_argument_contracts() { ... }

The rest of the file uses #[gtest] functions inside mod test { ... }. The new tests live at file scope, use #[test], and import use crate::{...}; inside the function bodies. This is inconsistent with the surrounding code and bypasses the module's helpers. Consider moving them into mod test (or documenting why they diverge). The same applies to the new mod union_callable_projection / mod union_projection_review_regressions in member.rs which sit outside the existing mod tests.

3. Performance: linear recursive re-query per union arm

In member_info_impl:

if let LuaType::Union(union) = prefix_type
    && union.iter().any(|component| {
        ...
        let component_info = member_info(model, component, key);
        ...
    })

Every union lookup now (a) calls expand_alias_head and (b) recursively calls member_info for each arm. For deeply nested unions, aliases, or generic unions this multiplies the cost significantly, and the results don't appear to be memoized at this level. The is_complete(revision) guard prevents false negatives but doesn't prevent the repeated re-computation. Consider caching per-(component, key) results or short-circuiting once all arms have been evaluated.

4. Unknown | Nil union may cause downstream regressions

The test union_projection_unknown_field_remains_conservative asserts that UnknownField | Empty yields Unknown | Nil. Semantically Unknown already absorbs everything, so Unknown | Nil is redundant and could confuse downstream assignability checks (some checkers treat Unknown as an opaque top that behaves differently when wrapped in a union). Either normalize the projection to Unknown when the present type is Unknown/Any, or add a comment explaining why the redundant union must be preserved.

5. Hover heuristic can misfire for nullable functions / unions containing functions

In build_hover.rs:

let projected_member = matches!(decl, SemanticId::Decl(_))
    && matches!(origin, SemanticId::Member(_))
    && !matches!(
        &origin_typ,
        LuaType::Function | LuaType::DocFunction(_) | LuaType::Signature(_)
    )
    && !matches!(&info.typ, LuaType::Unknown | LuaType::Any);

• If the member is declared as fun(): X? or (fun(): X) | nil, origin_typ is a Union, so the Function guard fails and projection kicks in with nil – probably desirable, but this contradicts the intent of excluding callables from projection. Consider matching on the union's inner type or documenting the intent.
• A Ref to a named function type also bypasses the guard.

6. effective_typ_override silently ignored when projected_value is true

let effective_typ = if projected_value {
    typ.clone()
} else if let Some(override_ty) = effective_typ_override {
    override_ty

If a call-site substitution or runtime override previously carried information the projected type lacks, it is now discarded for any member reached from a data alias. This seems intended, but the interaction isn’t documented; a short comment (or narrowed override handling) would prevent future regressions.

7. is_complete reliance in the union arm check

The new logic relies on the fact that "successful cache insertions do not advance the revision" (per the new comment in cache.rs) to distinguish "absent because unresolved/recursive" from "absent because it truly doesn't exist". This is subtle and fragile: any future change that bumps the revision on successful inserts would silently disable the nil injection for every union arm. The new doc comment helps, but consider making the intent explicit at the call-site too (e.g. a named absent_is_proven(...) helper).

8. The #[ignore] test documents an unresolved case

#[ignore = "Known upstream limitation: recursive generic unions with inline object arms"]
fn recursive_generic_inline_object_arm_retains_its_member() { ... }

This is honest, but it means the fix is incomplete for recursive generic unions with inline object arms. Worth tracking (or referencing the issue) so the limitation doesn't regress silently.

9. Minor: ws.def("") used as scratch file in a test

let file = ws.def("");
let cases = [ ... ].map(|source| ws.ty(source));

Works, but creating an empty file just to obtain a model is a little opaque. A comment or a dedicated helper would make the test clearer.

Positives

• The removed special case in param_type_check.rs is a genuine simplification; unify-once-into the projected type is the right design and the updated union_member_argument_test.rs expectations (uint32? instead of nil) confirm the message is now driven by the real type.
• The alias routing at the top of member_info_impl (with_expanded_alias) correctly handles chained aliases and generics, with tests covering Outer/Middle chains and generic instantiations.
• Cold/warm cache consistency is tested (union_alias_projection_is_identical_on_cold_and_warm_expression_queries), and the nil-deduplication assertion in nil_union_arm_preserves_member_type_and_receiver_diagnostics guards against duplicated nil.
• Letting individual union arms be “unresolved” rather than assuming absence (via unresolved_nominal and the is_complete guard) avoids manufacturing false “missing member” diagnostics — a common source of false positives.

[hgschmie]

Assessed all nine points in review 5724958314. NEW signed commit e1e51483bbe619a6a660fef570b65fd387bf57f9 reuses the existing model cache for complete per-component projections and clarifies the value-rendering contracts.

1. member_missing_in_union is still a public API even though internal use was removed. Removing an exported API is separate cleanup; all-target Clippy reports no dead-code error.
2. These are ordinary Rust tests in test-only parent modules. Their grouped modules keep the targeted regression matrices independent of older gtest helpers. Moving them is optional organization, not missing execution; the focused and full receipts confirm they run.
3. Per-component queries now go through SemanticModel::member_info, whose cache publishes only complete results. This avoids repeating complete component projections on later lookups while preserving cycle/provisional handling and the early any() exit. The initial declaration enumeration and absence scan remain distinct because the flattened member list cannot prove each arm's absence. Existing cold/warm, alias, recursive, unresolved and optional-intersection tests pass.
4. Unknown|nil is produced by the shared LuaType::from_vec constructor. A comment now explains that projection preserves explicit absence and leaves normalization to that common API; it does not impose a new Unknown absorption policy in one consumer.
   5–6. Added comments distinguishing plain callable signature rendering from nullable/aliased callable value contracts, and explaining why an origin declaration's override cannot replace an already-computed use-site projection. The existing copied-hover and callable consumer tests remain passing.
5. The per-component snapshot includes alias expansion; incomplete work in a different arm must not invalidate proof of absence in this one. The cache API and call-site comments document this invariant. No extra helper is needed to change that behavior.
6. The ignored recursive-generic inline-object case remains explicitly tracked with a baseline receipt and next-investigation plan in the workspace finding. This PR does not claim to fix it.
7. Added a short explanation for the empty caller file used by anonymous structural-type tests.

All projection regressions and all 2,713 all-feature workspace tests pass (32 ignored, including the explicitly tracked pre-existing limitation). Formatting and all-target/all-feature Clippy pass. The PR remains draft pending current-head review; USER approval remains separate.

[github-actions]

AI-generated review by OpenRouter Review.
Verify findings before applying changes. Feedback · Report to GitHub.

Code Review

Summary of Changes

This PR reworks how union member projection is handled. Instead of a special-case check in param_type_check.rs that reported ParamTypeMismatch for missing union members, the type system now correctly projects X? (or X | nil) when any non-nil union arm is absent or optional. It touches the member projection layer, expression flow analysis, and hover rendering.

Overall the approach is sound and the test surface is extensive. A few concerns below, mostly around subtle correctness and maintainability.

---

Issues & Concerns

1. Over-reliance on cache revision semantics in member.rs (correctness / fragility)

let revision = model.cache.borrow().revision();
// ... 
optional || (component_info.is_none() && model.cache.borrow().is_complete(revision))

The projection correctness for "absent arm → add nil" hinges on the invariant documented in cache.rs:

Recursion and budget exhaustion advance the revision; successful cache insertions do not.

This is subtle and only documented in a one-line comment. If the invariant drifts (e.g., someone adds a cache path that records an incomplete result without bumping the revision), this logic silently over-reports absence. Consider:

• Adding a debug-only assertion test that cycle-limited member lookups of a union do not add nil (partially covered by recursive_generic_inline_object_arm_retains_its_member but #[ignore]d).
• Documenting the invariant at the member-projection site as well, not only in cache.rs.

2. expand_alias_head semantics for direct (non-alias) unions in flow.rs

matches!(
    super::type_eval::expand_alias_head(model, &ty),
    Some(LuaType::Union(_))
)

It is unclear from this diff whether expand_alias_head returns Some(Union) when ty is already a LuaType::Union (not an alias). If it returns None for a plain union, then a direct union receiver will not take this path, and the new nil logic must be reachable through resolve_member. The test_nullable_union_projection_argument_contracts cases (e.g. "ProjectionPresent|ProjectionAbsent") imply the direct-union case works, but it is worth documenting this explicitly — a future change to expand_alias_head could silently regress direct-union nil projection.

3. union_prefix.is_some() short-circuits resolve_member

if union_prefix.is_none()
    && let Some(resolved) = model.resolve_member(index_expr)

If union_prefix.is_some() but the member_type lookup yields None (e.g., no key, or no component exposes it), the code falls through and skips resolve_member. Verify that member_type's resolution paths fully cover what resolve_member used to handle for union prefixes (inherited members, metatables, native bindings, anonymous-table fallbacks). If member_type returns None on an otherwise resolvable loc, the expression will end up Unknown rather than the expected type. A brief comment stating "member_type is authoritative for union prefixes" would help future readers.

4. Alias expansion now runs on every member lookup

if alias_id
    .and_then(|id| model.type_def_of(&id))
    .is_some_and(|def| def.kind == TypeDefKind::Alias)
{
    return with_expanded_alias(model, ...)
}

This runs on member_info_impl for any Ref/Def/Generic whose def is an alias. For non-union aliases this is new behavior. The test_union_projection_array_and_dynamic_indexes_preserve_existing_paths and union_callable_projection_keeps_imported_closure_identity cases appear to cover it, but consider:

• A benchmark/measurement on the alias-targeting-member path (e.g. require-returned modules aliased to tables), since this expands for every member lookup of a module.
• Confirming there is no infinite recursion path when with_expanded_alias returns None (the .flatten() path). The self_alias_member_projection_terminates_without_false_absence test exercises this, but only for a small handful of shapes.

5. Hover projected_member heuristic is brittle

let projected_member = matches!(decl, SemanticId::Decl(_))
    && matches!(origin, SemanticId::Member(_))
    && !matches!(&origin_typ,
        LuaType::Function | LuaType::DocFunction(_) | LuaType::Signature(_))
    && !matches!(&info.typ, LuaType::Unknown | LuaType::Any);

• An alias-to-function (---@alias F fun(): integer) resolves origin_typ to a LuaType::Ref, so projected_member will be true and the hover will fall through to the data renderer (no signature rendering). If aliases of function types are expected to render as signatures, this is a regression.
• A union of function signatures also is not LuaType::Function, so it will use the value contract — which is likely the desired behavior for the nullability fix, but means it will no longer render as a callable signature. Confirm this is intentional, and add a test for Present|Absent where handle: fun(): integer vs. handle: fun(): string union.

6. Tests: a few could assert stronger invariants

• union_callable_projection_keeps_typed_alternatives_and_absence uses .any(|f| f.get_ret() == &expected). This passes if there are duplicate callables or extra unexpected alternatives. Asserting a set (e.g. via BTreeSet of return types) would be more robust.
• test_nullable_union_projection_retains_integer_and_nil_in_type asserts the exact order [Integer, Nil]; that order is an implementation detail of LuaType::from_vec. If ordering changes, this fails for a non-behavioral reason. Consider assert_eq!(as_set(...), ...) or add a comment noting the ordering is asserted on purpose.
• union_projection_array_and_dynamic_indexes_preserve_existing_paths also uses for _ in 0..2 rather than named cold/warm phases (as done in other tests), which reads less clearly — minor style.

7. Formatting / small nits

• Missing blank line between the last test of the previous module and the new #[test] fn test_nullable_union_projection_argument_contracts (line } immediately followed by #[test]). Same for the transition into mod union_projection_review_regressions. cargo fmt may want a blank line here.
• Uses full crate path crate::semantic_model::type_eval::expand_alias_head inline rather than importing. Minor style.
• SemanticLocalCache::is_complete doc comment says "No incomplete dependency has been observed since this snapshot." — "this snapshot" is ambiguous; it refers to the revision argument. Suggest: "Returns true if no incomplete result was recorded since revision was captured."

8. Removed member_missing_in_union check: behavior change worth confirming

The old special case pushed ParamTypeMismatch regardless of the target param type:

message: mismatch_message(semantic_model, &LuaType::Nil, &param_ty, name),

The new behavior routes through normal assignability. That means ---@param x any (or ---@param x integer?) no longer errors for a nil-possessing union member read — a real semantic improvement, and the tests now assert ("any", "integer", true) and ("integer?", "integer", true) accordingly. Worth calling out in the PR description, since this is a user-visible change beyond the original message text tweak in union_member_argument_test.rs. Also:

• The two union_member_argument_test.rs changes assert the new message contains `uint32?`. That is derivable from the target type name, so the assertion is really "the message now names the declared param type rather than nil". If mismatch_message's format changes, these strings will break — consider asserting on a more stable piece (e.g. that the message contains both the target type name and does not contain nil).

---

Minor / Opinion

• The nil-projection block inside member_infos_with_key is nested five levels deep (if let Some(...) { if let Union = ... && any(...) { ... } }). Consider extracting a helper like union_has_proven_absent_arm(model, union, key) -> bool for readability.
• build_member_hover now takes a fourth boolean-ish parameter, plus multiple internal !projected_value conditionals. If this grows further, consider a small HoverMode enum (ProjectedValue, Signature, Default) rather than a raw bool threaded through.
• The #[ignore]d test recursive_generic_inline_object_arm_retains_its_member is useful context, but if the underlying limitation is not scheduled, consider linking to an issue number in the ignore reason.

---

Verdict

The direction is correct and the coverage is good. The main risks are (a) the subtle cache-revision completeness check that determines nil projection, and (b) the assumption that member_type subsumes resolve_member for union prefixes. I recommend:

1. Adding a brief comment (or test) pinning down the expand_alias_head(Some(Union)) behavior for plain unions.
2. Documenting the "skip resolve_member for union prefix" decision in flow.rs.
3. Tightening the union-callable test to assert exact alternative count / set.
4. Confirming the alias-to-function hover rendering behavior.

[hgschmie]

Developer assessment of review 5724985992 on exact head e1e51483bbe619a6a660fef570b65fd387bf57f9: all eight numbered points and the minor suggestions assessed; no demonstrated source blocker remains.

1. Active, non-ignored tests already cover self-alias and identity-generic cycles without false absence, unresolved nominal types and unresolved alias bodies. The ignored inline-object recursive-generic case is a different, baseline-proven limitation. The call site explicitly states that cycle-limited work is unresolved, and the cache API documents the incomplete-event invariant. The new per-component cache publishes only complete results.
2. expand_alias_head returns ty.clone() immediately when needs_reduction is false; a direct Union takes that path. Direct-union argument tests exercise it, alongside named/chained/generic union cases. This is established behavior, not an assumption that plain unions are aliases.
3. member_type is intentionally authoritative for the whole union projection. Selecting one resolve_member declaration would discard other alternatives and their absence. Keyed collection and member lookup retain inheritance and table/metatable handling; no-result cases continue into the existing general fallback. Unresolved, array, dynamic-key, callable, completion and signature-help controls pass. The existing dynamic-key Unknown limitation is explicitly retained, not described as repaired.
4. Alias-head reduction has its own complete-result cache, active-alias guard and shared evaluation budget; the component member cache now reuses complete projections as well. with_expanded_alias returning None terminates the branch through flatten rather than retrying it. The cycle and cold/warm regressions pass. No module-path benchmark or wall-clock improvement is claimed.
5. Plain callable declarations retain signature rendering. Nullable/aliased callable unions intentionally retain their value contract, including absent alternatives. A Ref alias already remains a Ref through the existing origin-type path; setting projected_value does not itself turn a previously concrete function into a Ref. The typed-alternative/absence, imported closure identity, optional/generic callable and signature-help tests remain passing. The source comments document the intentional distinction.
6. The callable test checks preservation of each required return alternative rather than imposing a unique declaration count: equivalent signatures can come from distinct declarations. Exact projected LuaType tests and the nil-dedup control cover the value contract. The Integer/Nil assertion is compared through the shared normalized union constructor. Naming a two-iteration loop's phases is optional style.
7. Formatting and all-target Clippy pass. The fully qualified helper, test-module spacing, and possible helper/enum extraction do not change the contract; they are deferred cleanup. The revision parameter and nearby snapshot make the existing completeness doc unambiguous.
8. Confirmed the intentional behavior improvement: integer? and any parameters accept the nullable projection; integer parameters report the real integer|nil mismatch. Updated the PR description to state this. The uint32? diagnostic tests assert the projected source type, not a replacement of the target type by its name; their message check is intentional user-visible coverage.

The ignored recursive-generic inline-object case remains tracked with a baseline receipt and an investigation plan in the workspace finding; no public issue or scheduled fix is invented.

Validation: 13 focused projection tests pass (one explicitly ignored), 2,713 all-feature workspace tests pass (32 ignored), formatting and all-target/all-feature Clippy pass. Developer signoff is complete for this head. Ready means awaiting separate USER approval; the active review history and maintained contribution remain unchanged until approval.

[hgschmie]

approved