fix(diagnostics): respect narrowed owners in union member arguments

[hgschmie]

After narrowing a union to a member that has a field, passing that field to a function can still report expected integer? but found nil, even though hover shows the narrowed type:

---@class OneGoal
---@field start_index? integer
---@class AllGoals
---@field goals integer[]

---@param values string[]
---@param result OneGoal|AllGoals
local function remove_value(values, result)
    ---@cast result OneGoal
    ---@cast result.start_index integer
    table.remove(values, result.start_index) -- false param-type-mismatch
end

The argument check for members missing from union components uses prefix_type_for_member_resolution, which deliberately reads the original declaration type. It therefore keeps considering AllGoals after the cast and reports a synthetic nil because that type lacks start_index.

Use the flow-sensitive owner type at the argument's position for this diagnostic check. The underlying non-flow member resolver remains unchanged to preserve its recursion safeguards. Casts, guards and assignments are respected, while actual missing-member and argument-type errors remain diagnosed. This change is independent of the named-expression cast fix in #1285.

Eleven regression tests cover owner/member casts before and after intervening statements, guards, assignments, narrowed unions, nested and recursive owners, unnarrowed/alternate paths, incompatible field types, a 17-component union and a widening cast. Each uses a fresh workspace; shared classes are defined once, without class merging. The helper checks diagnostic codes explicitly, and assertions use hard failures where later checks depend on the result.

The initial fix passed all 2,391 workspace tests with all features, workspace Clippy with warnings denied, and rustfmt. Five regressions were confirmed failing before the fix. The review follow-up changes only comments and tests; its expanded 11-test suite, Clippy and rustfmt pass. A direct LSP test with a copy of the affected Factorio mod also confirms that the two reported table.remove warnings disappear without changing the Lua code.

The review's performance/recursion concern is scoped to argument checking: this helper has one caller, ParamTypeChecker, and is not called by member resolution. type_of_expr_at_impl caches by file, syntax and offset; ordinary inference guards in-progress dependencies. The 11-test suite, including a 16-level recursive-class owner chain, completes in 0.25 seconds locally. This is bounded regression coverage, not a general performance benchmark.

A shortcut for declared non-union owners would be incorrect: ---@cast result OneGoal|AllGoals can widen an originally OneGoal parameter into a union with a missing member. The proposed shortcut was tested and suppresses the required diagnostic; the new regression catches that failure. The flow-sensitive check is therefore retained for those owners as well.

[github-actions]

Code Review

Overview

The change adds a test module for union member argument diagnostics and modifies access.rs to use flow-sensitive type resolution (type_of_expr_at) instead of prefix_type_for_member_resolution when checking union member arguments.

Issues & Observations

1. Potential performance regression (access.rs:526)
type_of_expr_at is typically more expensive than prefix_type_for_member_resolution since it performs full flow analysis. If this function is called frequently during member resolution, this could introduce a noticeable slowdown. Consider whether the flow-sensitive lookup can be cached or whether the fast path can be preserved when the prefix type is already a non-union.

2. Comment vs. code mismatch (access.rs:526-527)
The comment says:

"Keep the non-flow helper for member resolution itself to avoid recursive lookup."

But the code no longer calls prefix_type_for_member_resolution at all in this branch. If the intent is that this function is not member resolution but argument checking, the comment is misleading — it references a helper that isn't used here. Either:

• Clarify that this function is argument-checking (not member resolution), or
• Remove the second sentence if it's stale.

3. Recursion risk not fully addressed
The comment mentions avoiding recursive lookup, but switching to type_of_expr_at may itself trigger member resolution internally (e.g., resolving result.start_index requires resolving result). Verify there's no re-entrancy into this same code path for the same expression, which could cause infinite recursion or exponential blowup on deeply nested unions.

4. Test coverage gaps

• No test for deeply nested unions (e.g., A|B|C|D where only one member has the field) to confirm the "remaining components" logic scales.
• No test for cyclic/self-referential class fields that could expose recursion issues.
• narrowed_owner_union_checks_only_remaining_components uses table.remove({}, ...) — the empty table literal may mask issues; consider using a typed string[] for realism.
• No negative test verifying that a cast to an unrelated type still produces a diagnostic.

5. Test helper robustness (union_member_argument_test.rs:24-33)

let file = ws.def(&format!("{TYPES}\n{source}"));

TYPES is prepended to every source, so tests that redefine OneGoal/AllGoals (like narrowed_owner_union_checks_only_remaining_components adding AnotherGoal) rely on class merging semantics. This is fragile — if class redefinition behavior changes, these tests silently break. Consider making TYPES opt-in per test or documenting the dependency.

6. expect_that!(diagnostics, is_empty()) vs assert_that!
Mixing expect_that! and assert_that! across tests is inconsistent. expect_that! is typically for soft assertions in googletest; for hard assertions prefer assert_that! uniformly. Minor style issue.

7. Missing assertion on diagnostic code
Tests assert on message substrings ("nil", "string") but not on DiagnosticCode::ParamTypeMismatch. Since enable_only is set, this is mostly fine, but asserting the code explicitly would make tests more robust against message wording changes.

Recommendations

1. Benchmark or profile the type_of_expr_at call in this hot path; add a fast-path check for non-union prefix types before invoking flow analysis.
2. Fix or remove the stale comment about prefix_type_for_member_resolution.
3. Add a recursion-depth test (nested unions / self-referential classes).
4. Assert on diagnostic.code in addition to message substrings.
5. Standardize on assert_that! for hard assertions.
6. Consider isolating TYPES per test to avoid implicit class-merge dependencies.

[hgschmie]

rebased to current main

[CppCXY]

use VirtualWorkspace::has_no_diagnostic for test diagnostic

[hgschmie]

rebased to current main