Skip to content

Commit 8225ce9

Browse files
ondrejmirtesclaude
ondrejmirtes
and
claude
committed
Do not inline overridable methods of abstract and @api classes in the phar build
The getter inliner treated src/ and the bundled vendor packages as a closed world: a non-final method nothing there overrides was inlined at PHPStan's own call sites. Third-party subclasses were the blind spot, and the classes they subclass are exactly the ones whose hooks exist to be overridden: RuleTestCase::getCollectors() became `new CollectorRegistry([])` in the phar (shipmonk/dead-code-detector reported nothing), shouldPolluteScopeWithLoopInitialAssignments() became `true` (phpstan-strict-rules), ObjectType::describeAdditionalCacheKey() became '' (phpstan-symfony overrides it in ParentObjectType and TreeBuilderType). The closed world now stops at the extension surface: an abstract class or a non-final class tagged @api may be subclassed by extensions under the backward compatibility promise, so its overridable methods stay calls and its properties are not made public. Final classes, final and private methods and non-@api concrete classes are inlined as before. 7,177 of 8,578 call sites remain; the 1,401 dropped are in 33 classes, mostly the constant type getters. The scanned directories are an optional constructor argument so the collector can be tested against a fixture world. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UMpKB12KsL5HB8iUMCgrUM
1 parent ce1c164 commit 8225ce9

4 files changed

Lines changed: 309 additions & 9 deletions

File tree

‎build/PHPStan/Build/InlineCallCollector.php‎

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,13 @@
4747
*
4848
* Callees are ones no subclass can override — final class, final or private
4949
* method — or, closed-world, non-final ones nothing in the scanned code base
50-
* overrides (OverridesScanner). Every call frame saved is engine work saved:
51-
* a getter call costs about 40ns of frame setup for a body that reads one
52-
* property; a self-analysis measured -4% user CPU.
50+
* overrides (OverridesScanner). The closed world stops at PHPStan's extension
51+
* surface: an abstract class or a non-final class tagged `@api` exists to be
52+
* subclassed by third parties, whose overrides the scan cannot see, so its
53+
* overridable methods stay calls and its properties stay non-public
54+
* (isExtensible()). Every call frame saved is engine work saved: a getter
55+
* call costs about 40ns of frame setup for a body that reads one property;
56+
* a self-analysis measured -4% user CPU.
5357
*
5458
* @implements Collector<MethodCall, array{file: string, start: int, end: int, replacement: string, callee: string, publicize: list<array{class: string, property: string, file: string|null}>}>
5559
*/
@@ -76,7 +80,10 @@ final class InlineCallCollector implements Collector
7680
/** @var array<string, true>|null */
7781
private ?array $overrides = null;
7882

79-
public function __construct(private Parser $parser, private ReflectionProvider $reflectionProvider)
83+
/**
84+
* @param list<string>|null $directories the closed world to scan for overrides; null = directories()
85+
*/
86+
public function __construct(private Parser $parser, private ReflectionProvider $reflectionProvider, private ?array $directories = null)
8087
{
8188
}
8289

@@ -116,7 +123,7 @@ public function processNode(Node $node, Scope $scope): ?array
116123
return null;
117124
}
118125
$guardFree = $declaringClass->isFinal() || $method->isFinal()->yes() || $method->isPrivate();
119-
if (!$guardFree && $this->isOverridden($declaringClass, $methodName)) {
126+
if (!$guardFree && ($this->isExtensible($declaringClass) || $this->isOverridden($declaringClass, $methodName))) {
120127
return null;
121128
}
122129

@@ -235,6 +242,11 @@ public function processNode(Node $node, Scope $scope): ?array
235242
if ($target['file'] !== null && $this->isInProtectedPackage($target['file'])) {
236243
return null;
237244
}
245+
// a third-party subclass redeclaring the property would no longer load
246+
// ("Access level to Sub::$x must be public")
247+
if ($this->reflectionProvider->hasClass($target['class']) && $this->isExtensible($this->reflectionProvider->getClass($target['class']))) {
248+
return null;
249+
}
238250
$publicize[] = $target;
239251
}
240252
}
@@ -490,12 +502,40 @@ private function scanner(): OverridesScanner
490502
{
491503
if ($this->scanner === null) {
492504
$this->scanner = new OverridesScanner();
493-
$this->overrides = $this->scanner->scan(self::directories());
505+
$this->overrides = $this->scanner->scan($this->directories ?? self::directories());
494506
}
495507

496508
return $this->scanner;
497509
}
498510

511+
/**
512+
* Whether third parties are meant to subclass the class, so that the
513+
* closed-world scan cannot vouch for its overridable methods or for
514+
* subclasses redeclaring its properties: abstract classes and non-final
515+
* classes tagged `@api` (the backward compatibility promise lets
516+
* extensions extend those).
517+
*/
518+
private function isExtensible(ClassReflection $classReflection): bool
519+
{
520+
if ($classReflection->isFinal()) {
521+
return false;
522+
}
523+
if ($classReflection->isAbstract()) {
524+
return true;
525+
}
526+
$docBlock = $classReflection->getResolvedPhpDoc();
527+
if ($docBlock === null) {
528+
return false;
529+
}
530+
foreach ($docBlock->getPhpDocNodes() as $phpDocNode) {
531+
if (count($phpDocNode->getTagsByName('@api')) > 0) {
532+
return true;
533+
}
534+
}
535+
536+
return false;
537+
}
538+
499539
private function isOverridden(ClassReflection $declaringClass, string $methodName): bool
500540
{
501541
$this->scanner();

‎build/PHPStan/Build/OverridesScanner.php‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,11 @@
2121
* Which methods some class in the scanned code base overrides — the
2222
* closed-world half of InlineCallCollector: a non-final method nothing
2323
* overrides is as safe to inline as a final one, as long as the code base is
24-
* the whole world (PHPStan's own phar, where extensions subclassing
25-
* PHPStan's classes are the accepted exception: they still work, they just
26-
* see the parent's inlined bodies at PHPStan's own call sites).
24+
* the whole world. It is not for the classes PHPStan invites extensions to
25+
* subclass (abstract classes and non-final `@api` classes — a test case
26+
* overriding RuleTestCase::getCollectors(), an ObjectType subclass
27+
* overriding describeAdditionalCacheKey()); InlineCallCollector keeps those
28+
* out of the closed world on its own.
2729
*
2830
* Conservative: a method declared by a class (or by a trait it uses) counts
2931
* as overriding it on every ancestor, whether or not that ancestor declares

‎tests/PHPStan/Build/InlineCallCollectorTest.php‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Build;
4+
5+
use PhpParser\Node;
6+
use PHPStan\Analyser\Scope;
7+
use PHPStan\Node\CollectedDataNode;
8+
use PHPStan\Rules\Rule;
9+
use PHPStan\Rules\RuleErrorBuilder;
10+
use PHPStan\Testing\RuleTestCase;
11+
use function file_get_contents;
12+
use function sprintf;
13+
use function substr;
14+
use function substr_count;
15+
16+
/**
17+
* @extends RuleTestCase<Rule<CollectedDataNode>>
18+
*/
19+
class InlineCallCollectorTest extends RuleTestCase
20+
{
21+
22+
protected function getRule(): Rule
23+
{
24+
return new /** @implements Rule<CollectedDataNode> */ class implements Rule {
25+
26+
public function getNodeType(): string
27+
{
28+
return CollectedDataNode::class;
29+
}
30+
31+
public function processNode(Node $node, Scope $scope): array
32+
{
33+
$errors = [];
34+
foreach ($node->get(InlineCallCollector::class) as $file => $edits) {
35+
$contents = (string) file_get_contents($file);
36+
foreach ($edits as $edit) {
37+
$errors[] = RuleErrorBuilder::message(sprintf('%s => %s', $edit['callee'], $edit['replacement']))
38+
->identifier('test.inline')
39+
->file($file)
40+
->line(substr_count(substr($contents, 0, $edit['start']), "\n") + 1)
41+
->build();
42+
}
43+
}
44+
45+
return $errors;
46+
}
47+
48+
};
49+
}
50+
51+
protected function getCollectors(): array
52+
{
53+
return [
54+
new InlineCallCollector(
55+
self::getContainer()->getService('defaultAnalysisParser'),
56+
self::createReflectionProvider(),
57+
[__DIR__ . '/data/inline-call-collector'],
58+
),
59+
];
60+
}
61+
62+
public function testInlinesOnlyWhatNoSubclassCanOverride(): void
63+
{
64+
$this->analyse([__DIR__ . '/data/inline-call-collector/world.php'], [
65+
// FinalGetter::getValue (final class), ClosedWorldGetter::getValue
66+
// (nothing in the world overrides it) and getFinalValue (final method),
67+
// FinalApiGetter::getValue (@api but final), ReadsOthers::getClosedWorld
68+
// (private) are inlined; ClosedWorldGetter::getOverriddenValue
69+
// (OverridingGetter overrides it), AbstractHooks::getHooks (abstract
70+
// class: any subclass may override it) and ApiGetter::getValue and
71+
// describeAdditionalCacheKey (@api class: third parties may extend
72+
// it) are not; AbstractHooks::getSecret is private, so it is.
73+
[
74+
'InlineCallCollectorTest\AbstractHooks::getSecret => \'secret\'',
75+
75,
76+
],
77+
[
78+
'InlineCallCollectorTest\FinalGetter::getValue => $finalGetter->value',
79+
145,
80+
],
81+
[
82+
'InlineCallCollectorTest\ClosedWorldGetter::getValue => $closedWorldGetter->value',
83+
146,
84+
],
85+
[
86+
'InlineCallCollectorTest\ClosedWorldGetter::getFinalValue => $closedWorldGetter->value',
87+
148,
88+
],
89+
[
90+
'InlineCallCollectorTest\FinalApiGetter::getValue => $finalApiGetter->value',
91+
150,
92+
],
93+
[
94+
'InlineCallCollectorTest\ClosedWorldGetter::getValue => $this->getClosedWorld()->value',
95+
151,
96+
],
97+
[
98+
'InlineCallCollectorTest\ReadsOthers::getClosedWorld => $this->closedWorld',
99+
151,
100+
],
101+
]);
102+
}
103+
104+
}

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

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace InlineCallCollectorTest;
4+
5+
final class FinalGetter
6+
{
7+
8+
private string $value;
9+
10+
public function __construct(string $value)
11+
{
12+
$this->value = $value;
13+
}
14+
15+
public function getValue(): string
16+
{
17+
return $this->value;
18+
}
19+
20+
}
21+
22+
class ClosedWorldGetter
23+
{
24+
25+
private int $value;
26+
27+
public function __construct(int $value)
28+
{
29+
$this->value = $value;
30+
}
31+
32+
public function getValue(): int
33+
{
34+
return $this->value;
35+
}
36+
37+
public function getOverriddenValue(): int
38+
{
39+
return $this->value;
40+
}
41+
42+
final public function getFinalValue(): int
43+
{
44+
return $this->value;
45+
}
46+
47+
}
48+
49+
class OverridingGetter extends ClosedWorldGetter
50+
{
51+
52+
public function getOverriddenValue(): int
53+
{
54+
return 42;
55+
}
56+
57+
}
58+
59+
abstract class AbstractHooks
60+
{
61+
62+
/** @return list<string> */
63+
protected function getHooks(): array
64+
{
65+
return [];
66+
}
67+
68+
private function getSecret(): string
69+
{
70+
return 'secret';
71+
}
72+
73+
public function run(): string
74+
{
75+
return implode(',', $this->getHooks()) . $this->getSecret();
76+
}
77+
78+
}
79+
80+
/** @api */
81+
class ApiGetter
82+
{
83+
84+
private string $value;
85+
86+
public function __construct(string $value)
87+
{
88+
$this->value = $value;
89+
}
90+
91+
public function getValue(): string
92+
{
93+
return $this->value;
94+
}
95+
96+
protected function describeAdditionalCacheKey(): string
97+
{
98+
return '';
99+
}
100+
101+
public function describe(): string
102+
{
103+
return $this->value . $this->describeAdditionalCacheKey();
104+
}
105+
106+
}
107+
108+
/**
109+
* @api
110+
*/
111+
final class FinalApiGetter
112+
{
113+
114+
private string $value;
115+
116+
public function __construct(string $value)
117+
{
118+
$this->value = $value;
119+
}
120+
121+
public function getValue(): string
122+
{
123+
return $this->value;
124+
}
125+
126+
}
127+
128+
final class ReadsOthers
129+
{
130+
131+
private ClosedWorldGetter $closedWorld;
132+
133+
public function __construct(ClosedWorldGetter $closedWorld)
134+
{
135+
$this->closedWorld = $closedWorld;
136+
}
137+
138+
private function getClosedWorld(): ClosedWorldGetter
139+
{
140+
return $this->closedWorld;
141+
}
142+
143+
public function doSomething(FinalGetter $finalGetter, ClosedWorldGetter $closedWorldGetter, ApiGetter $apiGetter, FinalApiGetter $finalApiGetter): string
144+
{
145+
return $finalGetter->getValue()
146+
. $closedWorldGetter->getValue()
147+
. $closedWorldGetter->getOverriddenValue()
148+
. $closedWorldGetter->getFinalValue()
149+
. $apiGetter->getValue()
150+
. $finalApiGetter->getValue()
151+
. $this->getClosedWorld()->getValue();
152+
}
153+
154+
}

0 commit comments

Comments
 (0)