diff --git a/build/PHPStan/Build/InlineCallCollector.php b/build/PHPStan/Build/InlineCallCollector.php index 1c883ea2812..dffe5511743 100644 --- a/build/PHPStan/Build/InlineCallCollector.php +++ b/build/PHPStan/Build/InlineCallCollector.php @@ -47,9 +47,13 @@ * * Callees are ones no subclass can override — final class, final or private * method — or, closed-world, non-final ones nothing in the scanned code base - * overrides (OverridesScanner). Every call frame saved is engine work saved: - * a getter call costs about 40ns of frame setup for a body that reads one - * property; a self-analysis measured -4% user CPU. + * overrides (OverridesScanner). The closed world stops at PHPStan's extension + * surface: an abstract class or a non-final class tagged `@api` exists to be + * subclassed by third parties, whose overrides the scan cannot see, so its + * overridable methods stay calls and its properties stay non-public + * (isExtensible()). Every call frame saved is engine work saved: a getter + * call costs about 40ns of frame setup for a body that reads one property; + * a self-analysis measured -4% user CPU. * * @implements Collector}> */ @@ -76,7 +80,10 @@ final class InlineCallCollector implements Collector /** @var array|null */ private ?array $overrides = null; - public function __construct(private Parser $parser, private ReflectionProvider $reflectionProvider) + /** + * @param list|null $directories the closed world to scan for overrides; null = directories() + */ + public function __construct(private Parser $parser, private ReflectionProvider $reflectionProvider, private ?array $directories = null) { } @@ -116,7 +123,7 @@ public function processNode(Node $node, Scope $scope): ?array return null; } $guardFree = $declaringClass->isFinal() || $method->isFinal()->yes() || $method->isPrivate(); - if (!$guardFree && $this->isOverridden($declaringClass, $methodName)) { + if (!$guardFree && ($this->isExtensible($declaringClass) || $this->isOverridden($declaringClass, $methodName))) { return null; } @@ -235,6 +242,11 @@ public function processNode(Node $node, Scope $scope): ?array if ($target['file'] !== null && $this->isInProtectedPackage($target['file'])) { return null; } + // a third-party subclass redeclaring the property would no longer load + // ("Access level to Sub::$x must be public") + if ($this->reflectionProvider->hasClass($target['class']) && $this->isExtensible($this->reflectionProvider->getClass($target['class']))) { + return null; + } $publicize[] = $target; } } @@ -490,12 +502,40 @@ private function scanner(): OverridesScanner { if ($this->scanner === null) { $this->scanner = new OverridesScanner(); - $this->overrides = $this->scanner->scan(self::directories()); + $this->overrides = $this->scanner->scan($this->directories ?? self::directories()); } return $this->scanner; } + /** + * Whether third parties are meant to subclass the class, so that the + * closed-world scan cannot vouch for its overridable methods or for + * subclasses redeclaring its properties: abstract classes and non-final + * classes tagged `@api` (the backward compatibility promise lets + * extensions extend those). + */ + private function isExtensible(ClassReflection $classReflection): bool + { + if ($classReflection->isFinal()) { + return false; + } + if ($classReflection->isAbstract()) { + return true; + } + $docBlock = $classReflection->getResolvedPhpDoc(); + if ($docBlock === null) { + return false; + } + foreach ($docBlock->getPhpDocNodes() as $phpDocNode) { + if (count($phpDocNode->getTagsByName('@api')) > 0) { + return true; + } + } + + return false; + } + private function isOverridden(ClassReflection $declaringClass, string $methodName): bool { $this->scanner(); diff --git a/build/PHPStan/Build/OverridesScanner.php b/build/PHPStan/Build/OverridesScanner.php index 824044ce7e2..67825a0dbe7 100644 --- a/build/PHPStan/Build/OverridesScanner.php +++ b/build/PHPStan/Build/OverridesScanner.php @@ -21,9 +21,11 @@ * Which methods some class in the scanned code base overrides — the * closed-world half of InlineCallCollector: a non-final method nothing * overrides is as safe to inline as a final one, as long as the code base is - * the whole world (PHPStan's own phar, where extensions subclassing - * PHPStan's classes are the accepted exception: they still work, they just - * see the parent's inlined bodies at PHPStan's own call sites). + * the whole world. It is not for the classes PHPStan invites extensions to + * subclass (abstract classes and non-final `@api` classes — a test case + * overriding RuleTestCase::getCollectors(), an ObjectType subclass + * overriding describeAdditionalCacheKey()); InlineCallCollector keeps those + * out of the closed world on its own. * * Conservative: a method declared by a class (or by a trait it uses) counts * as overriding it on every ancestor, whether or not that ancestor declares diff --git a/tests/PHPStan/Build/InlineCallCollectorTest.php b/tests/PHPStan/Build/InlineCallCollectorTest.php new file mode 100644 index 00000000000..7932b956929 --- /dev/null +++ b/tests/PHPStan/Build/InlineCallCollectorTest.php @@ -0,0 +1,104 @@ +> + */ +class InlineCallCollectorTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + return new /** @implements Rule */ class implements Rule { + + public function getNodeType(): string + { + return CollectedDataNode::class; + } + + public function processNode(Node $node, Scope $scope): array + { + $errors = []; + foreach ($node->get(InlineCallCollector::class) as $file => $edits) { + $contents = (string) file_get_contents($file); + foreach ($edits as $edit) { + $errors[] = RuleErrorBuilder::message(sprintf('%s => %s', $edit['callee'], $edit['replacement'])) + ->identifier('test.inline') + ->file($file) + ->line(substr_count(substr($contents, 0, $edit['start']), "\n") + 1) + ->build(); + } + } + + return $errors; + } + + }; + } + + protected function getCollectors(): array + { + return [ + new InlineCallCollector( + self::getContainer()->getService('defaultAnalysisParser'), + self::createReflectionProvider(), + [__DIR__ . '/data/inline-call-collector'], + ), + ]; + } + + public function testInlinesOnlyWhatNoSubclassCanOverride(): void + { + $this->analyse([__DIR__ . '/data/inline-call-collector/world.php'], [ + // FinalGetter::getValue (final class), ClosedWorldGetter::getValue + // (nothing in the world overrides it) and getFinalValue (final method), + // FinalApiGetter::getValue (@api but final), ReadsOthers::getClosedWorld + // (private) are inlined; ClosedWorldGetter::getOverriddenValue + // (OverridingGetter overrides it), AbstractHooks::getHooks (abstract + // class: any subclass may override it) and ApiGetter::getValue and + // describeAdditionalCacheKey (@api class: third parties may extend + // it) are not; AbstractHooks::getSecret is private, so it is. + [ + 'InlineCallCollectorTest\AbstractHooks::getSecret => \'secret\'', + 75, + ], + [ + 'InlineCallCollectorTest\FinalGetter::getValue => $finalGetter->value', + 145, + ], + [ + 'InlineCallCollectorTest\ClosedWorldGetter::getValue => $closedWorldGetter->value', + 146, + ], + [ + 'InlineCallCollectorTest\ClosedWorldGetter::getFinalValue => $closedWorldGetter->value', + 148, + ], + [ + 'InlineCallCollectorTest\FinalApiGetter::getValue => $finalApiGetter->value', + 150, + ], + [ + 'InlineCallCollectorTest\ClosedWorldGetter::getValue => $this->getClosedWorld()->value', + 151, + ], + [ + 'InlineCallCollectorTest\ReadsOthers::getClosedWorld => $this->closedWorld', + 151, + ], + ]); + } + +} diff --git a/tests/PHPStan/Build/data/inline-call-collector/world.php b/tests/PHPStan/Build/data/inline-call-collector/world.php new file mode 100644 index 00000000000..453b976849c --- /dev/null +++ b/tests/PHPStan/Build/data/inline-call-collector/world.php @@ -0,0 +1,154 @@ +value = $value; + } + + public function getValue(): string + { + return $this->value; + } + +} + +class ClosedWorldGetter +{ + + private int $value; + + public function __construct(int $value) + { + $this->value = $value; + } + + public function getValue(): int + { + return $this->value; + } + + public function getOverriddenValue(): int + { + return $this->value; + } + + final public function getFinalValue(): int + { + return $this->value; + } + +} + +class OverridingGetter extends ClosedWorldGetter +{ + + public function getOverriddenValue(): int + { + return 42; + } + +} + +abstract class AbstractHooks +{ + + /** @return list */ + protected function getHooks(): array + { + return []; + } + + private function getSecret(): string + { + return 'secret'; + } + + public function run(): string + { + return implode(',', $this->getHooks()) . $this->getSecret(); + } + +} + +/** @api */ +class ApiGetter +{ + + private string $value; + + public function __construct(string $value) + { + $this->value = $value; + } + + public function getValue(): string + { + return $this->value; + } + + protected function describeAdditionalCacheKey(): string + { + return ''; + } + + public function describe(): string + { + return $this->value . $this->describeAdditionalCacheKey(); + } + +} + +/** + * @api + */ +final class FinalApiGetter +{ + + private string $value; + + public function __construct(string $value) + { + $this->value = $value; + } + + public function getValue(): string + { + return $this->value; + } + +} + +final class ReadsOthers +{ + + private ClosedWorldGetter $closedWorld; + + public function __construct(ClosedWorldGetter $closedWorld) + { + $this->closedWorld = $closedWorld; + } + + private function getClosedWorld(): ClosedWorldGetter + { + return $this->closedWorld; + } + + public function doSomething(FinalGetter $finalGetter, ClosedWorldGetter $closedWorldGetter, ApiGetter $apiGetter, FinalApiGetter $finalApiGetter): string + { + return $finalGetter->getValue() + . $closedWorldGetter->getValue() + . $closedWorldGetter->getOverriddenValue() + . $closedWorldGetter->getFinalValue() + . $apiGetter->getValue() + . $finalApiGetter->getValue() + . $this->getClosedWorld()->getValue(); + } + +}