Do not inline overridable methods of abstract and @api classes in the phar build

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<MethodCall, array{file: string, start: int, end: int, replacement: string, callee: string, publicize: list<array{class: string, property: string, file: string|null}>}>
  */
@@ -76,7 +80,10 @@ final class InlineCallCollector implements Collector
 	/** @var array<string, true>|null */
 	private ?array $overrides = null;
 
-	public function __construct(private Parser $parser, private ReflectionProvider $reflectionProvider)
+	/**
+	 * @param list<string>|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();

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

tests/PHPStan/Build/InlineCallCollectorTest.php

@@ -0,0 +1,104 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Build;
+
+use PhpParser\Node;
+use PHPStan\Analyser\Scope;
+use PHPStan\Node\CollectedDataNode;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
+use PHPStan\Testing\RuleTestCase;
+use function file_get_contents;
+use function sprintf;
+use function substr;
+use function substr_count;
+
+/**
+ * @extends RuleTestCase<Rule<CollectedDataNode>>
+ */
+class InlineCallCollectorTest extends RuleTestCase
+{
+
+	protected function getRule(): Rule
+	{
+		return new /** @implements Rule<CollectedDataNode> */ 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,
+			],
+		]);
+	}
+
+}

tests/PHPStan/Build/data/inline-call-collector/world.php

@@ -0,0 +1,154 @@
+<?php declare(strict_types = 1);
+
+namespace InlineCallCollectorTest;
+
+final class FinalGetter
+{
+
+	private string $value;
+
+	public function __construct(string $value)
+	{
+		$this->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<string> */
+	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();
+	}
+
+}