diff --git a/phpstan/include-by-php-version.php b/phpstan/include-by-php-version.php index 6038baf5..e2abf011 100644 --- a/phpstan/include-by-php-version.php +++ b/phpstan/include-by-php-version.php @@ -8,6 +8,11 @@ $includes[] = __DIR__ . '/../rules.neon'; } +// PHP < 8.0: exclude test fixtures using nullsafe operator syntax +if (version_compare(PHP_VERSION, '8.0', '<')) { + $includes[] = __DIR__ . '/php-below-8.0.neon'; +} + // PHP < 8.1: exclude enums, add ignores for older PHPStan versions if (version_compare(PHP_VERSION, '8.1', '<')) { $includes[] = __DIR__ . '/php-below-8.1.neon'; diff --git a/phpstan/php-below-8.0.neon b/phpstan/php-below-8.0.neon new file mode 100644 index 00000000..b293c9f5 --- /dev/null +++ b/phpstan/php-below-8.0.neon @@ -0,0 +1,4 @@ +parameters: + excludePaths: + - ../tests/PHPStan/data/method-chain-nullsafe-violations.php + - ../tests/PHPStan/data/method-chain-nullsafe-correct.php diff --git a/phpstan/php-below-8.1.neon b/phpstan/php-below-8.1.neon index a2f36593..5c8f46f7 100644 --- a/phpstan/php-below-8.1.neon +++ b/phpstan/php-below-8.1.neon @@ -30,6 +30,7 @@ parameters: # Return type differences in older PHPStan rule interfaces - '#Method MLL\\Utils\\PHPStan\\Rules\\MissingClosureParameterTypehintRule::processNode\(\) should return array but returns array\.#' + - '#Method MLL\\Utils\\PHPStan\\Rules\\OneThingPerLineRule::processNode\(\) should return array but returns array\.#' # Existing code with @phpstan-ignore that older versions don't understand - message: '#Cannot access property \$name on SimpleXMLElement\|null\.#' diff --git a/src/PHPStan/Rules/OneThingPerLineRule.php b/src/PHPStan/Rules/OneThingPerLineRule.php new file mode 100644 index 00000000..f58bda56 --- /dev/null +++ b/src/PHPStan/Rules/OneThingPerLineRule.php @@ -0,0 +1,85 @@ + + */ +final class OneThingPerLineRule implements Rule +{ + /** @var array */ + private array $fileContentsCache = []; + + public function getNodeType(): string + { + return CallLike::class; + } + + /** @return list */ + public function processNode(Node $node, Scope $scope): array + { + if (! $node instanceof MethodCall + && ! $node instanceof NullsafeMethodCall) { + return []; + } + + $var = $node->var; + if (! $var instanceof MethodCall + && ! $var instanceof NullsafeMethodCall) { + return []; + } + + if ($node->name->getStartLine() !== $var->name->getStartLine()) { + return []; + } + + if ($var->getArgs() === []) { + return []; + } + + if ($this->isInsideStringInterpolation($scope->getFile(), $node)) { + return []; + } + + return [ + RuleErrorBuilder::message('Method chain calls must each be on their own line.') + ->identifier('mll.oneThingPerLine') + ->line($node->name->getStartLine()) + ->build(), + ]; + } + + private function isInsideStringInterpolation(string $file, Expr $node): bool + { + $root = $node; + while ($root instanceof MethodCall + || $root instanceof NullsafeMethodCall + || $root instanceof PropertyFetch + || $root instanceof NullsafePropertyFetch) { + $root = $root->var; + } + + $startPos = $root->getStartFilePos(); + if ($startPos <= 0) { + return false; + } + + $this->fileContentsCache[$file] ??= file_get_contents($file); + + return $this->fileContentsCache[$file][$startPos - 1] === '{'; + } +} diff --git a/tests/PHPStan/OneThingPerLineRuleTest.php b/tests/PHPStan/OneThingPerLineRuleTest.php new file mode 100644 index 00000000..0e427407 --- /dev/null +++ b/tests/PHPStan/OneThingPerLineRuleTest.php @@ -0,0 +1,94 @@ +>}> */ + public static function dataIntegrationTests(): iterable + { + self::getContainer(); + + yield [__DIR__ . '/data/method-chain-violations.php', [ + 26 => [self::ERROR_MESSAGE], + 31 => [self::ERROR_MESSAGE], + 38 => [self::ERROR_MESSAGE], + 45 => [self::ERROR_MESSAGE], + ]]; + + yield [__DIR__ . '/data/method-chain-correct.php', []]; + + if (PHP_VERSION_ID >= 80000) { + yield [__DIR__ . '/data/method-chain-nullsafe-violations.php', [ + 19 => [self::ERROR_MESSAGE], + 24 => [self::ERROR_MESSAGE], + ]]; + + yield [__DIR__ . '/data/method-chain-nullsafe-correct.php', []]; + } + } + + /** + * @param array> $expectedErrors + * + * @dataProvider dataIntegrationTests + */ + #[DataProvider('dataIntegrationTests')] + public function testIntegration(string $file, array $expectedErrors): void + { + $errors = $this->runAnalyse($file); + + $ourErrors = array_filter( + $errors, + static fn (Error $error): bool => str_contains($error->getMessage(), self::ERROR_MESSAGE), + ); + + if ($expectedErrors === []) { + self::assertEmpty($ourErrors, 'Should not report errors for correct code'); + } else { + self::assertNotEmpty($ourErrors, 'Should detect method chain violations'); + $this->assertSameErrorMessages($expectedErrors, $ourErrors); + } + } + + /** @return array */ + private function runAnalyse(string $file): array + { + $file = self::getFileHelper()->normalizePath($file); + + /** @var Analyser $analyser */ + $analyser = self::getContainer()->getByType(Analyser::class); + + return $analyser->analyse([$file])->getErrors(); + } + + /** + * @param array> $expectedErrors + * @param array $errors + */ + private function assertSameErrorMessages(array $expectedErrors, array $errors): void + { + foreach ($errors as $error) { + $errorLine = $error->getLine() ?? 0; + $errorMessage = $error->getMessage(); + + self::assertArrayHasKey($errorLine, $expectedErrors, "Unexpected error at line {$errorLine}: {$errorMessage}"); + self::assertContains($errorMessage, $expectedErrors[$errorLine]); + } + } + + /** @return array */ + public static function getAdditionalConfigFiles(): array + { + return [ + __DIR__ . '/phpstan-one-thing-per-line-test.neon', + ]; + } +} diff --git a/tests/PHPStan/data/method-chain-correct.php b/tests/PHPStan/data/method-chain-correct.php new file mode 100644 index 00000000..4b2a4698 --- /dev/null +++ b/tests/PHPStan/data/method-chain-correct.php @@ -0,0 +1,123 @@ +foo(); + } + + public function staticPlusSingleMethod(): void + { + self::create()->foo(); + } + + public function propertyAccessPlusMethod(): void + { + $this->relation->foo(); + } + + public function properlySplitChain(): void + { + $this->foo() + ->bar(); + } + + public function properlySplitThreeChain(): void + { + $this->foo() + ->bar() + ->baz(); + } + + public function newExpressionPlusSingleMethod(): void + { + (new self())->baz(); + } + + public function multilineArgsWithSplitContinuation(): void + { + $this->foo( + 1 + )->bar(); + } + + public function noArgAccessorThenAction(): void + { + $this->foo()->baz(); + } + + public function noArgAccessorThenActionWithArgs(): void + { + $this->foo()->foo(1); + } + + public function noArgChainAllAccessors(): void + { + $this->foo()->bar()->baz(); + } + + public function staticPlusNoArgChain(): void + { + self::create()->foo()->bar(); + } + + public function propertyThenNoArgAccessorThenAction(): void + { + $this->relation->foo()->baz(); + } + + public function noArgAccessorChainInArrowFunction(): void + { + /** @var list $items */ + $items = []; + array_map(fn (self $x): self => $x->foo()->bar(), $items); + } + + public function noArgAccessorChainInClosure(): void + { + /** @var list $items */ + $items = []; + array_map(fn (self $x): int => spl_object_id($x->foo()->bar()), $items); + } + + public function chainInsideStringInterpolation(): string + { + return "value: {$this->foo()->name()}"; + } + + public function properlySplitChainInClosure(): void + { + /** @var list $items */ + $items = []; + array_map(fn (self $x): self => $x->foo() + ->bar(), $items); + } +} diff --git a/tests/PHPStan/data/method-chain-nullsafe-correct.php b/tests/PHPStan/data/method-chain-nullsafe-correct.php new file mode 100644 index 00000000..71d2e19e --- /dev/null +++ b/tests/PHPStan/data/method-chain-nullsafe-correct.php @@ -0,0 +1,32 @@ +foo() + ?->bar(); + } + + public function noArgNullSafeChain(?self $nullable): void + { + $nullable?->foo()?->bar(); + } + + public function mixedNoArgNullSafeChain(?self $nullable): void + { + $nullable?->foo()->bar(); + } +} diff --git a/tests/PHPStan/data/method-chain-nullsafe-violations.php b/tests/PHPStan/data/method-chain-nullsafe-violations.php new file mode 100644 index 00000000..134f30b7 --- /dev/null +++ b/tests/PHPStan/data/method-chain-nullsafe-violations.php @@ -0,0 +1,26 @@ +foo(1)?->bar(2); + } + + public function mixedNullSafeChainWithArgs(?self $nullable): void + { + $nullable?->foo(1)->bar(2); + } +} diff --git a/tests/PHPStan/data/method-chain-violations.php b/tests/PHPStan/data/method-chain-violations.php new file mode 100644 index 00000000..44dac234 --- /dev/null +++ b/tests/PHPStan/data/method-chain-violations.php @@ -0,0 +1,47 @@ +foo(1)->bar(2); + } + + public function threeCallsFirstHasArgs(): void + { + $this->foo(1)->bar()->baz(); + } + + public function arrowFunctionChainWithArgs(): void + { + /** @var list $items */ + $items = []; + array_map(fn (self $x): self => $x->foo(1)->bar(2), $items); + } + + public function closureChainWithArgs(): void + { + /** @var list $items */ + $items = []; + array_map(fn (self $x): int => spl_object_id($x->foo(1)->bar(2)), $items); + } +} diff --git a/tests/PHPStan/phpstan-one-thing-per-line-test.neon b/tests/PHPStan/phpstan-one-thing-per-line-test.neon new file mode 100644 index 00000000..ef40518d --- /dev/null +++ b/tests/PHPStan/phpstan-one-thing-per-line-test.neon @@ -0,0 +1,5 @@ +parameters: + customRulesetUsed: true + +rules: + - MLL\Utils\PHPStan\Rules\OneThingPerLineRule