Skip to content

Fix phpstan/phpstan#9349: Throw point are not properly recognized #5166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
VincentLanglet merged 3 commits into from
Mar 17, 2026
Merged

Fix phpstan/phpstan#9349: Throw point are not properly recognized #5166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6073f0
Fix throw points not properly matched to catch clauses
ondrejmirtes Mar 9, 2026
6539a4a
Separate second test case into its own class in bug-9349 test
phpstan-bot Mar 17, 2026
c459510
Add test case with union @throws annotation in bug-9349 test
phpstan-bot Mar 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Analyser/NodeScopeResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1878,6 +1878,7 @@ public function processStmtNode(

// explicit only
$onlyExplicitIsThrow = true;
$hasDirectExplicitNonThrowMatch = false;
if (count($matchingThrowPoints) === 0) {
foreach ($throwPoints as $throwPointIndex => $throwPoint) {
foreach ($catchTypes as $catchTypeIndex => $catchTypeItem) {
Expand All @@ -1895,14 +1896,17 @@ public function processStmtNode(
&& !($throwNode instanceof Node\Stmt\Expression && $throwNode->expr instanceof Expr\Throw_)
) {
$onlyExplicitIsThrow = false;
if ($catchTypeItem->isSuperTypeOf($throwPoint->getType())->yes()) {
$hasDirectExplicitNonThrowMatch = true;
}
}
$matchingThrowPoints[$throwPointIndex] = $throwPoint;
}
}
}

// implicit only
if (count($matchingThrowPoints) === 0 || $onlyExplicitIsThrow) {
if (count($matchingThrowPoints) === 0 || $onlyExplicitIsThrow || !$hasDirectExplicitNonThrowMatch) {
foreach ($throwPoints as $throwPointIndex => $throwPoint) {
if ($throwPoint->isExplicit()) {
continue;
Expand Down
19 changes: 19 additions & 0 deletions tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1403,6 +1403,25 @@ public function testBug14019(): void
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14019.php'], []);
}

public function testBug9349(): void
{
$this->cliArgumentsVariablesRegistered = true;
$this->polluteScopeWithLoopInitialAssignments = false;
$this->checkMaybeUndefinedVariables = true;
$this->polluteScopeWithAlwaysIterableForeach = true;

$this->analyse([__DIR__ . '/data/bug-9349.php'], [
[
'Variable $sql might not be defined.',
19,
],
[
'Variable $sql might not be defined.',
78,
],
]);
}

#[RequiresPhp('>= 8.0')]
public function testBug14274(): void
{
Expand Down
94 changes: 94 additions & 0 deletions tests/PHPStan/Rules/Variables/data/bug-9349.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
<?php declare(strict_types = 1);

namespace Bug9349;

class HelloWorld
{
public function test(): void
{
global $pdo;

try {
$this->maybeThrows();
$sql = "SELECT * FROM foo";
$rs = $pdo->query($sql);
if ($result = $rs->fetch(\PDO::FETCH_ASSOC)) {
// do something
}
} catch (\PDOException $e) {
var_dump($sql);
Copy link
Copy Markdown
Contributor

@staabm staabm Mar 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I think that PHPStan should know that $sql will always exist at this point.

atm the test asserts

Variable $sql might not be defined.

Copy link
Copy Markdown
Contributor

@staabm staabm Mar 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

ohh I now see that PDOException extends RuntimeException - its fine than.

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot Mar 17, 2026

Choose a reason for hiding this comment

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

No code changes are needed here. The reviewer (staabm) is retracting their earlier concern about $sql always being defined at line 19. They now understand that since PDOException extends RuntimeException, the @throws \RuntimeException annotation on maybeThrows() means a PDOException could come from that call (before $sql is assigned), so "Variable $sql might not be defined" is the correct behavior.

All review comments on this PR are resolved — no actionable changes remain.

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot Mar 17, 2026

Choose a reason for hiding this comment

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

All background tasks are now complete. The final make tests run passed (11,578 tests, 0 failures, 92 skipped) and both make phpstan runs passed with no errors. The branch is clean and up to date with the remote.

}
}

/**
* @throws \RuntimeException
*/
public function maybeThrows(): void
{
if (random_int(0, 1) === 1) {
throw new \RuntimeException();
}
}

}

class HelloWorld2
{
public function test2(): void
{
global $pdo;

try {
$this->maybeThrows2();
$sql = "SELECT * FROM foo";
$rs = $pdo->query($sql);
if ($result = $rs->fetch(\PDO::FETCH_ASSOC)) {
// do something
}
} catch (\PDOException $e) {
var_dump($sql);
}
}

/**
* @throws \LogicException
Copy link
Copy Markdown
Contributor

@staabm staabm Mar 17, 2026

Choose a reason for hiding this comment

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

I think it would be good to have a 3rd test-case in which the @throws is defined with a union like @throws \LogicException|\RuntimeException

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot Mar 17, 2026

Choose a reason for hiding this comment

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

Already retrieved the results - all 11,578 tests passed. Nothing more to do.

*/
public function maybeThrows2(): void
{
if (random_int(0, 1) === 1) {
throw new \LogicException();
}
}
Comment on lines +37 to +61
Copy link
Copy Markdown
Contributor

@staabm staabm Mar 17, 2026

Choose a reason for hiding this comment

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

I would separate this second test-case into a second class, e.g. HelloWorld2

Copy link
Copy Markdown
Contributor

@VincentLanglet VincentLanglet Mar 17, 2026

Choose a reason for hiding this comment

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

Out of curiosity, why ? Different method should be enough no ?

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot Mar 17, 2026

Choose a reason for hiding this comment

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

VincentLanglet's current review comment is a discussion question directed at staabm ("Out of curiosity, why? Different method should be enough no?"), not a code change request. VincentLanglet is agreeing that the current structure (separate methods in the same class) is fine and questioning staabm's suggestion to create a separate HelloWorld2 class.

There are no actionable code changes requested in this review. The two earlier review points from staabm were:

  1. Separate into HelloWorld2 class — VincentLanglet disagrees with this, saying different methods are sufficient.
  2. Whether $sql should always be defined at line 19 — This is a valid observation (since PDOException can only come from $pdo->query() after $sql is assigned), but it relates to how PHPStan handles implicit throw points. The implicit throw point from $this->maybeThrows() (before $sql assignment) is included because any method call could implicitly throw any exception regardless of @throws annotations. So "might not be defined" is correct behavior — it's an improvement over the previous "Undefined variable" and would be a separate enhancement to make it even smarter.

No code changes are needed for VincentLanglet's comment. Would you like me to respond to the PR review comment on GitHub instead, or would you like me to address staabm's suggestions?

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot Mar 17, 2026

Choose a reason for hiding this comment

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

Already handled — all tests passed and the commit has been pushed.

Copy link
Copy Markdown
Contributor

@staabm staabm Mar 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Out of curiosity, why ? Different method should be enough no ?

just because its easier to see that method1,2 do not have an effect on method 3,4 - and vice versa

Copy link
Copy Markdown
Collaborator Author

@phpstan-bot phpstan-bot Mar 17, 2026

Choose a reason for hiding this comment

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

The working tree is clean and up to date with the remote. All of staabm's review comments have already been addressed in previous commits:

  1. Separate test cases into separate classes - Done in commit 6539a4a01
  2. Add a third test case with union @throws - Done in commit c459510e2

No further changes are needed. The branch is already pushed to the remote.

}

class HelloWorld3
{
public function test3(): void
{
global $pdo;

try {
$this->maybeThrows3();
$sql = "SELECT * FROM foo";
$rs = $pdo->query($sql);
if ($result = $rs->fetch(\PDO::FETCH_ASSOC)) {
// do something
}
} catch (\PDOException $e) {
var_dump($sql);
}
}

/**
* @throws \LogicException|\RuntimeException
*/
public function maybeThrows3(): void
{
if (random_int(0, 1) === 1) {
throw new \RuntimeException();
}
if (random_int(0, 1) === 1) {
throw new \LogicException();
}
}
}
Loading