Skip to content

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

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

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

Conversation

@ondrejmirtes

@ondrejmirtes ondrejmirtes commented Sep 1, 2026

Copy link
Copy Markdown
Member

Follow-up to #6330 (merged): the two refinements listed there, the unset() false negative, and a few things found while dogfooding.

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 build the target write up front (VariableFlowBuilder::writeSite()) and walk the value in enterValueFlow(); 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() emits a read flow tagged with the target's id instead of a plain read. VariableLivenessResolver turns those into dependency edges between writes during its backward pass and resolves the used set as a worklist fixpoint from the sinks (resolveDependencies()); VariableWritesNode exposes both answers as isRead() (some read of the value exists) and isUsed() (the value reaches a sink).
  • $a = $b = $c + 1 and $a = ($b OP= …): the inner write's inputs are handed to the enclosing expression as a VariableInputFlow — copied to the outer target, or treated as consumed when the enclosing expression is a sink. $a = $b = 1; sink($a); still reports $b (its variable is never read).
  • UnusedParametersCheck and UnusedClosureUsesRule ask isUsed() too, so a parameter or by-value capture that only feeds unused values is reported.
  • Assignments to $this and superglobals are never write sites, so $_GET['x'] = $value; keeps $value observable.

The message tells the two apart: … is never read. when nothing reads the written value at all, … is never used. when it is read, but only by expressions computing other values that never reach a sink.

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). They are observed together with the whole write in the liveness pass and resolved through it.
  • $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, and are reported when a constant-offset read is the only thing after them ($a[$i] = 1; $a['x'] = 2; return $a['x'];).
  • 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.
  • The redundant-assignment check covers constant-offset writes: $a = [1, 2]; $a[0] = 1; reports Offset $a[0] is assigned value 1 but it already has that value. (nested offsets and coerced keys included; append, dynamic keys, by-reference items and PHPDoc-only certainty are excluded).
  • unset($x) / unset($a['k']) discard the reaching writes without reading them (VariableFlow::discard()), so $x = 1; unset($x); is reported too — unless releasing the value has side effects (objects, resources, arrays that may contain them), where the unset() counts as a read so $cache = new Cache(); unset($cache); stays quiet.

Messages: Value assigned to variable $x is never used., Value assigned to $a['k'] is never read. (also Value of $a['n'] after ++ …, Foreach value $a['x'] …), Offset 'k' of array assigned to variable $a is never used. (array.unusedOffset) — an item is reported on its own only when the array as a whole is used, otherwise the assignment is.

Verification

  • Rule test: 25 tests / 214 expectations; new fixtures unused-variable-value-flow.php, unused-variable-offsets.php, unused-variable-redundant-offsets.php, unused-variable-offset-overwrite.php, unused-variable-destructor.php, plus unused-input-value-flow.php for the parameter and closure-use rules. The #12012 case now reports exactly the lines the reporter asked for.
  • Two 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.
  • On the current tip: the three rule tests green (UnusedVariableRuleTest, UnusedClosureUsesRuleTest, UnusedFunctionParametersRuleTest), make phpstan clean, --group levels green — the one new expectation file (arrayDimFetches-4.json) is part of this PR.
  • Slevomat dogfood and the perf A/B were done on an earlier revision of this branch (before Report values assigned to variables that are never read #6330 landed as the flow-tree engine): 54 reports = the base's 48 + 6 new, all true positives; the perf difference was inside the noise. Neither has been re-measured on the current tip.
  • The integration-tests jobs are red because the downstream projects run with bleeding edge and now see the unused-variable/parameter reports (the same as on the base PR); doctrine/collections fails on a property type error that reproduces identically on plain 2.3.x (ArrayCollection::$elements does not accept array<int|TKey, T>), not on this branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CbZPnVnRJDLbnqmtD3sYSy

@ondrejmirtes
ondrejmirtes force-pushed the unused-variables-followups branch 2 times, most recently from 6041802 to 8e260fa Compare September 7, 2026 16:33
@ondrejmirtes
ondrejmirtes force-pushed the unused-variables branch 6 times, most recently from f13bef3 to 0a3ccf8 Compare September 9, 2026 20:44
@ondrejmirtes
ondrejmirtes force-pushed the unused-variables-followups branch from 8e260fa to 0de7e0c Compare September 10, 2026 07:23
@ondrejmirtes
ondrejmirtes force-pushed the unused-variables branch 2 times, most recently from 5de5f2f to d21d791 Compare September 10, 2026 08:36
@ondrejmirtes
ondrejmirtes force-pushed the unused-variables-followups branch 5 times, most recently from e76cfae to 8dce798 Compare September 10, 2026 11:48
@ondrejmirtes
ondrejmirtes force-pushed the unused-variables branch 2 times, most recently from 4f5a86e to 5195842 Compare September 10, 2026 12:03
Base automatically changed from unused-variables to 2.3.x September 10, 2026 12:04
@ondrejmirtes
ondrejmirtes force-pushed the unused-variables-followups branch from 8dce798 to 157dbbd Compare September 10, 2026 12:05
@ondrejmirtes
ondrejmirtes force-pushed the unused-variables-followups branch from 157dbbd to ad8ad52 Compare September 10, 2026 16:05
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