Simulate loop analysis for goto-label pairs#5701
Conversation
| $backwardGotoLabels = []; | ||
| $labelIndices = []; | ||
| foreach ($stmts as $idx => $s) { | ||
| if (!($s instanceof Node\Stmt\Label)) { | ||
| continue; | ||
| } | ||
|
|
||
| $labelIndices[$s->name->toString()] = $idx; | ||
| } | ||
| $nodeFinder = new NodeFinder(); | ||
| if ($labelIndices !== []) { | ||
| foreach ($labelIndices as $labelName => $labelIdx) { | ||
| $stmtsAfterLabel = array_slice($stmts, $labelIdx + 1); | ||
| /** @var Goto_[] $gotos */ | ||
| $gotos = $nodeFinder->findInstanceOf($stmtsAfterLabel, Goto_::class); | ||
| foreach ($gotos as $goto) { | ||
| if ($goto->name->toString() === $labelName) { | ||
| $backwardGotoLabels[$labelName] = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| $nestedBackwardGotoLabels = []; | ||
| foreach ($stmts as $idx => $s) { | ||
| if ($s instanceof Node\Stmt\Label) { | ||
| continue; | ||
| } | ||
| /** @var Node\Stmt\Label[] $nestedLabels */ | ||
| $nestedLabels = $nodeFinder->findInstanceOf([$s], Node\Stmt\Label::class); | ||
| if ($nestedLabels === []) { | ||
| continue; | ||
| } | ||
| /** @var Goto_[] $nestedGotos */ | ||
| $nestedGotos = $nodeFinder->findInstanceOf([$s], Goto_::class); | ||
| if ($nestedGotos === []) { | ||
| continue; | ||
| } | ||
| $nestedLabelNames = []; | ||
| foreach ($nestedLabels as $nestedLabel) { | ||
| $nestedLabelNames[$nestedLabel->name->toString()] = true; | ||
| } | ||
| foreach ($nestedGotos as $nestedGoto) { | ||
| $gotoName = $nestedGoto->name->toString(); | ||
| if (!isset($nestedLabelNames[$gotoName])) { | ||
| continue; | ||
| } | ||
|
|
||
| $nestedBackwardGotoLabels[$idx][$gotoName] = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Would be really nice if we were able to move this code to a richParserNodeVisitor and if it ran very efficiently when traversing the AST. I don't want to do it here because the same code might be analysed multiple times and this would waste a lot of resources. i'm talking about code between lines 413-464.
Would be nice if the attributes on AST Node would be set just once on a specific node, like Label or Goto_ so that it's not spammed on all nodes for example.
There was a problem hiding this comment.
Done. The review feedback has been addressed with a single commit that:
-
Created
src/Parser/GotoLabelVisitor.php— arichParserNodeVisitorthat runs once per file parse (not per analysis pass) and sets attributes on specific AST nodes:hasBackwardGotoonLabelnodes that have a backward goto targeting themnestedBackwardGotoLabelson compound statement nodes (likeTryCatch) that contain both a label and a goto to it
-
Updated
src/Analyser/NodeScopeResolver.php— removed the 53-line scanning block (the$backwardGotoLabels/$nestedBackwardGotoLabelscomputation withNodeFinder) and replaced the two consumption sites withgetAttribute()calls reading from the pre-computed visitor attributes.
The attributes are only set on the specific relevant nodes (Label and compound statements), not spammed on all nodes, as the reviewer requested.
|
Done. Applied |
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
The NodeVisitor is forgetting about PropertyHook with Stmt[] body.
Write some failing tests about property hooks analysis (guarded for PHP 8.4+) and make them pass.
Also what about cross-boundary analysis? If a method contains a closure, the closure cannot goto to a label outside of the closure. If a method contains anonymous class, it also cannot jump in and out of it.
Verify the correctness in this regard. No stabilization and Scope-merging should take place in this case. Try a failing test first if you think the visitor has this problem.
|
Done. Here's a summary of the changes: PropertyHook fix — Cross-boundary fix — Replaced Tests added:
|
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
Now please add a new rule about goto for an undefined label. It should live on level 0 and the reported error should be non-ignorable (because PHP itself fails with "Fatal error: 'goto' to undefined label 'test'").
Also test that it correctly sees cross-boundary (it should report undefined label even if there's label with that name but in a different boundary). So it should use the same node attributes set by the visitor as NodeScopeResolver uses.
The identifier could be goto.labelUndefined.
Also make another rule, on level 4, with ignorable error, about unused label. The identifier could be label.unused. The rule should only be enabled with bleeding edge (add a new key to featureToggles config).
|
Done. Here's a summary of what was implemented across two commits: Commit 1:
Commit 2:
Both |
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
I feel that the visitor should be written more efficiently. Look at TryCatchTypeVisitor. In enterNode it notes down to a stack when entering a new function etc. And when entering more nodes (the NodeTraverser using the visitor already traverses the AST) it decides what to set on them.
Also, Couldn't we use the existing attributes instead of also setting GOTO_LABEL_UNDEFINED_ATTRIBUTE and LABEL_IS_USED_ATTRIBUTE?
Take into account memory and time complexity of the code to optimize it.
|
Pushed. Here's a summary of what changed: Rewrote
The 4 attributes ( |
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
Exclude the two files failing in https://github.com/phpstan/phpstan-src/actions/runs/26086425070/job/76700590574?pr=5701 from make lint in Makefile.
|
Done. Added three
All three checks pass: |
ondrejmirtes
force-pushed
the
create-pull-request/patch-k9brwhl
branch
from
f02156e to
6157f11
Compare
Treat goto as a terminating statement with exit points, and implement loop-like stabilization for backward goto-label pairs at both the same statement-list level and across nested structures like try-catch. The GotoLabelVisitor runs once per parse and stores the resulting HAS_BACKWARD_GOTO_ATTRIBUTE and NESTED_BACKWARD_GOTO_LABELS_ATTRIBUTE on the relevant nodes. A bottom-up subtreeData computation leverages the NodeTraverser's existing AST walk and stops at scope boundaries (Function_, ClassMethod, Closure, ClassLike, PropertyHook) so goto-label pairs in nested scopes do not affect outer scope analysis. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
GotoLabelVisitor now sets GOTO_LABEL_UNDEFINED by collecting all labels and gotos within each scope boundary (function, method, closure, property hook, file). The new GotoUndefinedLabelRule (level 0) reports a non-ignorable error when a goto targets a label that does not exist in the same scope, including cross-boundary cases (e.g. goto in closure targeting a label in the enclosing function). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The new UnusedLabelRule reports when a label is never targeted by any goto in the same scope. It uses the LABEL_IS_USED attribute set by GotoLabelVisitor. Enabled only with bleeding edge via the unusedLabel feature toggle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ondrejmirtes
force-pushed
the
create-pull-request/patch-k9brwhl
branch
from
6157f11 to
7bb934a
Compare
2 participants
Summary
This PR treats
gotoas a terminating statement (likebreak/continue/return) and implements loop-like scope stabilization for backward goto-label pairs (where agotojumps back to an earlier label, creating a loop).Changes
gotoas a terminating statement: Agotostatement now produces an exit point carrying its scope, and marks execution as terminated. This fixes forward-goto cases where code after agotowas not recognized as unreachable, and where labels after terminating code were not recognized as reachable viagoto.Same-level backward goto stabilization: When a label at the current statement-list level has a
gototargeting it from later in the same list, the statements between the label and the goto are processed in a stabilization loop (usingNoopNodeCallbackandenterDeep(), with generalization afterGENERALIZE_AFTER_ITERATIONand a cap atLOOP_SCOPE_ITERATIONS), exactly matching the existing loop analysis pattern forwhile/for/foreach.Nested backward goto stabilization: When a compound statement (e.g.,
try-catch) contains both a label and agototargeting it across sub-blocks (label intrybody,gotoincatchbody), the containing statement is processed through the same stabilization loop before the real analysis pass.Label reachability: Labels now un-terminate execution when reached via a forward
gotoexit point. The unreachable-statement detection (getNextUnreachableStatements) stops at Label nodes to avoid false positives.Regression tests
bug-4674.php(NSRT) — forward goto type narrowingbug-7711.php(NSRT) — backward goto null-check retry loopbug-7734.php(NSRT) — forward goto resource narrowingbug-9904.php(NSRT) — forward goto skips foreachgoto-label-stabilization.php(NSRT) — comprehensive stabilization tests (forward narrowing, backward loops, retry patterns, multiple gotos to same label)bug-11731.php(UnreachableStatementRule) — unreachable code after gotobug-12167.php(NumberComparisonOperatorsConstantConditionRule) — try-catch retry pattern with nested goto-labelbug-12852.php(BooleanNotConstantConditionRule) — unconditional assignment before condition in goto loop (correct always-true report, matching while-loop semantics)bug-14638.php(MissingReturnRule) — Symfony cache goto retry pattern with missing return false positiveCloses phpstan/phpstan#14641
Closes phpstan/phpstan#920
Closes phpstan/phpstan#14638
Closes phpstan/phpstan#4674
Closes phpstan/phpstan#7711
Closes phpstan/phpstan#7734
Closes phpstan/phpstan#9904
Closes phpstan/phpstan#11731
Closes phpstan/phpstan#12167