diff --git a/conf/config.neon b/conf/config.neon index 5174d60f881..6cb38f0f620 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -109,6 +109,7 @@ parameters: reportPossiblyNonexistentGeneralArrayOffset: false reportPossiblyNonexistentConstantArrayOffset: false checkMissingOverrideMethodAttribute: false + checkMissingPurePhpDocTag: false checkMissingOverridePropertyAttribute: null mixinExcludeClasses: [] scanFiles: [] diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index fbf06496f89..05a1830bb44 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -113,6 +113,7 @@ parametersSchema: reportPossiblyNonexistentGeneralArrayOffset: bool() reportPossiblyNonexistentConstantArrayOffset: bool() checkMissingOverrideMethodAttribute: bool() + checkMissingPurePhpDocTag: bool() checkMissingOverridePropertyAttribute: schema(bool(), nullable()) parallel: structure([ jobSize: int(), diff --git a/src/Node/FunctionReturnStatementsNode.php b/src/Node/FunctionReturnStatementsNode.php index 3403425697a..0880f432f5e 100644 --- a/src/Node/FunctionReturnStatementsNode.php +++ b/src/Node/FunctionReturnStatementsNode.php @@ -102,6 +102,11 @@ public function getSubNodeNames(): array return []; } + public function getFunction(): Function_ + { + return $this->function; + } + public function getFunctionReflection(): PhpFunctionFromParserNodeReflection { return $this->functionReflection; diff --git a/src/Node/MethodReturnStatementsNode.php b/src/Node/MethodReturnStatementsNode.php index 620e271dc36..47fe850d59b 100644 --- a/src/Node/MethodReturnStatementsNode.php +++ b/src/Node/MethodReturnStatementsNode.php @@ -92,6 +92,11 @@ public function getYieldStatements(): array return $this->yieldStatements; } + public function getClassMethod(): ClassMethod + { + return $this->classMethod; + } + public function getClassReflection(): ClassReflection { return $this->classReflection; diff --git a/src/Rules/Pure/FunctionPurityCheck.php b/src/Rules/Pure/FunctionPurityCheck.php index 94d265f6732..08257afb170 100644 --- a/src/Rules/Pure/FunctionPurityCheck.php +++ b/src/Rules/Pure/FunctionPurityCheck.php @@ -2,6 +2,7 @@ namespace PHPStan\Rules\Pure; +use PhpParser\Comment\Doc; use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\Variable; use PhpParser\Node\Name; @@ -10,12 +11,19 @@ use PHPStan\Analyser\Scope; use PHPStan\Analyser\ThrowPoint; use PHPStan\DependencyInjection\AutowiredService; +use PHPStan\Fixable\PhpDoc\PhpDocEditor; +use PHPStan\PhpDocParser\Ast\Node as PhpDocAstNode; +use PHPStan\PhpDocParser\Ast\PhpDoc\GenericTagValueNode; +use PHPStan\PhpDocParser\Ast\PhpDoc\PhpDocNode; +use PHPStan\PhpDocParser\Ast\PhpDoc\PhpDocTagNode; use PHPStan\Reflection\ExtendedMethodReflection; use PHPStan\Reflection\ExtendedParameterReflection; use PHPStan\Reflection\FunctionReflection; +use PHPStan\Reflection\PassedByReference; use PHPStan\Rules\Functions\CallToFunctionStatementWithoutSideEffectsRule; use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\RuleErrorBuilder; +use PHPStan\TrinaryLogic; use PHPStan\Type\Type; use function array_filter; use function array_key_exists; @@ -25,11 +33,18 @@ use function is_string; use function lcfirst; use function sprintf; +use function str_contains; +use function substr; +use function trim; #[AutowiredService] final class FunctionPurityCheck { + public function __construct(private PhpDocEditor $phpDocEditor) + { + } + /** * @param 'Function'|'Method'|'PropertyHook' $identifier * @param ExtendedParameterReflection[] $parameters @@ -226,6 +241,117 @@ public function check( return $errors; } + /** + * Whether a function without a purity tag could get @phpstan-pure without + * any error from check(), and without changing what calling it as a + * statement means. Overridable methods are left out, because the tag + * would bind the overriding methods too. + * + * @param ExtendedParameterReflection[] $parameters + * @param ImpurePoint[] $impurePoints + * @param ThrowPoint[] $throwPoints + */ + public function canBeMarkedPure( + FunctionReflection|ExtendedMethodReflection $functionReflection, + array $parameters, + Type $returnType, + array $impurePoints, + array $throwPoints, + bool $isGenerator, + bool $returnsByRef, + bool $isConstructor, + ): bool + { + if (!$functionReflection->isPure()->maybe()) { + return false; + } + + if ( + $functionReflection->getPureUnlessCallableIsImpureParameters() !== [] + || $functionReflection->getPureUnlessParameterPassedParameters() !== [] + ) { + return false; + } + + // a caller can write through a returned reference + if (!$returnType->isVoid()->no() || $isGenerator || $returnsByRef) { + return false; + } + + if (count($impurePoints) !== 0) { + return false; + } + + // a function that throws on purpose may be called only for that, and + // a call to a pure function as a statement is reported as useless; + // implicit throw points come from every call and do not count + foreach ($throwPoints as $throwPoint) { + if ($throwPoint->isExplicit()) { + return false; + } + } + + $throwType = $functionReflection->getThrowType(); + if ($throwType !== null && !$throwType->isVoid()->yes()) { + return false; + } + + if (count($functionReflection->getAsserts()->getAll()) !== 0) { + return false; + } + + foreach ($parameters as $parameter) { + if (!$parameter->passedByReference()->equals(PassedByReference::createNo())) { + return false; + } + } + + if ($isConstructor) { + return false; + } + + if ($functionReflection instanceof ExtendedMethodReflection) { + return $functionReflection->isPrivate() + || $functionReflection->isFinal()->equals(TrinaryLogic::createYes()) + || $functionReflection->getDeclaringClass()->isFinal(); + } + + return true; + } + + /** + * @param 'Function'|'Method' $identifier + */ + public function createMissingPureTagError(string $functionDescription, string $identifier, Stmt\Function_|Stmt\ClassMethod $node): IdentifierRuleError + { + return RuleErrorBuilder::message(sprintf( + '%s does not have any side effects and can be marked @phpstan-pure.', + $functionDescription, + )) + ->identifier(sprintf('pure%s.missing', $identifier)) + ->fixNode($node, function (Stmt\Function_|Stmt\ClassMethod $node): Stmt\Function_|Stmt\ClassMethod { + // a tag appended to a single-line PHPDoc would stay on its line + $docComment = $node->getDocComment(); + if ($docComment !== null && !str_contains($docComment->getText(), "\n")) { + $text = trim(substr($docComment->getText(), 3, -2)); + $node->setDocComment(new Doc($text === '' ? '/** */' : sprintf("/**\n * %s\n */", $text))); + } + + $this->phpDocEditor->edit($node, static function (PhpDocAstNode $phpDocNode): ?PhpDocAstNode { + if (!$phpDocNode instanceof PhpDocNode) { + return null; + } + + $phpDocNode->children[] = new PhpDocTagNode('@phpstan-pure', new GenericTagValueNode('')); + + return $phpDocNode; + }); + + return $node; + }) + ->build(); + } + /** * @param ImpurePoint[] $impurePoints * @param array $pureUnlessCallableParamNames diff --git a/src/Rules/Pure/PureFunctionRule.php b/src/Rules/Pure/PureFunctionRule.php index bfa65d09618..9801d580601 100644 --- a/src/Rules/Pure/PureFunctionRule.php +++ b/src/Rules/Pure/PureFunctionRule.php @@ -4,6 +4,7 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; +use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\RegisteredRule; use PHPStan\Node\FunctionReturnStatementsNode; use PHPStan\Rules\Rule; @@ -16,7 +17,11 @@ final class PureFunctionRule implements Rule { - public function __construct(private FunctionPurityCheck $check) + public function __construct( + private FunctionPurityCheck $check, + #[AutowiredParameter] + private bool $checkMissingPurePhpDocTag, + ) { } @@ -28,10 +33,11 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { $function = $node->getFunctionReflection(); + $description = sprintf('Function %s()', $function->getName()); - return $this->check->check( + $errors = $this->check->check( $scope, - sprintf('Function %s()', $function->getName()), + $description, 'Function', $function, $function->getParameters(), @@ -41,6 +47,24 @@ public function processNode(Node $node, Scope $scope): array $node->getStatements(), false, ); + + if ( + $this->checkMissingPurePhpDocTag + && $this->check->canBeMarkedPure( + $function, + $function->getParameters(), + $function->getReturnType(), + $node->getImpurePoints(), + $node->getStatementResult()->getThrowPoints(), + $node->isGenerator(), + $node->returnsByRef(), + false, + ) + ) { + $errors[] = $this->check->createMissingPureTagError($description, 'Function', $node->getFunction()); + } + + return $errors; } } diff --git a/src/Rules/Pure/PureMethodRule.php b/src/Rules/Pure/PureMethodRule.php index f105cc723b2..4d5d1ceb2c8 100644 --- a/src/Rules/Pure/PureMethodRule.php +++ b/src/Rules/Pure/PureMethodRule.php @@ -4,6 +4,7 @@ use PhpParser\Node; use PHPStan\Analyser\Scope; +use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\RegisteredRule; use PHPStan\Node\MethodReturnStatementsNode; use PHPStan\Rules\Rule; @@ -16,7 +17,11 @@ final class PureMethodRule implements Rule { - public function __construct(private FunctionPurityCheck $check) + public function __construct( + private FunctionPurityCheck $check, + #[AutowiredParameter] + private bool $checkMissingPurePhpDocTag, + ) { } @@ -28,10 +33,11 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { $method = $node->getMethodReflection(); + $description = sprintf('Method %s::%s()', $method->getDeclaringClass()->getDisplayName(), $method->getName()); - return $this->check->check( + $errors = $this->check->check( $scope, - sprintf('Method %s::%s()', $method->getDeclaringClass()->getDisplayName(), $method->getName()), + $description, 'Method', $method, $method->getParameters(), @@ -41,6 +47,26 @@ public function processNode(Node $node, Scope $scope): array $node->getStatements(), $method->isConstructor(), ); + + // a trait method is analysed once per using class, and its PHPDoc is shared by all of them + if ( + $this->checkMissingPurePhpDocTag + && !$scope->isInTrait() + && $this->check->canBeMarkedPure( + $method, + $method->getParameters(), + $method->getReturnType(), + $node->getImpurePoints(), + $node->getStatementResult()->getThrowPoints(), + $node->isGenerator(), + $node->returnsByRef(), + $method->isConstructor(), + ) + ) { + $errors[] = $this->check->createMissingPureTagError($description, 'Method', $node->getClassMethod()); + } + + return $errors; } } diff --git a/tests/PHPStan/Rules/Pure/PureFunctionRuleTest.php b/tests/PHPStan/Rules/Pure/PureFunctionRuleTest.php index d6cf230e4c0..fa2b2e0a1c9 100644 --- a/tests/PHPStan/Rules/Pure/PureFunctionRuleTest.php +++ b/tests/PHPStan/Rules/Pure/PureFunctionRuleTest.php @@ -12,9 +12,11 @@ class PureFunctionRuleTest extends RuleTestCase { + private bool $checkMissingPurePhpDocTag = false; + public function getRule(): Rule { - return new PureFunctionRule(new FunctionPurityCheck()); + return new PureFunctionRule(self::getContainer()->getByType(FunctionPurityCheck::class), $this->checkMissingPurePhpDocTag); } public function testRule(): void @@ -541,4 +543,33 @@ public function testPureUnlessParameterPassedBuiltin(): void ]); } + public function testMissingPureTag(): void + { + $this->checkMissingPurePhpDocTag = true; + $this->analyse([__DIR__ . '/data/missing-pure-tag.php'], [ + [ + 'Function MissingPureTag\\add() does not have any side effects and can be marked @phpstan-pure.', + 5, + ], + [ + 'Function MissingPureTag\\addOne() does not have any side effects and can be marked @phpstan-pure.', + 13, + ], + [ + 'Function MissingPureTag\\markedImpure() is marked as impure but does not have any side effects.', + 25, + ], + [ + 'Function MissingPureTag\\callsPure() does not have any side effects and can be marked @phpstan-pure.', + 132, + ], + ]); + } + + public function testFixMissingPureTag(): void + { + $this->checkMissingPurePhpDocTag = true; + $this->fix(__DIR__ . '/data/missing-pure-function-tag-fix.php', __DIR__ . '/data/missing-pure-function-tag-fix.php.fixed'); + } + } diff --git a/tests/PHPStan/Rules/Pure/PureMethodRuleTest.php b/tests/PHPStan/Rules/Pure/PureMethodRuleTest.php index f67e9172b60..93052b48738 100644 --- a/tests/PHPStan/Rules/Pure/PureMethodRuleTest.php +++ b/tests/PHPStan/Rules/Pure/PureMethodRuleTest.php @@ -13,11 +13,13 @@ class PureMethodRuleTest extends RuleTestCase { + private bool $checkMissingPurePhpDocTag = false; + private bool $treatPhpDocTypesAsCertain; public function getRule(): Rule { - return new PureMethodRule(new FunctionPurityCheck()); + return new PureMethodRule(self::getContainer()->getByType(FunctionPurityCheck::class), $this->checkMissingPurePhpDocTag); } protected function shouldTreatPhpDocTypesAsCertain(): bool @@ -438,4 +440,29 @@ public function testPureUnlessParameterPassed(): void ]); } + public function testMissingPureTag(): void + { + $this->checkMissingPurePhpDocTag = true; + $this->analyse([__DIR__ . '/data/missing-pure-tag.php'], [ + [ + 'Method MissingPureTag\\FinalClass::getValue() does not have any side effects and can be marked @phpstan-pure.', + 77, + ], + [ + 'Method MissingPureTag\\OpenClass::finalMethod() does not have any side effects and can be marked @phpstan-pure.', + 98, + ], + [ + 'Method MissingPureTag\\OpenClass::privateMethod() does not have any side effects and can be marked @phpstan-pure.', + 103, + ], + ]); + } + + public function testFixMissingPureTag(): void + { + $this->checkMissingPurePhpDocTag = true; + $this->fix(__DIR__ . '/data/missing-pure-method-tag-fix.php', __DIR__ . '/data/missing-pure-method-tag-fix.php.fixed'); + } + } diff --git a/tests/PHPStan/Rules/Pure/PurePropertyHookRuleTest.php b/tests/PHPStan/Rules/Pure/PurePropertyHookRuleTest.php index 4d8bb6d17a1..591cf04c7be 100644 --- a/tests/PHPStan/Rules/Pure/PurePropertyHookRuleTest.php +++ b/tests/PHPStan/Rules/Pure/PurePropertyHookRuleTest.php @@ -14,7 +14,7 @@ class PurePropertyHookRuleTest extends RuleTestCase public function getRule(): Rule { - return new PurePropertyHookRule(new FunctionPurityCheck()); + return new PurePropertyHookRule(self::getContainer()->getByType(FunctionPurityCheck::class)); } #[RequiresPhp('>= 8.4.0')] diff --git a/tests/PHPStan/Rules/Pure/data/missing-pure-function-tag-fix.php b/tests/PHPStan/Rules/Pure/data/missing-pure-function-tag-fix.php new file mode 100644 index 00000000000..bd74d5a55bc --- /dev/null +++ b/tests/PHPStan/Rules/Pure/data/missing-pure-function-tag-fix.php @@ -0,0 +1,28 @@ += 8.0 + +namespace MissingPureTag; + +function add(int $a, int $b): int +{ + return $a + $b; +} + +/** + * Adds one. + */ +function addOne(int $a): int +{ + return $a + 1; +} + +/** @phpstan-pure */ +function alreadyPure(int $a): int +{ + return $a; +} + +/** @phpstan-impure */ +function markedImpure(): int +{ + return 1; +} + +function echoes(int $a): int +{ + echo $a; + return $a; +} + +function returnsVoid(int $a): void +{ +} + +function throws(int $a): int +{ + if ($a < 0) { + throw new \InvalidArgumentException(); + } + + return $a; +} + +/** @throws \RuntimeException */ +function declaresThrows(int $a): int +{ + return $a; +} + +function byRef(int &$a): int +{ + return $a; +} + +function callsUnknown(callable $c): mixed +{ + return $c(); +} + +function generator(): \Generator +{ + yield 1; +} + +final class FinalClass +{ + + public function __construct(private int $value) + { + } + + public function getValue(): int + { + return $this->value; + } + + public function setValue(int $value): int + { + $this->value = $value; + return $value; + } + +} + +class OpenClass +{ + + public function overridable(): int + { + return 1; + } + + final public function finalMethod(): int + { + return 1; + } + + private function privateMethod(): int + { + return 1; + } + + public static function staticOverridable(): int + { + return 1; + } + +} + +trait SomeTrait +{ + + private function inTrait(): int + { + return 1; + } + +} + +final class UsesTrait +{ + + use SomeTrait; + +} + +function callsPure(int $a): int +{ + return alreadyPure($a) + 1; +} + +function callsImpure(int $a): int +{ + return echoes($a) + 1; +} + +function &returnsByRef(array &$items): array +{ + return $items; +} + +function readsGlobal(): int +{ + global $counter; + return $counter; +} + +function readsGlobalsArray(): mixed +{ + return $GLOBALS['counter']; +} + +function readsServer(): mixed +{ + return $_SERVER['argv']; +} + +function staticVariable(): int +{ + static $calls = 0; + return $calls; +} + +final class SharedState +{ + + private static int $count = 0; + + /** @var list */ + private array $items = []; + + public function readsStaticProperty(): int + { + return self::$count; + } + + /** @return list */ + public function &itemsByRef(): array + { + return $this->items; + } + +} + +/** @return int|void */ +function maybeReturnsVoid(int $a) +{ + if ($a > 0) { + return $a; + } +} + +/** @throws \RuntimeException|void */ +function maybeThrows(int $a): int +{ + return $a; +}