[mareklangiewicz]

Summary

A flow state memoized for one read loses the guards of a later read:

---@param w Res?
local function f(w)
  if w and w.ok then return end
  if not w then return take(w) end   -- this read fills the memo
  cool(w)                            -- false: expected `Res` but found `nil`
end

trace_decl keys its memo without the incoming path, on the basis that every route to a node crosses the same guards. That holds within one query, but the memo outlives the query. The memo now holds the state for an empty incoming path, and a query arriving with guards narrows it:

 if tree.is_loop_reachable(flow_id) {
     return trace_decl_uncached(...);
 }
+if !path.narrowings.is_empty() && path.casts.is_empty() && !has_correlated_narrowing {
+    let state = trace_decl(..., &mut PathState::default())?;
+    return Some(finalize(model, state, path));
+}
 let key = decl_trace_key(model, decl, flow_id, purpose, mode);

Queries with casts or correlated narrowings keep today's behavior. Sending them down the uncached walk instead times out test_deep_branch_casts_merge_surviving_overloads.

Evidence

• Before (plain main de89376): test_and_narrows_after_merge_of_earlier_guard fails.
• After: it passes. Full emmylua_code_analysis suite: 1703 passed. clippy reports nothing new.
• Untested conditions: no test needs the casts and correlated conditions. Removing both still passes the suite, but it adds one need-check-nil on a real codebase, so I kept them.

Merge Danger

Door: two-way

Blast Radius: narrowing

emmylua_check from main and from this branch on three real Lua codebases:

Diagnostics on main	Removed	Added
~4.9k	20	0
~1.2k	10	1
~9.1k	99	31

The third codebase's own code loses 2 and gains 0; the rest of that row is vendored code. The 1 added in the second looks false: a string param reads string? after if dir ~= memo_dir or .... I haven't reproduced it in a small test.

This is one of two fixes #1359 needs: see that PR.

---

Written with help from Claude Code.

🤖 Generated with Claude Code

https://claude.ai/code/session_018NxUUQmc3YFNEXrUcrVPxV

[github-actions]

Code Review

Summary

The change fixes a subtle narrowing bug: when a flow node's state is memoized for an empty incoming path, guards collected by the current query must be reapplied on top of the cached state. The fix adds a fast path that recomputes with an empty PathState and then finalizes with the caller's path. The test and doc updates are consistent with the new semantics.

Issues & Recommendations

1. Potential infinite recursion / cache bypass (High)

In the new branch, trace_decl is called recursively with &mut PathState::default(). Since path.narrowings is now empty, the new branch won't re-trigger, so this specific recursion is bounded. However:

• The recursive call passes the same visited set. If flow_id was already inserted into visited by the caller before this point, the recursive call may return None prematurely (or behave differently than the cached path). Please confirm visited is not mutated before this branch, or that the recursive call is safe with the current visited contents.
• The recursive call bypasses the cache lookup entirely (it goes through the same function, hits the empty-path branch, and then does the cache lookup). This is fine, but it means the "fast path" is actually a full re-entry. Consider extracting a helper like trace_decl_with_empty_path to make the intent explicit and avoid re-running the loop-reachability check.

2. finalize semantics with empty-path state (Medium)

The comment says guards "distribute over branch merges," but finalize(model, state, path) applies the path's narrowings to the result type. This is only correct if:

• The cached state was computed for the same flow_id and purpose/mode (it is, via decl_trace_key), and
• Applying narrowings post-hoc is equivalent to applying them during traversal.

For simple Narrowing::Type-style guards this holds, but the code explicitly excludes Correlated narrowings — good. Please add a comment or assertion documenting why non-correlated narrowings commute with the merge, since this is the core correctness argument and future narrowing variants may not satisfy it. Consider a match on Narrowing variants instead of matches!(.., Correlated) so new variants force a review.

3. Casts exclusion is correct but under-documented (Low)

path.casts.is_empty() is required because casts don't commute with merges. The comment mentions this, but a one-line note on why (casts are order-sensitive / not idempotent under merge) would help future maintainers.

4. Test robustness (Low)

The test relies on new_with_init_std_lib() and "every check" being enabled, per the comment. This is fragile:

• If std lib loading changes, the test may silently stop exercising the bug (the comment even admits "the test passes without the fix" under weaker conditions).
• Consider asserting the diagnostic is produced when the guard is removed (a negative control), or adding a direct unit test on trace_decl to lock in the behavior independent of std lib.

5. Doc comment consistency (Nit)

The updated TraceStateKey doc says "The memo is consulted only for nodes with no loop behind them" — but the new code path also consults the memo for nodes with incoming narrowings (via the recursive empty-path call). The doc is still accurate for the memo's contents, but consider clarifying that callers may layer guards on top.

6. Minor: redundant condition ordering

if !path.narrowings.is_empty()
    && path.casts.is_empty()
    && !path.narrowings.iter().any(...)

The !path.narrowings.is_empty() check is redundant with the any check (an empty iterator yields false, so !any is true). It's harmless and arguably clearer, but could be simplified to just the casts.is_empty() && !any(...) pair.

Verdict

The fix is logically sound for the stated case and the test covers the regression. Main follow-ups: document the commutation argument for narrowings, verify visited safety in the recursive call, and consider a negative-control test.