lint: recurse W012/W013 into unobserved blocks and match arms - #787
Conversation
check_builtin_shadow recursed only through IF/LOOP/FOR/FUNC/TRY/PROGRAM, breaking on AST_UNOBSERVED and AST_MATCH, so an assignment (W012) or define (W013) shadowing a builtin inside an unobserved block or a match arm lint'd clean while the same shadow at top level warned. Recurse into both, matching the shape collect_refs/collect_loop_assign_names already use for these node types. Diagnostics themselves are unchanged. Closes InauguralSystems#784 Closes InauguralSystems#785 Co-Authored-By: Kimi K3 <noreply@kimi.com>
InauguralPhysicist
left a comment
There was a problem hiding this comment.
Review — approved
Correct fix, correctly scoped. Verified against the tree, not just the diff:
- Recursion shape is right.
AST_MATCHiteratescase_count/body_counts[i]/bodies[i][j]andAST_UNOBSERVEDusesdata.block— both byte-match the shapescollect_refsalready uses for these nodes, as claimed. No invented traversal. - No missed arm. I checked
eigenscript.h's match struct for a separate else/default body the recursion could skip — there isn't one; a default arm is just another pattern inpatterns[], socase_countcovers everything. - Skipping
match.exprandpatterns[i]is correct, not an oversight. W012 fires on assignment and W013 ondefine, and neither can occur in a subject expression or a pattern —isis a statement. (Lambdas in expressions are already in the walker's break group at top level, so behavior stays consistent.) - The
-Werror=switchcontract is preserved — the two labels moved from the break-only group to the recursing group, so the exhaustiveness gate #779 installed still covers this switch, and the comment explaining why the break group is enumerated survives.
The evidence discipline is what makes this an easy approve: red-then-green proven by reverting src/lint.c (4 red → 4 green), and the zero-diff W012/W013 sweep across every .eigs in the repo answers the crying-wolf question before I had to ask it — the widening surfaces nothing on our own sources.
One note for the sibling gaps (#780–#783): when you get to collect_assigns and friends, the same "does match have a hidden arm" and "which sub-expressions can legally contain the construct" checks apply per-walker — worth stating per PR as you did here.
CHANGELOG n/a is fine by this repo's convention (diagnostics text unchanged; only where the walker looks changed).
collect_assigns broke on AST_MATCH, so a variable assigned only inside a match arm was never recorded and W001 never fired. Mirrors #787's shape for the same lint-gap family, one walker earlier in the same file. Co-authored-by: Kimi K3 <noreply@kimi.com>
…match arms (#782) (#792) check_func_unreachable recursed only through IF/LOOP/FOR/FUNC/TRY/PROGRAM, breaking on AST_UNOBSERVED and AST_MATCH, so a function defined inside an unobserved block or a match arm never had its body scanned and W003 never fired there while the same dead code at top level warned. Recurse into both, matching the shape check_builtin_shadow (#787) already uses for these node types. Diagnostics themselves are unchanged. Closes #782 Co-authored-by: Kimi K3 <noreply@kimi.com>
…ch arms (#793) check_unused_params recursed only through IF/FUNC/FOR/PROGRAM, breaking on AST_UNOBSERVED and AST_MATCH, so a define inside an unobserved block or a match arm never had its parameters checked and W002 lint'd clean while the same unused parameter at top level warned. Recurse into both, matching the shape check_builtin_shadow (#787) and collect_assigns (#790) already use for these node types. Diagnostics themselves are unchanged. Closes #781 Co-authored-by: Kimi K3 <noreply@kimi.com>
…ms (#794) check_dup_keys broke on AST_UNOBSERVED and AST_MATCH, so a dict literal with a duplicate key inside an unobserved: block or a match node (scrutinee, case pattern, or case body) was never reached and W010 never fired. Recurse into both, matching the shape #787 and #790 use for the same lint-gap family in this file and the pattern-walk collect_refs already uses. Diagnostics themselves are unchanged. Closes #783 Co-authored-by: Kimi K3 <noreply@kimi.com>
What does this PR do?
Closes #784 and #785 — one recursion fix closes both, because W012 and W013 share a walker.
check_builtin_shadowrecursed only throughIF/LOOP/FOR/FUNC/TRY/PROGRAM, withAST_MATCHandAST_UNOBSERVEDin its break-only group. So a shadow that warned at top level lint'd clean inside anunobserved:block or amatcharm:Both node types now recurse, following the shape
collect_refsandcollect_loop_assign_namesalready use for them rather than inventing one. Diagnostic codes, messages and suppression logic are untouched — this only changes where the walker looks.Scope is deliberately these two. The sibling gaps in #780–#783 live in four other walkers (
collect_assigns,check_unused_params,check_func_unreachable,check_dup_keys) and are separate changes.Checklist
make testpasses locally —RESULTS: 3279/3279 passed, 0 failed, against a3275/3275baseline onmainin a separate worktree. The +4 are the new tests; no pre-existing test broke.docs/BUILTINS.md— n/adocs/STDLIB.md— n/aFour regression tests in
tests/test_lint.sh, in the existingcheck_containsfixture style, one per newly-firing case. Proven non-vacuous rather than merely passing: revertingsrc/lint.cand rebuilding turns all four red —Results: 114 passed, 4 failed— and restoring returns118 passed, 0 failed.The thing I'd most want checked in review, since a recursion widening is exactly where a linter starts crying wolf: I diffed
--lintW012/W013 counts between themainbinary and this branch's binary across every.eigsinlib/ tests/ examples/ tools/ bench/ web/ fuzz/. Zero differences — the fix surfaces nothing new against the repo's own sources, so it is not about to flood anyone's build.Generated by Claude Opus 5 (brief, review), Kimi K3 (implementation)