Fix phpstan/phpstan#13806: PHPStan assumes string conversions cannot throw exceptions#5391
Conversation
VincentLanglet
left a comment
There was a problem hiding this comment.
This should be dependant on phpVersion
public function throwsOnStringCast(): bool
{
return $this->versionId >= 70400;
}
|
Both background tasks (static analysis and tests) also completed successfully. All checks are green and the changes have been pushed. |
VincentLanglet
left a comment
There was a problem hiding this comment.
Add test when casting a non object, like int and ensure the useless throw is reported
|
Background |
| } | ||
|
|
||
| class MyString { | ||
| public function __toString(): never { |
There was a problem hiding this comment.
Remove the native return type to avoid lint failure
Bug13806\MyString::__toString(): Return type must be string when declared
There was a problem hiding this comment.
All checks passed:
make phpstan: No errorsmake tests: 11722 tests, 78963 assertions, all OK
The commit has been pushed to the branch.
VincentLanglet
left a comment
There was a problem hiding this comment.
Add test with
- A
@throws Exception - A
@throws void
|
Background |
| if ($this->phpVersion->throwsOnStringCast()) { | ||
| $throwType = $toStringMethod->getThrowType(); | ||
| if ($throwType === null) { | ||
| $returnType = $toStringMethod->getVariants()[0]->getReturnType(); |
There was a problem hiding this comment.
should this use getOnlyVariant() instead of getVariants()[0]?
There was a problem hiding this comment.
Background make tests also completed successfully. All done — both checks passed and the commit addressing staabm's review has been pushed.
| private InitializerExprTypeResolver $initializerExprTypeResolver, | ||
| private PhpVersion $phpVersion, | ||
| #[AutowiredParameter(ref: '%exceptions.implicitThrows%')] | ||
| private bool $implicitThrows, |
There was a problem hiding this comment.
do we need separate tests for this bool beeing true/false?
There was a problem hiding this comment.
Background make tests also completed successfully. All tasks are done — both review comments have been addressed and pushed.
|
ondrej mentioned |
Indeed cause someone could write a dynamic return type extension for the toStringMethod. |
| } | ||
|
|
||
| if ($this->phpVersion->throwsOnStringCast()) { | ||
| $throwType = $toStringMethod->getThrowType(); |
There was a problem hiding this comment.
This code is similar to MethodCallHandler::getMethodThrowPoint and also have the benefit of supporting the dynamicReturnTypeExtension.
You should look for a refactoring.
Maybe introduce a MethodCallHelper similar to the MethodCallReturnTypeHelper or just rename it MethodCallHelper.
It could also maybe be refactored with StaticCallHandler::getStaticMethodThrowPoint
There was a problem hiding this comment.
All background tasks have completed successfully. The refactoring is done and pushed.
… __toString() - Added throw point creation in CastStringHandler when __toString() method exists - Respects explicit throw types from __toString() and implicit throws configuration - New regression test in tests/PHPStan/Rules/Exceptions/data/bug-13806.php - The root cause was that CastStringHandler tracked impurity but not throw points for __toString()
__toString() can only throw exceptions since PHP 7.4. Add PhpVersion::throwsOnStringCast() and gate the throw point creation in CastStringHandler on it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ensures that casting a non-object type (int) to string correctly reports the catch block as dead code, since no __toString() method is involved and no exception can be thrown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The native `never` return type on `__toString()` causes a lint error since PHP requires `__toString()` to declare `string` as its return type. Use a `@return never` PHPDoc instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
__toString() always has exactly one variant, so getOnlyVariant() is the correct API to use here. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address review feedback: - Use getOnlyVariant() instead of getVariants()[0] for __toString() - Add test in AbilityToDisableImplicitThrowsTest to verify string cast throw points behave correctly when implicitThrows is disabled Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
32e6017 to
9fac56e
Compare
Refactor duplicate throw point resolution from CastStringHandler, MethodCallHandler, and StaticCallHandler into a shared helper class. This also fixes StaticCallHandler missing the never-return-type fallback to Throwable that MethodCallHandler already had. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9fac56e to
d898b9a
Compare
VincentLanglet
left a comment
There was a problem hiding this comment.
Fix
FILE: src/Analyser/ExprHandler/CastStringHandler.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
67 | ERROR | [x] Multi-line function calls must have a trailing comma after
| | the last parameter.
| | (SlevomatCodingStandard.Functions.RequireTrailingCommaInCall.MissingTrailingComma)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
FILE: src/Analyser/ExprHandler/Helper/MethodThrowPointHelper.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
69 | ERROR | [x] Function in_array() should not be referenced via a fallback
| | global name, but via a use statement.
| | (SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly.ReferenceViaFallbackGlobalName)
70 | ERROR | [x] Function in_array() should not be referenced via a fallback
| | global name, but via a use statement.
| | (SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly.ReferenceViaFallbackGlobalName)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
…intHelper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Both background tasks confirmed complete. All changes are already pushed. |
Summary
PHPStan incorrectly reported "Dead catch - Throwable is never thrown in the try block" for try/catch blocks wrapping string casts
(string) $variable. Since PHP 7.4,__toString()can throw exceptions, so string casts on objects with__toString()should create throw points.Changes
src/Analyser/ExprHandler/CastStringHandler.phpto create throw points when a__toString()method is detected on the cast expression's typeimplicitThrowsconstructor parameter (autowired from%exceptions.implicitThrows%config)__toString()has a declared throw type or returnsneverimplicitThrowsis enabled and no explicit throw type is declaredtests/PHPStan/Rules/Exceptions/data/bug-13806.phpRoot cause
CastStringHandleralready detected__toString()methods to track impurity (side effects), but it never created throw points for the potential exceptions. This meant the try/catch dead-code analysis saw no throw points from string casts and incorrectly reported catch blocks as dead code.The fix mirrors the throw point logic from
MethodCallHandler::getMethodThrowPoint(): check for explicit throw types first, then fall back to implicit throws if configured.Test
Added
testBug13806inCatchWithUnthrownExceptionRuleTestwith a test case that casts aStringable|stringvariable to string inside a try block with a catch for\Throwable. The test expects no errors (the catch should not be reported as dead).Fixes phpstan/phpstan#13806
Closes #4555