From 63956f7896780551ed1ab29e75a6a645d8a0919c Mon Sep 17 00:00:00 2001 From: Ondrej Mirtes Date: Tue, 24 Sep 2024 17:30:35 +0200 Subject: [PATCH] Moved illegalConstructorMethodCall rules from phpstan to phpstan-strict-rules --- README.md | 1 + rules.neon | 14 ++- .../IllegalConstructorMethodCallRule.php | 34 ++++++ .../IllegalConstructorStaticCallRule.php | 92 ++++++++++++++++ .../IllegalConstructorMethodCallRuleTest.php | 37 +++++++ .../IllegalConstructorStaticCallRuleTest.php | 59 ++++++++++ tests/Rules/Methods/data/bug-9577.php | 40 +++++++ .../illegal-constructor-call-rule-test.php | 103 ++++++++++++++++++ 8 files changed, 378 insertions(+), 2 deletions(-) create mode 100644 src/Rules/Methods/IllegalConstructorMethodCallRule.php create mode 100644 src/Rules/Methods/IllegalConstructorStaticCallRule.php create mode 100644 tests/Rules/Methods/IllegalConstructorMethodCallRuleTest.php create mode 100644 tests/Rules/Methods/IllegalConstructorStaticCallRuleTest.php create mode 100644 tests/Rules/Methods/data/bug-9577.php create mode 100644 tests/Rules/Methods/data/illegal-constructor-call-rule-test.php diff --git a/README.md b/README.md index 835ecca7..14ade92e 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,7 @@ parameters: switchConditionsMatchingType: false noVariableVariables: false strictArrayFilter: false + illegalConstructorMethodCall: false ``` Aside from introducing new custom rules, phpstan-strict-rules also [change the default values of some configuration parameters](https://github.com/phpstan/phpstan-strict-rules/blob/1.6.x/rules.neon#L1) that are present in PHPStan itself. These parameters are [documented on phpstan.org](https://phpstan.org/config-reference#stricter-analysis). diff --git a/rules.neon b/rules.neon index ba091691..f268d243 100644 --- a/rules.neon +++ b/rules.neon @@ -10,8 +10,6 @@ parameters: reportStaticMethodSignatures: true reportMaybesInPropertyPhpDocTypes: true reportWrongPhpDocTypeInVarTag: true - featureToggles: - illegalConstructorMethodCall: true strictRules: allRules: true disallowedLooseComparison: %strictRules.allRules% @@ -31,6 +29,7 @@ parameters: switchConditionsMatchingType: %strictRules.allRules% noVariableVariables: %strictRules.allRules% strictArrayFilter: %strictRules.allRules% + illegalConstructorMethodCall: %strictRules.allRules% parametersSchema: strictRules: structure([ @@ -52,6 +51,7 @@ parametersSchema: switchConditionsMatchingType: anyOf(bool(), arrayOf(bool())) noVariableVariables: anyOf(bool(), arrayOf(bool())) strictArrayFilter: anyOf(bool(), arrayOf(bool())) + illegalConstructorMethodCall: anyOf(bool(), arrayOf(bool())) ]) conditionalTags: @@ -133,6 +133,10 @@ conditionalTags: phpstan.rules.rule: %strictRules.noVariableVariables% PHPStan\Rules\VariableVariables\VariablePropertyFetchRule: phpstan.rules.rule: %strictRules.noVariableVariables% + PHPStan\Rules\Methods\IllegalConstructorMethodCallRule: + phpstan.rules.rule: %strictRules.illegalConstructorMethodCall% + PHPStan\Rules\Methods\IllegalConstructorStaticCallRule: + phpstan.rules.rule: %strictRules.illegalConstructorMethodCall% services: - @@ -207,6 +211,12 @@ services: - class: PHPStan\Rules\Methods\WrongCaseOfInheritedMethodRule + - + class: PHPStan\Rules\Methods\IllegalConstructorMethodCallRule + + - + class: PHPStan\Rules\Methods\IllegalConstructorStaticCallRule + - class: PHPStan\Rules\Operators\OperandInArithmeticPostDecrementRule diff --git a/src/Rules/Methods/IllegalConstructorMethodCallRule.php b/src/Rules/Methods/IllegalConstructorMethodCallRule.php new file mode 100644 index 00000000..1dba6ed6 --- /dev/null +++ b/src/Rules/Methods/IllegalConstructorMethodCallRule.php @@ -0,0 +1,34 @@ + + */ +final class IllegalConstructorMethodCallRule implements Rule +{ + + public function getNodeType(): string + { + return Node\Expr\MethodCall::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if (!$node->name instanceof Node\Identifier || $node->name->toLowerString() !== '__construct') { + return []; + } + + return [ + RuleErrorBuilder::message('Call to __construct() on an existing object is not allowed.') + ->identifier('constructor.call') + ->build(), + ]; + } + +} diff --git a/src/Rules/Methods/IllegalConstructorStaticCallRule.php b/src/Rules/Methods/IllegalConstructorStaticCallRule.php new file mode 100644 index 00000000..fa747d6a --- /dev/null +++ b/src/Rules/Methods/IllegalConstructorStaticCallRule.php @@ -0,0 +1,92 @@ + + */ +final class IllegalConstructorStaticCallRule implements Rule +{ + + public function getNodeType(): string + { + return Node\Expr\StaticCall::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if (!$node->name instanceof Node\Identifier || $node->name->toLowerString() !== '__construct') { + return []; + } + + if ($this->isCollectCallingConstructor($node, $scope)) { + return []; + } + + return [ + RuleErrorBuilder::message('Static call to __construct() is only allowed on a parent class in the constructor.') + ->identifier('constructor.call') + ->build(), + ]; + } + + private function isCollectCallingConstructor(Node\Expr\StaticCall $node, Scope $scope): bool + { + // __construct should be called from inside constructor + if ($scope->getFunction() === null) { + return false; + } + + if ($scope->getFunction()->getName() !== '__construct') { + if (!$this->isInRenamedTraitConstructor($scope)) { + return false; + } + } + + if (!$scope->isInClass()) { + return false; + } + + if (!$node->class instanceof Node\Name) { + return false; + } + + $parentClasses = array_map(static fn (string $name) => strtolower($name), $scope->getClassReflection()->getParentClassesNames()); + + return in_array(strtolower($scope->resolveName($node->class)), $parentClasses, true); + } + + private function isInRenamedTraitConstructor(Scope $scope): bool + { + if (!$scope->isInClass()) { + return false; + } + + if (!$scope->isInTrait()) { + return false; + } + + if ($scope->getFunction() === null) { + return false; + } + + $traitAliases = $scope->getClassReflection()->getNativeReflection()->getTraitAliases(); + $functionName = $scope->getFunction()->getName(); + if (!array_key_exists($functionName, $traitAliases)) { + return false; + } + + return $traitAliases[$functionName] === sprintf('%s::%s', $scope->getTraitReflection()->getName(), '__construct'); + } + +} diff --git a/tests/Rules/Methods/IllegalConstructorMethodCallRuleTest.php b/tests/Rules/Methods/IllegalConstructorMethodCallRuleTest.php new file mode 100644 index 00000000..8b0f957e --- /dev/null +++ b/tests/Rules/Methods/IllegalConstructorMethodCallRuleTest.php @@ -0,0 +1,37 @@ + + */ +class IllegalConstructorMethodCallRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new IllegalConstructorMethodCallRule(); + } + + public function testMethods(): void + { + $this->analyse([__DIR__ . '/data/illegal-constructor-call-rule-test.php'], [ + [ + 'Call to __construct() on an existing object is not allowed.', + 13, + ], + [ + 'Call to __construct() on an existing object is not allowed.', + 18, + ], + [ + 'Call to __construct() on an existing object is not allowed.', + 60, + ], + ]); + } + +} diff --git a/tests/Rules/Methods/IllegalConstructorStaticCallRuleTest.php b/tests/Rules/Methods/IllegalConstructorStaticCallRuleTest.php new file mode 100644 index 00000000..19904f72 --- /dev/null +++ b/tests/Rules/Methods/IllegalConstructorStaticCallRuleTest.php @@ -0,0 +1,59 @@ + + */ +class IllegalConstructorStaticCallRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new IllegalConstructorStaticCallRule(); + } + + public function testMethods(): void + { + $this->analyse([__DIR__ . '/data/illegal-constructor-call-rule-test.php'], [ + [ + 'Static call to __construct() is only allowed on a parent class in the constructor.', + 31, + ], + [ + 'Static call to __construct() is only allowed on a parent class in the constructor.', + 43, + ], + [ + 'Static call to __construct() is only allowed on a parent class in the constructor.', + 44, + ], + [ + 'Static call to __construct() is only allowed on a parent class in the constructor.', + 49, + ], + [ + 'Static call to __construct() is only allowed on a parent class in the constructor.', + 50, + ], + [ + 'Static call to __construct() is only allowed on a parent class in the constructor.', + 100, + ], + ]); + } + + public function testBug9577(): void + { + if (PHP_VERSION_ID < 80100) { + self::markTestSkipped('Test requires PHP 8.1.'); + } + + $this->analyse([__DIR__ . '/data/bug-9577.php'], []); + } + +} diff --git a/tests/Rules/Methods/data/bug-9577.php b/tests/Rules/Methods/data/bug-9577.php new file mode 100644 index 00000000..2214d45b --- /dev/null +++ b/tests/Rules/Methods/data/bug-9577.php @@ -0,0 +1,40 @@ += 8.1 + +namespace Bug9577IllegalConstructorStaticCall; + +trait StringableMessageTrait +{ + public function __construct( + private readonly \Stringable $StringableMessage, + int $code = 0, + ?\Throwable $previous = null, + ) { + parent::__construct((string) $StringableMessage, $code, $previous); + } + + public function getStringableMessage(): \Stringable + { + return $this->StringableMessage; + } +} + +class SpecializedException extends \RuntimeException +{ + use StringableMessageTrait { + StringableMessageTrait::__construct as __traitConstruct; + } + + public function __construct( + private readonly object $aService, + \Stringable $StringableMessage, + int $code = 0, + ?\Throwable $previous = null, + ) { + $this->__traitConstruct($StringableMessage, $code, $previous); + } + + public function getService(): object + { + return $this->aService; + } +} diff --git a/tests/Rules/Methods/data/illegal-constructor-call-rule-test.php b/tests/Rules/Methods/data/illegal-constructor-call-rule-test.php new file mode 100644 index 00000000..f5198a50 --- /dev/null +++ b/tests/Rules/Methods/data/illegal-constructor-call-rule-test.php @@ -0,0 +1,103 @@ + 1) { + return; + } + $this->__construct($datetime, $timezone); + } + + public function mutate(string $datetime = "now", ?\DateTimeZone $timezone = null): void + { + $this->__construct($datetime, $timezone); + } +} + +class ExtendedDateTimeWithParentCall extends \DateTimeImmutable +{ + public function __construct(string $datetime = "now", ?\DateTimeZone $timezone = null) + { + parent::__construct($datetime, $timezone); + } + + public function mutate(string $datetime = "now", ?\DateTimeZone $timezone = null): void + { + parent::__construct($datetime, $timezone); + } +} + +class ExtendedDateTimeWithSelfCall extends \DateTimeImmutable +{ + public function __construct(string $datetime = "now", ?\DateTimeZone $timezone = null) + { + // Avoid infinite loop + if (count(debug_backtrace()) > 1) { + return; + } + self::__construct($datetime, $timezone); + ExtendedDateTimeWithSelfCall::__construct($datetime, $timezone); + } + + public function mutate(string $datetime = "now", ?\DateTimeZone $timezone = null): void + { + self::__construct($datetime, $timezone); + ExtendedDateTimeWithSelfCall::__construct($datetime, $timezone); + } +} + +class Foo +{ + + public function doFoo() + { + $extendedDateTime = new ExtendedDateTimeWithMethodCall('2022/04/12'); + $extendedDateTime->__construct('2022/04/13'); + } + +} + +abstract class Presenter +{ + + public function __construct() + { + + } + +} + +abstract class BasePresenter extends Presenter +{ + + public function __construct() + { + Presenter::__construct(); + } + +} + +class CatPresenter extends BasePresenter +{ + + public function __construct() + { + Presenter::__construct(); + } + +} + +class DogPresenter extends BasePresenter +{ + + public function __construct() + { + CatPresenter::__construct(); + } + +}