diff --git a/src/Analyser/ExprHandler/FuncCallHandler.php b/src/Analyser/ExprHandler/FuncCallHandler.php index 72f612fdabf..9e64e632345 100644 --- a/src/Analyser/ExprHandler/FuncCallHandler.php +++ b/src/Analyser/ExprHandler/FuncCallHandler.php @@ -132,7 +132,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex ) { // processed later } elseif ($parametersAcceptor instanceof CallableParametersAcceptor) { - $callableThrowPoints = array_map(static fn (SimpleThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $expr, $throwPoint->canContainAnyThrowable()) : InternalThrowPoint::createImplicit($scope, $expr), $parametersAcceptor->getThrowPoints()); + $callableThrowPoints = array_map(static fn (SimpleThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $expr, $throwPoint->canContainAnyThrowable(), $throwPoint->isFromThrowExpr()) : InternalThrowPoint::createImplicit($scope, $expr), $parametersAcceptor->getThrowPoints()); if (!$this->implicitThrows) { $callableThrowPoints = array_values(array_filter($callableThrowPoints, static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit())); } diff --git a/src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php b/src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php index cb8411fed35..51fb62e2332 100644 --- a/src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php +++ b/src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php @@ -908,7 +908,7 @@ private function assembleClosureType( ); } - $throwPointsForClosureType = array_map(static fn (ThrowPoint $throwPoint) => $throwPoint->isExplicit() ? SimpleThrowPoint::createExplicit($throwPoint->getType(), $throwPoint->canContainAnyThrowable()) : SimpleThrowPoint::createImplicit(), $throwPoints); + $throwPointsForClosureType = array_map(static fn (ThrowPoint $throwPoint) => $throwPoint->isExplicit() ? SimpleThrowPoint::createExplicit($throwPoint->getType(), $throwPoint->canContainAnyThrowable(), $throwPoint->isFromThrowExpr()) : SimpleThrowPoint::createImplicit(), $throwPoints); $impurePointsForClosureType = array_map(static fn (ImpurePoint $impurePoint) => new SimpleImpurePoint($impurePoint->getIdentifier(), $impurePoint->getDescription(), $impurePoint->isCertain()), $impurePoints); if ($writeCache) { diff --git a/src/Analyser/ExprHandler/ThrowHandler.php b/src/Analyser/ExprHandler/ThrowHandler.php index e9b1ce7d37a..28c12e7bce0 100644 --- a/src/Analyser/ExprHandler/ThrowHandler.php +++ b/src/Analyser/ExprHandler/ThrowHandler.php @@ -48,7 +48,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex expr: $expr, hasYield: false, isAlwaysTerminating: true, - throwPoints: array_merge($exprResult->getThrowPoints(), [InternalThrowPoint::createExplicit($scope, $exprResult->getType(), $expr, false)]), + throwPoints: array_merge($exprResult->getThrowPoints(), [InternalThrowPoint::createExplicit($scope, $exprResult->getType(), $expr, false, fromThrowExpr: true)]), impurePoints: $exprResult->getImpurePoints(), ); } diff --git a/src/Analyser/InternalThrowPoint.php b/src/Analyser/InternalThrowPoint.php index a50c4bd0427..45d424df975 100644 --- a/src/Analyser/InternalThrowPoint.php +++ b/src/Analyser/InternalThrowPoint.php @@ -20,6 +20,7 @@ private function __construct( private Node $node, private bool $explicit, private bool $canContainAnyThrowable, + private bool $fromThrowExpr = false, ) { } @@ -27,7 +28,7 @@ private function __construct( public function toPublic(): ThrowPoint { if ($this->explicit) { - return ThrowPoint::createExplicit($this->scope, $this->type, $this->node, $this->canContainAnyThrowable); + return ThrowPoint::createExplicit($this->scope, $this->type, $this->node, $this->canContainAnyThrowable, $this->fromThrowExpr); } return ThrowPoint::createImplicit($this->scope, $this->node); @@ -36,9 +37,9 @@ public function toPublic(): ThrowPoint /** * @param Node\Expr|Node\Stmt $node */ - public static function createExplicit(MutatingScope $scope, Type $type, Node $node, bool $canContainAnyThrowable): self + public static function createExplicit(MutatingScope $scope, Type $type, Node $node, bool $canContainAnyThrowable, bool $fromThrowExpr = false): self { - return new self($scope, $type, $node, true, $canContainAnyThrowable); + return new self($scope, $type, $node, true, $canContainAnyThrowable, $fromThrowExpr); } /** @@ -51,7 +52,7 @@ public static function createImplicit(MutatingScope $scope, Node $node): self public static function createFromPublic(ThrowPoint $throwPoint, MutatingScope $scope): self { - return new self($scope, $throwPoint->getType(), $throwPoint->getNode(), $throwPoint->isExplicit(), $throwPoint->canContainAnyThrowable()); + return new self($scope, $throwPoint->getType(), $throwPoint->getNode(), $throwPoint->isExplicit(), $throwPoint->canContainAnyThrowable(), $throwPoint->isFromThrowExpr()); } public function getScope(): MutatingScope @@ -82,9 +83,19 @@ public function canContainAnyThrowable(): bool return $this->canContainAnyThrowable; } + /** + * Whether the throw point comes from a `throw` written in the analysed code, + * as opposed to a throw inferred from what a called function or an operation + * can throw. + */ + public function isFromThrowExpr(): bool + { + return $this->fromThrowExpr; + } + public function subtractCatchType(Type $catchType): self { - return new self($this->scope, TypeCombinator::remove($this->type, $catchType), $this->node, $this->explicit, $this->canContainAnyThrowable); + return new self($this->scope, TypeCombinator::remove($this->type, $catchType), $this->node, $this->explicit, $this->canContainAnyThrowable, $this->fromThrowExpr); } } diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 059c3401333..f8747d712cf 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -2135,7 +2135,7 @@ public function processArgs( // still complete the gathered data - then it keeps re-walking) $this->container->getByType(ClosureTypeResolver::class)->seedCacheFromClosureWalk($scopeToPass, $arg->value, $closureResult); if ($this->callCallbackImmediately($parameter, $parameterType, $calleeReflection)) { - $throwPoints = array_merge($throwPoints, array_map(static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $arg->value, $throwPoint->canContainAnyThrowable()) : InternalThrowPoint::createImplicit($scope, $arg->value), $closureResult->getThrowPoints())); + $throwPoints = array_merge($throwPoints, array_map(static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $arg->value, $throwPoint->canContainAnyThrowable(), $throwPoint->isFromThrowExpr()) : InternalThrowPoint::createImplicit($scope, $arg->value), $closureResult->getThrowPoints())); $impurePoints = array_merge($impurePoints, $closureResult->getImpurePoints()); } @@ -2227,7 +2227,7 @@ public function processArgs( $this->container->getByType(ClosureTypeResolver::class)->seedCacheFromArrowFunctionWalk($scopeToPass, $arg->value, $processArrowFunctionResult); $arrowFunctionResult = $processArrowFunctionResult->getExpressionResult(); if ($this->callCallbackImmediately($parameter, $parameterType, $calleeReflection)) { - $throwPoints = array_merge($throwPoints, array_map(static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $arg->value, $throwPoint->canContainAnyThrowable()) : InternalThrowPoint::createImplicit($scope, $arg->value), $arrowFunctionResult->getThrowPoints())); + $throwPoints = array_merge($throwPoints, array_map(static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $arg->value, $throwPoint->canContainAnyThrowable(), $throwPoint->isFromThrowExpr()) : InternalThrowPoint::createImplicit($scope, $arg->value), $arrowFunctionResult->getThrowPoints())); $impurePoints = array_merge($impurePoints, $arrowFunctionResult->getImpurePoints()); } if ($this->shouldInvalidateCallbackExpressions($parameter)) { @@ -2264,7 +2264,7 @@ public function processArgs( $deferredInvalidateExpressions[] = [$acceptors[0]->getInvalidateExpressions(), $acceptors[0]->getUsedVariables()]; } if ($this->callCallbackImmediately($parameter, $parameterType, $calleeReflection)) { - $callableThrowPoints = array_map(static fn (SimpleThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $arg->value, $throwPoint->canContainAnyThrowable()) : InternalThrowPoint::createImplicit($scope, $arg->value), $acceptors[0]->getThrowPoints()); + $callableThrowPoints = array_map(static fn (SimpleThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $arg->value, $throwPoint->canContainAnyThrowable(), $throwPoint->isFromThrowExpr()) : InternalThrowPoint::createImplicit($scope, $arg->value), $acceptors[0]->getThrowPoints()); if (!$this->implicitThrows) { $callableThrowPoints = array_values(array_filter($callableThrowPoints, static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit())); } diff --git a/src/Analyser/StmtHandler/ExpressionHandler.php b/src/Analyser/StmtHandler/ExpressionHandler.php index 010f8715627..322fc22cfc3 100644 --- a/src/Analyser/StmtHandler/ExpressionHandler.php +++ b/src/Analyser/StmtHandler/ExpressionHandler.php @@ -11,6 +11,7 @@ use PHPStan\Analyser\ExpressionResultStorage; use PHPStan\Analyser\InternalStatementExitPoint; use PHPStan\Analyser\InternalStatementResult; +use PHPStan\Analyser\InternalThrowPoint; use PHPStan\Analyser\MutatingScope; use PHPStan\Analyser\NodeScopeResolver; use PHPStan\Analyser\Scope; @@ -77,9 +78,10 @@ public function processStmt( $nodeScopeResolver->callNodeCallback($nodeCallback, $stmt, $entryScope, $storage); // Errors signal programmer mistakes (ValueError, TypeError, DivisionByZeroError...), // nobody calls an otherwise pure expression just to have them thrown, so they - // do not make the expression statement meaningful. + // do not make the expression statement meaningful. A `throw` written in the + // statement is a different story - throwing is the whole point of it. $errorType = new ObjectType(Error::class); - $throwPoints = array_filter($result->getThrowPoints(), static fn ($throwPoint) => $throwPoint->isExplicit() && !$errorType->isSuperTypeOf($throwPoint->getType())->yes()); + $throwPoints = array_filter($result->getThrowPoints(), static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit() && ($throwPoint->isFromThrowExpr() || !$errorType->isSuperTypeOf($throwPoint->getType())->yes())); if ( count($result->getImpurePoints()) === 0 && count($throwPoints) === 0 diff --git a/src/Analyser/ThrowPoint.php b/src/Analyser/ThrowPoint.php index a3a2a4808a7..d8490017d30 100644 --- a/src/Analyser/ThrowPoint.php +++ b/src/Analyser/ThrowPoint.php @@ -23,6 +23,7 @@ private function __construct( private Node $node, private bool $explicit, private bool $canContainAnyThrowable, + private bool $fromThrowExpr = false, ) { } @@ -30,9 +31,9 @@ private function __construct( /** * @param Node\Expr|Node\Stmt $node */ - public static function createExplicit(Scope $scope, Type $type, Node $node, bool $canContainAnyThrowable): self + public static function createExplicit(Scope $scope, Type $type, Node $node, bool $canContainAnyThrowable, bool $fromThrowExpr = false): self { - return new self($scope, $type, $node, true, $canContainAnyThrowable); + return new self($scope, $type, $node, true, $canContainAnyThrowable, $fromThrowExpr); } /** @@ -71,9 +72,19 @@ public function canContainAnyThrowable(): bool return $this->canContainAnyThrowable; } + /** + * Whether the throw point comes from a `throw` written in the analysed code, + * as opposed to a throw inferred from what a called function or an operation + * can throw. + */ + public function isFromThrowExpr(): bool + { + return $this->fromThrowExpr; + } + public function subtractCatchType(Type $catchType): self { - return new self($this->scope, TypeCombinator::remove($this->type, $catchType), $this->node, $this->explicit, $this->canContainAnyThrowable); + return new self($this->scope, TypeCombinator::remove($this->type, $catchType), $this->node, $this->explicit, $this->canContainAnyThrowable, $this->fromThrowExpr); } } diff --git a/src/Reflection/Callables/SimpleThrowPoint.php b/src/Reflection/Callables/SimpleThrowPoint.php index 5ccd3ed18d1..f6ff23df4dc 100644 --- a/src/Reflection/Callables/SimpleThrowPoint.php +++ b/src/Reflection/Callables/SimpleThrowPoint.php @@ -24,13 +24,14 @@ private function __construct( private Type $type, private bool $explicit, private bool $canContainAnyThrowable, + private bool $fromThrowExpr = false, ) { } - public static function createExplicit(Type $type, bool $canContainAnyThrowable): self + public static function createExplicit(Type $type, bool $canContainAnyThrowable, bool $fromThrowExpr = false): self { - return new self($type, true, $canContainAnyThrowable); + return new self($type, true, $canContainAnyThrowable, $fromThrowExpr); } public static function createImplicit(): self @@ -53,4 +54,13 @@ public function canContainAnyThrowable(): bool return $this->canContainAnyThrowable; } + /** + * Whether the throw point comes from a `throw` written in the callable's body, + * as opposed to a throw only declared in PHPDoc. + */ + public function isFromThrowExpr(): bool + { + return $this->fromThrowExpr; + } + } diff --git a/tests/PHPStan/Rules/DeadCode/NoopRuleTest.php b/tests/PHPStan/Rules/DeadCode/NoopRuleTest.php index e79cc53a5db..5c7305ada2e 100644 --- a/tests/PHPStan/Rules/DeadCode/NoopRuleTest.php +++ b/tests/PHPStan/Rules/DeadCode/NoopRuleTest.php @@ -159,6 +159,12 @@ public function testErrorThrows(): void ]); } + #[RequiresPhp('>= 8.0.0')] + public function testBug15258(): void + { + $this->analyse([__DIR__ . '/data/bug-15258.php'], []); + } + public function testNullsafe(): void { $this->analyse([__DIR__ . '/data/nullsafe-property-fetch-noop.php'], [ diff --git a/tests/PHPStan/Rules/DeadCode/data/bug-15258.php b/tests/PHPStan/Rules/DeadCode/data/bug-15258.php new file mode 100644 index 00000000000..1139897a995 --- /dev/null +++ b/tests/PHPStan/Rules/DeadCode/data/bug-15258.php @@ -0,0 +1,18 @@ += 8.0 + +declare(strict_types = 1); + +namespace Bug15258; + +/** + * Given a string of the shape 'foo-bar', extract the prefix 'foo-' + * @phpstan-pure + * @param non-empty-string $x + * @return non-empty-string + */ +function extract_prefix(string $x): string { + $pos = strpos($x, '-'); + if ($pos === false) + throw new \ValueError('cannot extract a prefix from a string that has no prefix'); + return substr($x, 0, $pos + 1); +} diff --git a/tests/PHPStan/Rules/DeadCode/data/noop-error-throws.php b/tests/PHPStan/Rules/DeadCode/data/noop-error-throws.php index 3bd7e790000..8ab34a4879b 100644 --- a/tests/PHPStan/Rules/DeadCode/data/noop-error-throws.php +++ b/tests/PHPStan/Rules/DeadCode/data/noop-error-throws.php @@ -10,3 +10,60 @@ function (int $a, int $b, string $s) { }; new \DateTimeImmutable($s); }; + +function () { + throw new \ValueError('x'); +}; + +function (\Error $e) { + throw $e; +}; + +function (?int $n) { + $n ?? throw new \ValueError('x'); +}; + +function (bool $c) { + $c ? throw new \ValueError('x') : null; +}; + +function (?int $n) { + $n ?: throw new \ValueError('x'); +}; + +function (bool $c) { + $c && throw new \ValueError('x'); +}; + +function (bool $c) { + $c || throw new \ValueError('x'); +}; + +function (int $a) { + match (true) { + $a > 0 => throw new \ValueError('x'), + default => null, + }; +}; + +function () { + (function (): void { + throw new \ValueError('x'); + })(); +}; + +function () { + $fn = function (): void { + throw new \ValueError('x'); + }; + $fn(); +}; + +function () { + new class { + public function __construct() + { + throw new \ValueError('x'); + } + }; +}; diff --git a/tests/PHPStan/Rules/Functions/CallToFunctionStatementWithoutSideEffectsRuleTest.php b/tests/PHPStan/Rules/Functions/CallToFunctionStatementWithoutSideEffectsRuleTest.php index f81bbcf3291..ab196bcc32e 100644 --- a/tests/PHPStan/Rules/Functions/CallToFunctionStatementWithoutSideEffectsRuleTest.php +++ b/tests/PHPStan/Rules/Functions/CallToFunctionStatementWithoutSideEffectsRuleTest.php @@ -139,6 +139,17 @@ public function testErrorThrows(): void ]); } + #[RequiresPhp('>= 8.0.0')] + public function testThrowExprInCallbackArgument(): void + { + $this->analyse([__DIR__ . '/data/function-call-statement-no-side-effects-throw-expr.php'], [ + [ + 'Call to function array_map() on a separate line has no effect.', + 21, + ], + ]); + } + public function testBug4455(): void { require_once __DIR__ . '/data/bug-4455.php'; diff --git a/tests/PHPStan/Rules/Functions/data/function-call-statement-no-side-effects-throw-expr.php b/tests/PHPStan/Rules/Functions/data/function-call-statement-no-side-effects-throw-expr.php new file mode 100644 index 00000000000..8220896a0ac --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/function-call-statement-no-side-effects-throw-expr.php @@ -0,0 +1,24 @@ += 8.0 + +namespace FunctionCallStatementNoSideEffectsThrowExpr; + +class Foo +{ + + /** + * @param list $a + */ + public function doFoo(array $a): void + { + array_map(static fn (int $i) => throw new \ValueError('x'), $a); + array_map(static function (int $i): int { + throw new \ValueError('x'); + }, $a); + $callback = static function (int $i): int { + throw new \ValueError('x'); + }; + array_map($callback, $a); + array_map(static fn (int $i) => $i * 2, $a); + } + +}