Skip to content

Report values assigned to variables that are never read - #6330

Open
ondrejmirtes wants to merge 21 commits into
2.3.xfrom
unused-variables
Open

Report values assigned to variables that are never read#6330
ondrejmirtes wants to merge 21 commits into
2.3.xfrom
unused-variables

Conversation

@ondrejmirtes

@ondrejmirtes ondrejmirtes commented Sep 1, 2026

Copy link
Copy Markdown
Member

Reports local variables whose assigned value is never read afterwards on any path (variable.unused, level 4, behind the new unusedVariable bleeding-edge toggle).

⚠️ The last commit enables the toggle for everyone on purpose, so that CI and the downstream dogfooding projects surface false positives — revert Temporarily enable unusedVariable for everyone before merging.

How it works

A write is dead iff no path from it reaches a read of the written value — so $a = 1; $a = 2; echo $a; reports line 1 and if (c) { $a = 1; } return; reports the write, while for ($i = 0; $i < 3; $i++), back-edge reads in loops, $x[] = ...; return $x; etc. do not.

  • Every source-level write site of a local variable (assignment, compound assignment, inc/dec, offset write on an array/string, list() item, foreach value/key, catch variable) becomes an immutable VariableWrite with a VariableWrittenExpr marker in the scope's expression types (__phpstanVariableWritten($x, <id>), listed as a compositional virtual key, PHP + turbo mirror). A new write of the same variable kills the earlier markers explicitly (assignVariable(..., write:, supersededMarkerExprs:) — the scope is told what to kill, it never consults engine state); merges keep markers as Maybe, so at any point they say which writes still reach it.
  • The one place a source-level read is priced (VariableHandler) records the reaching writes as read in an immutable, persistent VariableWritesFrame (one per function-like body; NodeScopeResolver holds the stack and swaps the top after each with*() transition; arrow functions share the enclosing frame; processNodes isolates the stack). compact(), get_defined_vars(), extract(), eval, include read everything; goto makes the frame opaque; by-ref parameters/uses, global, static and reference aliases are untracked.
  • The frame is emitted as a VariableWritesNode after each *ReturnStatementsNode; UnusedVariableRule reports the unread, tracked writes (catch variables only where non-capturing catches exist; $_-prefixed names are exempt).

Engine fixes needed on the way: generalizeWith() now carries markers planted only by a later loop pass (a branch dead while the variable was still null), createConditionalExpressions() no longer records certainty-No conditionals for virtual nodes (a later narrowing could have erased a marker), ClosureTypeResolver's body walk gets its own throwaway frame, and reads walked in consume-stored mode (arguments of a nullsafe call's plain twin) count as reads.

The first commit removes the 30 genuine dead stores the rule found in src/ and tests/.

Verification

  • Rule test with 31 cases (Psalm's valid/invalid catalogue as the trap list; fail-first verified), make tests green turbo-off and turbo-on, make phpstan clean in both modes, cs/lint clean.
  • Dogfood on slevomat (bleeding edge, final build): 48 reports = 41 true positives (two real bugs among them: a duplicate list() target and an offset write on an array the closure had already captured by value) + 7 writes in branches PHPStan itself proves unreachable (always-false is_int() guard, impossible isset(), dead catch) — left as is, consistent with the existing *.alwaysFalse / catch.neverThrown diagnostics.
  • Performance (locally built phars, fork + turbo, cold cache, ABBA pairs): slevomat +0.7 % user CPU (5 pairs, t = 0.8, n.s.); self-analysis +1.55 % (8 pairs, sd 0.82, t = 6.4); on src/Type alone the tracking is ≈ 3.7 % (reads ≈ 2.5 %, scope markers ≈ 1–2.7 %).

Follow-ups (not in this PR)

  • Psalm's value-flow refinement ($b = $b + 1 chains that never reach a sink): reads in pure RHS positions become dependency edges between writes, the rule computes a fixpoint.
  • Unused array-literal offsets ($a = ['a' => 1, 'b' => 2] with only $a['b'] read) — bounded per-offset sub-writes.
  • Known false negative: $x = 1; unset($x); (unset() walks the variable as a read).
  • Whether f($raw = true) (value consumed, variable never read) and writes in PHPStan-proven-dead branches should be reported is a policy call.

Closes phpstan/phpstan#12789
Closes phpstan/phpstan#12012
Closes phpstan/phpstan#11483
Closes phpstan/phpstan#10202

🤖 Generated with Claude Code

https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy

ondrejmirtes and others added 11 commits September 7, 2026 17:41
Dead stores found by the upcoming unused-variable check: null
initialisations overwritten on every path, unused foreach values and
destructured items, an assignment from the never-returning fail(), and
catch variables that are never read.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Every source-level write of a local variable (plain and compound
assignment, inc/dec, array-offset write on an array or string, list()
item, foreach value/key, catch variable) becomes a VariableWrite with a
VariableWrittenExpr marker in the scope. A new write of the same
variable kills the earlier markers explicitly; merges keep them as Maybe,
so at any point the markers say which writes still reach it. The one
source-level read of a variable (VariableHandler) records the reaching
writes as read in an immutable per-function-like VariableWritesFrame
held by NodeScopeResolver; compact(), get_defined_vars(), extract(),
eval and include read everything, goto makes the frame opaque, and
by-ref parameters/uses, global, static and reference aliases are
untracked. The frame is emitted as a VariableWritesNode after each
ReturnStatementsNode.

generalizeWith() now carries markers planted only by the newer loop
pass, and createConditionalExpressions() no longer records
certainty-No conditionals for virtual nodes (a later narrowing could
otherwise erase a marker).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
…e never read

Level 4, behind the unusedVariable bleeding-edge toggle. A write is
reported when no path from it reaches a read of the written value -
including writes overwritten before being read and writes on only one
branch. Catch variables are reported only where non-capturing catches
exist; $_-prefixed names are exempt.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
A nullsafe call's plain twin re-enters the already-walked receiver in
consume-stored mode; its arguments are walked there for the first time,
so their variable reads are genuine and must not be treated like
on-demand synthetic pricing. Found on slevomat: every variable read only
inside the arguments of $x?->m(...) was reported as never read.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Reads are recorded on walk scopes, never on a promoted one, so the
marker is only needed in the phpDoc-typed map; keeping it out of the
native map halves its share of every merge, generalization, equality
check and invalidation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
So that CI and the downstream dogfooding projects surface false positives
before the rule ships behind bleeding edge only. Revert before merging.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Closes phpstan/phpstan#12789

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Closes phpstan/phpstan#12012

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Closes phpstan/phpstan#11483

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Closes phpstan/phpstan#10202

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
ondrejmirtes and others added 10 commits September 7, 2026 18:18
simple-downgrader cannot downgrade named arguments that skip an optional
parameter, so the write:/supersededMarkerExprs: assignVariable() call sites
and the writeSiteKind: processVirtualAssign() call sites left named
arguments in the PHP 7.4 build and its lint failed to parse them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
The rule already skips KIND_CATCH writes when
PhpVersion::supportsNoncapturingCatches() is false - a catch clause cannot
drop the variable before PHP 8.0, so there is nothing actionable to report.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
…'s constructor

This will help not report KIND_CATCH in projects with `php: ^7.4`
in composer.json
…le-writes engine

UnusedClosureUsesRule and UnusedConstructorParametersRule become
Rule<VariableWritesNode> and UnusedFunctionParametersCheck - a syntactic
re-scan of the body with its own blind spots - dies.

The frame now records which variable names the body mentions at all (a
read, a write target, a statement naming the variable, or a construct that
can see every variable: eval, include, a dynamic compact() or $$name;
func_get_args() observes every parameter's original value without reading
the variables). On top of that, a constructor's parameters and a closure's
by-value uses are registered as initial writes (KIND_PARAMETER,
KIND_CLOSURE_USE) whose markers are planted on the body's entry scope
(MutatingScope::withVariableWriteMarkers(), re-planted on each by-ref
convergence pass), so the rules ask the precise question: is the incoming
value ever read? By-ref parameters and uses fall back to "is the name
mentioned"; UnusedVariableRule skips the import kinds - they belong to the
dedicated rules. VariableWritesNode now exposes its function-like and is
documented as emitted with the scope inside it.

This is more precise than the old check in both directions, covered by new
tests: a $$name or compact() argument resolved during the walk counts only
the variables it can actually name (the old check treated an unresolvable
one as using everything, or nothing, depending on the construct), and a
parameter or use whose value is overwritten before any read is now reported
even though its name appears in the body.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
isAnalysedFile() is keyed by normalized paths (TraitUseHandler and
CalledMethodProcessor look traits and methods up that way), but the stored
set used the caller-provided strings verbatim. A caller passing an
unnormalized path - mixed directory separators on Windows, as
AnalyserIntegrationTest does with __DIR__-based lists - silently skipped
the in-class-context analysis of a trait, which made testBug6253 miss the
dead-store report there. The bug-6253 file list now keeps a deliberately
unnormalized entry as a cross-platform guard.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Found by the upcoming UnusedMethodParametersRule: refactor leftovers in the
expression handlers ($nodeScopeResolver, $storage threaded along but no
longer consumed), TypeNodeResolver::resolveThisTypeNode()'s $typeNode and
the fixer entry points' $onlyFiles/$output.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
UnusedFunctionParametersRule and UnusedMethodParametersRule run on
VariableWritesNode: every function's and method's non-promoted, non-by-ref
parameters are registered as KIND_PARAMETER initial writes, so the rules
report a parameter whose incoming value is never read - a completely unused
one as well as one overwritten before any read. By-ref parameters fall back
to "is the name mentioned"; func_get_args() keeps everything unreported; a
function-like with an empty body is a deliberate no-op stub and is skipped.

Parameters the signature's PhpDoc contract refers to are exempt: the
subject of a phpstan-assert tag and a parameter that a conditional type in
the signature switches on serve the call site even when the body ignores
them.

The methods rule is limited to private methods for now - a public or
protected signature may be dictated by an interface, a parent or an
override - and leaves magic methods and constructors (which have their own
rule) alone. Both rules sit at level 4 behind the new unusedParameters
bleeding-edge toggle.

The shared per-parameter logic lives in UnusedParametersCheck, now also
used by UnusedConstructorParametersRule. PHPStan's own no-op analysis
functions (PHPStan\dumpType(), PHPStan\Testing\assertType(), ...) carry
explicit ignores - their parameters are consumed by the analyser, not by
the function bodies. The rules found two genuine dead parameters in
integration fixtures (bug-7012, bug-7153), now expected.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Same experiment as unusedVariable: CI and the downstream dogfooding
projects surface false positives before the rules ship behind bleeding
edge only. Revert before merging.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Found by the upcoming redundant-assignment detection: a null re-assignment
in a branch where the variable is provably still null, null-if-null
else-arms, and a null assignment after a fail() that proves it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Closes the second half of phpstan/phpstan#10202:
when the variable's current type allows exactly one value
(getFiniteTypes() count of 1) and the assigned type equals it, the
assignment is provably redundant. AssignHandler evaluates this at each
plain assignment site - in the native type flavour too, so a phpDoc-only
certainty never reports on its own - and records it per write in the
frame, re-evaluated on every convergence pass so a loop-widened variable
does not keep a stale first-pass verdict. UnusedVariableRule reports it
(identifier variable.redundantAssignment) for writes that are read; an
unread write keeps the stronger never-read report.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant