Skip to content

Hand-written Expr walkers with catch-all arms silently skip new variants — #10445 was the second instance; audit and make the total ones exhaustive #10654

Description

@proggeramlug

The pattern

CLAUDE.md's "Common Pitfalls" already names this for one function:

Closure Capturescollect_local_refs_expr() must handle all expression types — catch-all silently skips refs

PR #10650 (Fixes #10445) is the same defect in a different walker. replace_this_in_expr /
replace_this_in_stmts (crates/perry-hir/src/analysis.rs) had no arm for Expr::GetIterator,
GetAsyncIterator, MapEntries or SetValues — the wrappers a for…of iterable lowers to when it cannot be
proven a plain Array/Map/Set. A this.gen() inside one fell through the catch-all, stayed unreplaced, and
evaluated to undefined outside any method body. The fix was four added arms mirroring the existing
Await/TypeOf/Void ones.

The shared cause is structural, not a coding mistake: these walkers match on Expr with a catch-all _ =>
arm, so adding a new Expr variant compiles cleanly everywhere and silently does the wrong thing in every
walker that should have recursed into it. The compiler cannot help, and the symptom appears far from the cause
undefined at runtime, in a different function, with no diagnostic.

Suggested work

  1. Audit the existing walkers. Find every hand-written Expr/Stmt traversal with a catch-all and check
    which recently-added variants it silently skips. Known instances: collect_local_refs_expr,
    replace_this_in_expr/replace_this_in_stmts. There are likely more.
  2. Remove the catch-alls where the walker is meant to be total. An exhaustive match makes adding an Expr
    variant a compile error in exactly the places that must be updated — which is the outcome we want. Where a
    walker genuinely only cares about a few variants, an explicit _ => {} with a comment saying so is fine;
    the problem is the ones that are supposed to be exhaustive and aren't.
  3. Consider a test or lint that enumerates Expr variants and asserts each total walker handles them, for
    cases where an exhaustive match isn't practical.

This is hardening rather than a user-visible bug, but the class has now produced at least two real defects
(#10445 and whatever motivated the collect_local_refs_expr note), and each one presented as an unrelated
runtime undefined far from the actual omission.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    package-auditFound by the 2026 package audit: compiling real npm packages from source instead of native bindings

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions