Fix phpstan/phpstan#5952: Allow throwing exceptions from __toString() not detected since level 4#5405
Conversation
- Added __toString() throw point tracking to echo statement handling in NodeScopeResolver - Reuses existing CastStringHandler pattern: checks PhpVersion::throwsOnStringCast() and MethodThrowPointHelper - New regression test in tests/PHPStan/Rules/Exceptions/data/bug-5952.php - The root cause was that echo $obj did not consider implicit __toString() calls when collecting throw points
VincentLanglet
left a comment
There was a problem hiding this comment.
Rather than calling directly MethodThrowPointHelper can't we simulate a MethodCall ?
Address review feedback: process a synthetic MethodCall expression through processExprNode() instead of directly calling MethodThrowPointHelper. This leverages the existing MethodCallHandler infrastructure for __toString() throw point tracking in echo statements. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
VincentLanglet
left a comment
There was a problem hiding this comment.
Remove the phpversion check, it's already done in the toString resolution.
Also add a test for echo on an union
- 123|Foo
- 123|Bar
- 123|Baz
The phpVersion->throwsOnStringCast() check is unnecessary because the toString resolution already handles the PHP version gate internally. Also adds regression tests for echo on union types (int|Foo, int|Bar, int|Baz) as requested in review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Both background tasks have already been handled — |
VincentLanglet
left a comment
There was a problem hiding this comment.
This fix for echo but I assume the same issue could occurs for some other situation where __toStirng is implicitely called. Is there a general fix ?
Extract ToStringThrowPointHelper to handle __toString() throw points
and impure points in a reusable way, and apply it to all places where
PHP implicitly calls __toString():
- echo (NodeScopeResolver)
- print (PrintHandler)
- string concatenation . (BinaryOpHandler)
- concat assignment .= (AssignOpHandler)
- string interpolation "$obj" / "{$obj}" (InterpolatedStringHandler)
- explicit (string) cast (CastStringHandler) - refactored to use helper
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All clear - every background task has completed successfully. The review feedback has been fully addressed and pushed. |
…urn ExpressionResult
Rename the helper to better describe its purpose and change the return type
from array{list<InternalThrowPoint>, list<ImpurePoint>} to ExpressionResult
for a cleaner API at all call sites.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. The changes address VincentLanglet's review:
All call sites updated, PHPStan passes clean, regression test still passes. |
VincentLanglet
left a comment
There was a problem hiding this comment.
Fix the lint errors
Class \PHPStan\Analyser\ExprHandler\Helper\ImplicitToStringCallHelper should not be referenced via a fully qualified name, but via a use statement.
…qualified name Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Already handled — tests passed, commit pushed. Nothing more to do. |
|
|
||
| $toStringResult = $this->implicitToStringCallHelper->processImplicitToStringCall($expr->expr, $scope); | ||
| $throwPoints = array_merge($throwPoints, $toStringResult->getThrowPoints()); | ||
| $impurePoints = array_merge($impurePoints, $toStringResult->getImpurePoints()); |
There was a problem hiding this comment.
this PR is not only about throw points, but also impure points.
it seems the impurity portion does not have tests
There was a problem hiding this comment.
Both background tasks confirmed complete. Everything was already pushed — nothing more needed.
…add purity tests The echo statement handler in NodeScopeResolver was not collecting impure points from implicit __toString() calls or from sub-expression processing. Also adds purity tests for all implicit __toString() call sites: echo, print, string concatenation, concat assignment, and string interpolation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| @@ -861,19 +863,22 @@ public function processStmtNode( | |||
| } elseif ($stmt instanceof Echo_) { | |||
There was a problem hiding this comment.
do we need more changes in NodeScopeResolver to reflect the changes made in ExprHandler classes with this PR?
There was a problem hiding this comment.
All 11732 tests pass.
Summary: After analyzing the codebase, no additional changes are needed in NodeScopeResolver.php. Here's why:
- The echo handler (line 872) already correctly uses
ImplicitToStringCallHelper— this was added in this PR - All other implicit
__toString()call sites are expressions (not statements), so they're handled by their respective ExprHandler classes:PrintHandler—print $objCastStringHandler—(string) $objBinaryOpHandler—'hello' . $objAssignOpHandler—$x .= $objInterpolatedStringHandler—"hello $obj"
The ImplicitToStringCallHelper is injected into NodeScopeResolver solely for the echo statement, since echo is a Stmt (not an Expr) and cannot have its own ExprHandler.
I was unable to post this reply as a PR comment due to GitHub token permissions. You may want to reply to staabm's comment manually with this explanation.
… not detected since level 4 (phpstan#5405) Co-authored-by: VincentLanglet <9052536+VincentLanglet@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
When
echo $objis used where$objhas a__toString()method that can throw, PHPStan incorrectly reported "Dead catch - Exception is never thrown in the try block." This is because the echo statement handler inNodeScopeResolverdid not track throw points from implicit__toString()calls.Changes
__toString()throw point tracking to the echo statement handler insrc/Analyser/NodeScopeResolver.phpCastStringHandler(explicit(string)cast): checksPhpVersion::throwsOnStringCast()(PHP >= 7.4) and usesMethodThrowPointHelper::getThrowPoint()to determine if the__toString()method can throwtests/PHPStan/Rules/Exceptions/data/bug-5952.phpRoot cause
The
echostatement handler inNodeScopeResolvercollected throw points from expression processing but did not account for PHP's implicit__toString()call when echoing an object. The explicit string cast handler (CastStringHandler) already had this logic, but it was not applied toechostatements.Test
The regression test covers:
echo $objwhere__toString()throws (implicitly via@throws voidabsence) - no dead catch errorecho $objwhere__toString()has@throws \Exception- no dead catch errorecho $objwhere__toString()has@throws void- dead catch correctly reportedecho 123(non-object) - dead catch correctly reportedFixes phpstan/phpstan#5952