[lewis6991]

After the rewrite, strict.arrayIndex = true reports need-check-nil for a guarded array element in a class field. This restores narrowing so the following access is accepted:

if t.items[1] then
    print(t.items[1]:sub(1))
end

Match the root declaration and index keys, require a guard on each incoming path, and reject intervening direct writes or guards from another function. Apply narrowing after existing inference to preserve assignment types. Unchecked reads still report need-check-nil.

Restore the expected behavior for #600. Add a TODO to replace the separate guard walk with shared access-path queries.

[github-actions]

Code Review

Overall the change is well-structured and the test coverage is thorough. A few issues worth addressing:

1. index_access_path — LuaMemberKey::None used as a wildcard sentinel

In index_access_path, dynamic keys (Expr(_), non-integer numeric literals) are mapped to LuaMemberKey::None. This overloads None to mean "unknown/dynamic key". The write-matching logic then treats a.is_none() as a wildcard:

.all(|(a, b)| a.is_none() || a == b)

This is subtle and easy to get wrong. Consider t.items[_index] = nil where _index is dynamic — it correctly invalidates the guard. But LuaMemberKey::None is also the natural "no key" value elsewhere in the codebase; a future reader may not realize it's being used as a wildcard here. Recommend a dedicated enum (e.g. AccessKey::Static(LuaMemberKey) | AccessKey::Dynamic) or at least a comment on the LuaMemberKey::None arms explaining the wildcard semantics.

2. guarded_index_type — ? on flow_assign_stat aborts the whole walk

FlowNodeKind::Assignment(ptr) if !ptr.get_syntax_id().get_range().contains(offset) => {
    let assign = flow_assign_stat(model, &tree, current)?;

If flow_assign_stat returns None for one assignment node, the entire guard analysis bails out and returns None (no narrowing). That's conservative/safe, but it means a single unparseable assignment anywhere on the path silently disables narrowing for the whole expression. Worth a comment noting this is intentional, or handling the None case by treating it as a potential write (return None explicitly rather than via ? for clarity).

3. narrowing state is order-dependent across the DFS

let mut narrowing = Narrowing::Truthy;
...
if matches!(guard, Narrowing::NotNil) {
    narrowing = guard;
}

The narrowing variable is mutated while walking a stack-based DFS over multiple antecedents. Because pending is a stack and paths are explored in LIFO order, a NotNil guard found on one path can leak into the final result even if another path only established Truthy. The intent seems to be "if any path proves NotNil, use NotNil", but combined with the early return None on writes/unknown conditions, the semantics are hard to reason about. Consider computing narrowing per-path and combining explicitly (e.g. NotNil only if all surviving paths yield NotNil), or document the intended lattice.

4. LuaClosureId::from_node(index.syntax()) vs condition closure check

let closure = LuaClosureId::from_node(index.syntax());
...
if LuaClosureId::from_node(cond.syntax()) != closure {
    return None;
}

This correctly rejects guards established in an outer/inner closure. But note the test case:

"if t.items[1] then local function later() print(t.items[1]:sub(1)) end; t.items = {}; later() end"

expects NeedCheckNil. The closure check handles the later case, but the t.items = {} write happens after the closure is defined and before it's called — the write-invalidation logic must catch this. It appears to (the write is on the path), but it's worth confirming the write check runs for assignments that occur after the closure definition but before the call site, since the closure body is analyzed at its own offset. The test passing suggests it works, but the interaction is non-obvious.

5. Test helper check_class_array_guard — format! with {element_type} inside a raw string

let file_id = ws.def(&format!(
    r#"
    ---@class Container
    ---@field items {element_type}[]
    ...

{element_type} is interpolated, but the surrounding { and } in local _Container = {{}} and read({{ items = {{ 'hello' }} }}, ...) are escaped as {{/}}. This works, but the mix of escaped braces and interpolation makes the template hard to read. Consider building the type string separately or using a named placeholder to reduce brace-escaping noise.

6. test_class_array_element_guard_writes — 1.0 and 1e0 cases

"if t.items[1] then t.items[1.0] = nil; print(t.items[1]:sub(1)) end",
"if t.items[1] then t.items[1e0] = nil; print(t.items[1]:sub(1)) end",

These rely on index_access_path mapping non-Int numeric literals to LuaMemberKey::None (wildcard), which then matches the 1 key. Good that this is tested, but it's exactly the fragile behavior flagged in point 1 — the test documents the wildcard fallback rather than a precise key match. If 1.0 should be canonicalized to integer 1, that would be more correct than treating it as a wildcard (which would also invalidate guards on t.items[2]).

7. Minor: apply_expr_casts called twice in the fallback path

let ty = model.type_of_expr(expr_syntax);
let ty = guarded_index_type(model, index_expr, offset, &ty).unwrap_or(ty);
apply_expr_casts(model, expr_syntax, ty)

This is fine, but note the earlier branch:

if let Some(ty) = guarded_index_type(model, index_expr, offset, &member_ty) {
    return apply_expr_casts(model, expr_syntax, ty);
}

guarded_index_type is now called in two places with different ty inputs (member_ty vs model.type_of_expr). If both can be reached for the same expression, the ordering matters. Worth a brief comment on why the member path takes precedence.

8. Narrowing import / definition not shown

Narrowing::Truthy / Narrowing::NotNil and apply_narrowing are used but not defined in the diff. Assuming they exist elsewhere — just flagging that the review can't verify their semantics (e.g. whether Truthy on a string|nil yields string).

---

Summary: The logic is sound and the tests are excellent. Main concerns are (a) the overloaded LuaMemberKey::None wildcard semantics, (b) the order-dependent narrowing accumulation across DFS paths, and (c) the ? early-abort on flow_assign_stat. Addressing these with clearer types/comments would improve maintainability.