TCO warning: split benign vs. probably-leak (option E)#13
Merged
Conversation
The "TCO function not reachable from process root" warning fired 3/3 on
bounded structural AST walkers (rename_node, walk_node, walk_for_fields)
and 0/0 on the dangerous unbounded-recursion-under-long-running-loop
pattern it was designed for. With Phase 2a adding the third walker and
more on the LSP roadmap, signal-to-noise was eroding.
Diagnostic now splits in two based on whether an outer reset boundary
exists above the demoted fn:
- note: ...outer reset boundary above (process exit, PT loop, or PT TCO
back-edge) caps allocation per outer iteration. Likely benign...
(emitted when at least one caller chain reaches a process root or a
process_tail_fns member)
- warning: ...not reachable from any process root or process-tail
context...
(the original wording, now reserved for the genuinely dangerous case)
Mechanism: backward BFS through the call graph in mark_process_tail.
Three substrate blind spots had to be fixed for the BFS to find the
outer reset:
1. Selective imports (load "./mod" (fn_a)) desugar to
let fn_a = _mod_mod_fn_a; call sites use the alias, not the
prefixed fn_def. Resolve via a top_stmts scan.
2. Whole-module loads desugar to let mod = {fn_a: _mod_mod_fn_a, ...};
mod.fn_a(...) is a dot-call. Resolve via a two-level map.
3. Closure-mediated calls record edges into anon:N callers, losing the
connection to the lexical fn that built the closure. Track
anon_parent[anon:N] at AST-walk time and hop through it in BFS.
Heuristic, not proof: the note: assumes bounded inner recursion. The
principled fix (option B in OPTIMIZATIONS.md - structural-decrease
termination check) remains TODO and is documented with rationale for
why it's still worth doing despite E being landed.
All three in-tree false-positives downgrade to note:; bootstrap
roundtrip clean; make test green (118 files + 4 LSP smoke tests).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The "TCO function not reachable from process root" warning fired 3/3 on bounded structural AST walkers (
rename_node,walk_node,walk_for_fields) and 0/0 on the dangerous unbounded-recursion-under-long-running-loop pattern it was designed for. With Phase 2a adding the third walker and more on the LSP roadmap, signal-to-noise was eroding.This is option E from
docs/OPTIMIZATIONS.md"Tighten 'TCO function not reachable from process root' warning" — split the diagnostic into a benignnote:and a probably-leakwarning:without changing soundness. Option B (structural-decrease termination check) remains the principled long-term fix and stays tracked in OPTIMIZATIONS.md with rationale for why it's still worth doing.What changed
note: ...outer reset boundary above caps allocation per outer iteration. Likely benign...when at least one caller chain reaches a process root or aprocess_tail_fnsmember. The originalwarning:wording is reserved for the genuinely dangerous case (no outer reset above).mark_process_tailwalksincomingedges and the newanon_parentmap to find the outer reset boundary.load \"./mod\" (fn_a)) desugar tolet fn_a = _mod_mod_fn_a— call sites use the alias. Resolve via a top_stmts scan.let mod = {fn_a: _mod_mod_fn_a, ...}—mod.fn_a(...)is a dot-call. Resolve via a two-level map.anon:Ncallers, losing the lexical-parent connection. Trackanon_parent[anon:N]and hop through it in BFS.All three in-tree false-positives (
rename_node,walk_node,walk_for_fields) downgrade cleanly tonote:; zerowarning:in the codebase today.Soundness note
The
note:is a heuristic, not a proof — bounded inner recursion under the outer reset is assumed but not verified. That's why `note:` not `warning:`: the user is being told "this is probably fine, but if it isn't here's what to look at" rather than "you have a leak." The structural-decrease check (option B) is what closes the gap; it's documented in OPTIMIZATIONS.md with the same soundness bar that gated this change.Option A (suppress when reachable caller is PT, no termination check) was explicitly rejected — silently silencing a real leak class is the wrong direction. The rationale is preserved in OPTIMIZATIONS.md so it survives the next consideration.
Test plan
note:notwarning:(verified manually after rebuild).make bootstrap).make test— 118 examples + 4 LSP smoke tests green.make test-lsp— symbols, definition, completion smoke tests pass.