Skip to content

Report variable writes whose value never reaches a use, per array offset - #6334

Open
ondrejmirtes wants to merge 14 commits into
unused-variablesfrom
unused-variables-followups
Open

Report variable writes whose value never reaches a use, per array offset#6334
ondrejmirtes wants to merge 14 commits into
unused-variablesfrom
unused-variables-followups

Conversation

@ondrejmirtes

Copy link
Copy Markdown
Member

Follow-up to #6330: the two refinements listed there, plus the unset() false negative.

Value flow (Psalm's "better unused variable detection")

A read is no longer a use by itself. A write is used iff its value reaches a sink — a call argument, a condition, a return, echo, throw, a property write… — directly or through the writes it is computed into. So $a = 5; $a = $a + 1;, $s .= … chains, $i++ runs and Psalm's article example are reported at every write, while $a = 5; $a = $a + 1; sink($a); stays quiet.

  • ExpressionContext carries a value-flow target. AssignHandler, AssignOpHandler and the inc/dec handlers register the write before walking its value (registerWriteSite()); pure combinators (arithmetic/concat/comparisons, casts, unary ops, ternary and match arms, literal arrays, interpolated strings, ??'s right side) keep the target via enterDeepKeepingValueFlow(), everything else drops it — enterDeep() for nested sub-expressions, withoutValueFlow() for same-depth sinks (&&/|| right operands, piped calls, closure uses, assignment-target sub-walks).
  • VariableHandler::composeResult() records a dependency edge in the VariableWritesFrame instead of a read when a target is set; VariableWritesNode resolves the used set as a fixpoint over the edges.
  • $a = $b = $c + 1 and $a = ($b OP= …) copy the inner write's dependencies to the outer target; $a = $b = 1; sink($a); still reports $b (its variable is never read).

Array offsets — literals and $a['k'] = …

  • Items of a literal assigned straight to a variable become child writes of the whole write (constant keys, implicit indices after explicit ones, spreads/unknown keys as unknown offsets; TRACKED_LITERAL_ITEMS_LIMIT = 32). Their markers are planted together with the whole write's.
  • $a['k'] = … is an offset write that kills only the earlier writes of offset 'k'; $a['k']['j'] = … extends offset 'k' (reads it, kills nothing); $a[$i] = … / $a[] = … are unknown-offset writes.
  • The receiver of an offset access is read as a container (whole-variable writes only); $a['k'] reads offset 'k' plus unknown-offset writes; a dynamic offset reads everything; $a['k'] OP= and $a['k'] ??= read the offset first.
  • unset($x) / unset($a['k']) discard the reaching writes without reading them (MutatingScope::withoutVariableWriteMarkers()), so $x = 1; unset($x); is reported too.

Messages: Value assigned to variable $x is never used., Value assigned to $a['k'] is never used., Offset 'k' of array assigned to variable $a is never used. — an item is reported on its own only when the array as a whole is used, otherwise the assignment is.

Verification

  • Rule test: 10 tests / 167 expectations across four fixtures (unused-variable-value-flow.php and unused-variable-offsets.php are new); the #12012 case now reports exactly the lines the reporter asked for.
  • make phpstan clean (it caught a real false positive on the way — $cache[$k] ??= … in ScopeOps, since PreparedAssignTarget::isAssignOp() is false for coalesce mode; fixed and covered), make tests green turbo-off and turbo-on, cs/lint clean.
  • Two more genuine bugs in fixtures surfaced and are now expected: bug-10847's loop appends to the iterated $overloads instead of $processedOverloads, and array-destructuring-array-dim-fetch builds $barcodes for nothing.
  • Slevomat dogfood (phar of this branch): 54 reports = the base PR's 48 + 6 new, all true positives (a dead foreach filling $tags, $headerTable feeding an already-dead offset write, four literal offsets always overwritten or never read).
  • A single-process A/B on src/Type was too noisy to resolve a difference (−0.4 %, t ≈ 0); no large regression.

The levels expectation JSONs (tests/PHPStan/Levels/data/*-4.json) are not part of this PR — the levels config includes bleeding edge, so both branches need them once the base PR settles.

🤖 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 3 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
A read no longer counts as a use by itself: a write is used iff its value
reaches a sink (a call argument, a condition, a return, echo, throw, a
property write...) directly or through the writes it is computed into. The
right side of an assignment, a compound assignment and the operand of an
increment are walked with the write as the ExpressionContext's value-flow
target; pure combinators (arithmetic, concat, casts, unary ops, ternary and
match arms, literal arrays, interpolated strings, ??'s right side) keep it,
everything else drops it (enterDeep()/withoutValueFlow()). VariableHandler
then records a dependency edge instead of a read, VariableWritesNode
computes the used set as a fixpoint over the edges and the rule reports
`$b = $b + 1` chains, `$s .= ...` chains and `$i++` runs that never reach a
sink, as Psalm does. A nested assignment or compound assignment copies its
dependencies to the enclosing target.

Array offsets are tracked as their own writes of the variable: a literal
assigned straight to a variable registers one write per constant offset
(items of `$a = ['x' => 1, 'y' => 2]`, up to TRACKED_LITERAL_ITEMS_LIMIT),
`$a['k'] = ...` is an offset write that kills only the earlier writes of
offset 'k', `$a['k']['j'] = ...` extends offset 'k' (reads it, kills
nothing). The receiver of an offset access is read as a container (its
whole-variable writes only), `$a['k']` reads offset 'k' plus the writes of
unknown offsets, a dynamic offset reads everything, `$a['k'] OP=` and
`$a['k'] ??=` read the offset first. unset() discards the reaching writes of
a variable or a constant offset without reading them (MutatingScope::
withoutVariableWriteMarkers()), so `$x = 1; unset($x);` is reported too.

Messages: "Value assigned to variable $x is never used.", "Value assigned
to $a['k'] is never used." and "Offset 'k' of array assigned to variable $a
is never used." (an item is reported on its own only when the array as a
whole is used).

The stronger rule found two more genuine bugs in fixtures: bug-10847's
loop appends to the iterated $overloads instead of $processedOverloads,
and array-destructuring-array-dim-fetch builds $barcodes for nothing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy
@ondrejmirtes
ondrejmirtes force-pushed the unused-variables-followups branch from 6041802 to 8e260fa Compare September 7, 2026 16:33
@ondrejmirtes
ondrejmirtes force-pushed the unused-variables branch 2 times, most recently from 2152a04 to 88d0188 Compare September 8, 2026 09:30
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