Skip to content

Respect @throws void on getIterator() when determining foreach Traversable throw points#5666

Merged
VincentLanglet merged 8 commits into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-t0v6vom
May 15, 2026
Merged

Respect @throws void on getIterator() when determining foreach Traversable throw points#5666
VincentLanglet merged 8 commits into
phpstan:2.1.xfrom
phpstan-bot:create-pull-request/patch-t0v6vom

Conversation

@phpstan-bot
Copy link
Copy Markdown
Collaborator

Summary

When iterating over an IteratorAggregate with foreach inside a try block, PHPStan adds an implicit throw point representing the possibility that the iteration mechanism (particularly getIterator()) could throw. This throw point uses the pre-foreach scope (without the loop variable), causing the loop variable to be reported as "might not be defined" in catch blocks.

The issue is that this implicit throw point was added unconditionally — it ignored both @throws void annotations on getIterator() and the exceptions.implicitThrows configuration parameter. This meant users had no way to suppress the false positive even when they explicitly annotated getIterator() with @throws void.

Changes

  • Extracted a new getTraversableForeachThrowPoint() method in src/Analyser/NodeScopeResolver.php that replaces the inline implicit throw point creation for foreach over Traversable types
  • For IteratorAggregate types, the method checks getIterator()'s @throws annotation:
    • @throws void → returns null (no throw point, iteration init cannot throw)
    • Explicit @throws SomeException → returns an explicit throw point with that type
    • No annotation → falls through to implicit throw point handling
  • The method also respects the implicitThrows configuration parameter, which was previously bypassed for the foreach Traversable throw point (unlike all other implicit throw points in NodeScopeResolver, MethodThrowPointHelper, and FuncCallHandler)
  • Added use IteratorAggregate; import

Root cause

The foreach Traversable implicit throw point at line 1440-1442 was added unconditionally:

if (!(new ObjectType(Traversable::class))->isSuperTypeOf($scope->getType($stmt->expr))->no()) {
    $throwPoints[] = InternalThrowPoint::createImplicit($scope, $stmt->expr);
}

This pattern differs from how all other method call throw points work (via MethodThrowPointHelper), which check @throws annotations and the implicitThrows config. The fix brings the foreach Traversable throw point in line with the established pattern.

Test

Rule test (tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php::testBug6833)

Test data file tests/PHPStan/Rules/Variables/data/bug-6833.php covers:

  1. @throws void on getIterator() — no "might not be defined" error (the fix)
  2. No @throws annotation — "might not be defined" error (correct, getIterator() could throw)
  3. @throws \RuntimeException matching \Throwable catch — error (correct, explicit throw type matches)
  4. @throws \RuntimeException with \LogicException catch — no error (correct, throw type doesn't match catch)
  5. Array foreach — no error (correct, arrays don't call getIterator())

NSRT test (tests/PHPStan/Analyser/nsrt/bug-6833.php)

Verifies variable certainty directly with assertVariableCertainty():

  • @throws voidTrinaryLogic::createYes() in catch scope
  • No annotation → TrinaryLogic::createMaybe() in catch scope
  • Explicit throws → TrinaryLogic::createMaybe() in catch scope
  • Array foreach → TrinaryLogic::createYes() in catch scope

Fixes phpstan/phpstan#6833

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge the two bug-6833 fixtures files into a single one

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test for array|IteratorAggregate where IteratorAggregate throws void or throws exception ; since iterating an array does not produce exception it should only be impacted by IteratorAggregate throws type.

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Done. Merged the two separate bug-6833 test fixture files (tests/PHPStan/Analyser/nsrt/bug-6833.php and tests/PHPStan/Rules/Variables/data/bug-6833.php) into a single file at tests/PHPStan/Rules/Variables/data/bug-6833.php. The merged file contains both the assertVariableCertainty calls (for NSRT testing) and the rule test scenarios. The NSRT file was deleted and NodeScopeResolverTest now yields the merged file directly. All tests and static analysis pass.

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Pushed. Here's what was done to address VincentLanglet's review:

Implementation fix (src/Analyser/NodeScopeResolver.php): The getTraversableForeachThrowPoint() method now decomposes union types and checks each member individually. Non-Traversable members (like array) are skipped entirely. Each IteratorAggregate member has its getIterator() @throws annotation checked independently. This was needed because the previous ->yes() check on the whole type would fail for unions like array|IteratorAggregate (returning maybe), causing @throws void to be ignored.

Tests added (3 new cases in tests/PHPStan/Rules/Variables/data/bug-6833.php):

  1. array|FileCollection (with @throws void) — no error, variable is definitely defined
  2. array|FileCollectionExplicitThrows (with @throws RuntimeException) — error, variable might not be defined
  3. array|FileCollectionWithoutThrowsVoid (no annotation) — error, variable might not be defined

Each test includes both assertVariableCertainty() and rule error expectations.

Comment thread src/Analyser/NodeScopeResolver.php Outdated
Comment thread src/Analyser/NodeScopeResolver.php Outdated
Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solve

FILE: tests/PHPStan/Analyser/NodeScopeResolverTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 288 | ERROR | [x] Use early exit to reduce code nesting.
     |       |     (SlevomatCodingStandard.ControlStructures.EarlyExit.EarlyExitNotUsed)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

@phpstan-bot
Copy link
Copy Markdown
Collaborator Author

Done. Replaced the if (PHP_VERSION_ID >= 80000) { yield ... } with an early return if (PHP_VERSION_ID < 80000) { return; } followed by the unconditional yield, fixing the SlevomatCodingStandard.ControlStructures.EarlyExit violation. All tests, static analysis, and coding standards pass.

Comment thread tests/PHPStan/Analyser/NodeScopeResolverTest.php Outdated
@VincentLanglet VincentLanglet requested a review from staabm May 15, 2026 12:04
@VincentLanglet
Copy link
Copy Markdown
Contributor

Please check again with the new changes @staabm

VincentLanglet and others added 8 commits May 15, 2026 14:33
…aversable throw points

- Extract `getTraversableForeachThrowPoint()` from inline implicit throw point
  creation in the `Foreach_` handler of `NodeScopeResolver`
- For `IteratorAggregate` types, check `getIterator()` throw type:
  - `@throws void` → no throw point (iteration init cannot throw)
  - Explicit `@throws SomeException` → explicit throw point with that type
  - No annotation → fall through to implicit throw point
- Also respect the `implicitThrows` config parameter, which was previously
  ignored for the foreach Traversable throw point (unlike all other implicit
  throw points in the codebase)
- Probed analogous cases: direct `Iterator` types, union types, array foreach,
  `@throws` with non-matching catch types — all behave correctly
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ection

Decompose union types to check each member individually. Non-Traversable
members (like arrays) are skipped. IteratorAggregate members have their
getIterator() @throws annotation checked. This ensures @throws void is
respected even when the iteratee is a union like array|IteratorAggregate.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…each throw point detection

Replace manual union decomposition (`$exprType instanceof UnionType ? $exprType->getTypes() : [...]`)
with `TypeCombinator::intersect($exprType, $traversableType)` to extract the Traversable part,
then check if it's entirely IteratorAggregate via `isSuperTypeOf`. This follows the codebase
convention of never using `instanceof` to check types and lets the type system handle
union/intersection decomposition internally.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The implicitThrows config should only gate the no-annotation case for
IteratorAggregate types. For non-IteratorAggregate Traversables, the
implicit throw point should always be added since there is no
getIterator() to inspect.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test file belongs in tests/PHPStan/Analyser/nsrt/ since it contains
assertVariableCertainty() calls. The nsrt directory auto-discovers test
files, removing the need for the manual yield and PHP version guard in
NodeScopeResolverTest. The DefinedVariableRuleTest path is updated to
reference the new location.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@staabm staabm force-pushed the create-pull-request/patch-t0v6vom branch from 880195f to b2409b8 Compare May 15, 2026 12:33
@VincentLanglet VincentLanglet merged commit 3b4e3e7 into phpstan:2.1.x May 15, 2026
656 of 660 checks passed
@VincentLanglet VincentLanglet deleted the create-pull-request/patch-t0v6vom branch May 15, 2026 12:58
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.

3 participants