Skip to content

Support multiple anonymous class definitions on the same line #3328

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 22, 2024
Merged

Support multiple anonymous class definitions on the same line #3328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
27476be
Support multiple anonymous class definitions on the same line
tscni Aug 17, 2024
d04bd26
Use a line-based index to disambiguate anonymous classes
tscni Aug 19, 2024
ab3d1bd
Move anonymous class node attribute handling into AnonymousClassVisitor
tscni Aug 19, 2024
d764925
Test anonymous class reflection names
tscni Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions conf/config.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,11 @@ services:
options:
preserveOriginalNames: true

-
class: PHPStan\Parser\AnonymousClassVisitor
tags:
- phpstan.parser.richParserNodeVisitor

-
class: PHPStan\Parser\ArrayFilterArgVisitor
tags:
Expand Down
3 changes: 2 additions & 1 deletion src/Analyser/NodeScopeResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@
use PHPStan\Node\UnreachableStatementNode;
use PHPStan\Node\VariableAssignNode;
use PHPStan\Node\VarTagChangedExpressionTypeNode;
use PHPStan\Parser\AnonymousClassVisitor;
use PHPStan\Parser\ArrowFunctionArgVisitor;
use PHPStan\Parser\ClosureArgVisitor;
use PHPStan\Parser\Parser;
Expand Down Expand Up @@ -855,7 +856,7 @@ private function processStmtNode(
if ($stmt->name === null) {
throw new ShouldNotHappenException();
}
if ($stmt->getAttribute('anonymousClass', false) === false) {
if ($stmt->getAttribute(AnonymousClassVisitor::ATTRIBUTE_ANONYMOUS_CLASS, false) === false) {
$classReflection = $this->reflectionProvider->getClass($stmt->name->toString());
} else {
$classReflection = $this->reflectionProvider->getAnonymousClassReflection($stmt, $scope);
Expand Down
11 changes: 10 additions & 1 deletion src/Broker/AnonymousClassNameHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PhpParser\Node;
use PHPStan\File\FileHelper;
use PHPStan\File\RelativePathHelper;
use PHPStan\Parser\AnonymousClassVisitor;
use PHPStan\ShouldNotHappenException;
use function md5;
use function sprintf;
Expand Down Expand Up @@ -32,9 +33,17 @@ public function getAnonymousClassName(
$this->fileHelper->normalizePath($filename, '/'),
);

/** @var int|null $lineIndex */
$lineIndex = $classNode->getAttribute(AnonymousClassVisitor::ATTRIBUTE_LINE_INDEX);
if ($lineIndex === null) {
$hash = md5(sprintf('%s:%s', $filename, $classNode->getStartLine()));
} else {
$hash = md5(sprintf('%s:%s:%d', $filename, $classNode->getStartLine(), $lineIndex));
}

return sprintf(
'AnonymousClass%s',
md5(sprintf('%s:%s', $filename, $classNode->getStartLine())),
Comment on lines 45 to -37
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's not clear to me, why differ between the class name (AnonymousClass<hash>) and the display name (class@anonymous<location>) at all?
Is it a memory optimization, avoiding problematic characters in some context, or just an artifact? Or in other words, would it be a problem to align them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same turn I was thinking about setting the name already in the node visitor, but then stumbled upon PHPStan\Parser\Parser::parseString().
In theory that could be amended with an optional file name argument.

That could then lead to some minor simplifications in the handling of anonymous class names.

$hash,
);
}

Expand Down
51 changes: 51 additions & 0 deletions src/Parser/AnonymousClassVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php declare(strict_types = 1);

namespace PHPStan\Parser;

use PhpParser\Node;
use PhpParser\NodeVisitorAbstract;
use function count;

class AnonymousClassVisitor extends NodeVisitorAbstract
{

public const ATTRIBUTE_ANONYMOUS_CLASS = 'anonymousClass';
public const ATTRIBUTE_LINE_INDEX = 'anonymousClassLineIndex';

/** @var array<int, non-empty-list<Node\Stmt\Class_>> */
private array $nodesPerLine = [];
Comment on lines +15 to +16
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly concerned about keeping the node reference, especially considering that nodes could be replaced.
It seems unlikely that a node visitor might replace class nodes though (except that I propose exactly that in another comment.. :P)

Ultimately it's a performance optimization to avoid having to traverse through all nodes in afterTraverse().


public function beforeTraverse(array $nodes): ?array
{
$this->nodesPerLine = [];
return null;
}

public function enterNode(Node $node): ?Node
{
if (!$node instanceof Node\Stmt\Class_ || !$node->isAnonymous()) {
return null;
}

$node->setAttribute(self::ATTRIBUTE_ANONYMOUS_CLASS, true);
$this->nodesPerLine[$node->getStartLine()][] = $node;

return null;
}

public function afterTraverse(array $nodes): ?array
{
foreach ($this->nodesPerLine as $nodesOnLine) {
if (count($nodesOnLine) === 1) {
continue;
}
for ($i = 0; $i < count($nodesOnLine); $i++) {
$nodesOnLine[$i]->setAttribute(self::ATTRIBUTE_LINE_INDEX, $i + 1);
}
}

$this->nodesPerLine = [];
return null;
}

}
12 changes: 10 additions & 2 deletions src/Reflection/BetterReflection/BetterReflectionProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use PHPStan\File\FileHelper;
use PHPStan\File\FileReader;
use PHPStan\File\RelativePathHelper;
use PHPStan\Parser\AnonymousClassVisitor;
use PHPStan\Php\PhpVersion;
use PHPStan\PhpDoc\PhpDocInheritanceResolver;
use PHPStan\PhpDoc\StubPhpDocProvider;
Expand Down Expand Up @@ -201,7 +202,6 @@ public function getAnonymousClassReflection(Node\Stmt\Class_ $classNode, Scope $
$scopeFile,
);
$classNode->name = new Node\Identifier($className);
$classNode->setAttribute('anonymousClass', true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this into AnonymousClassVisitor felt like a reasonable step to take now that it exists.

In theory the node visitor would also offer the possibility to fix Node\Stmt\Class_::isAnonymous() by implementing an AnonymousClass extends Node\Stmt\Class_ and replacing the original node with that.
But maybe that's more complexity than desired?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea for a follow up PR! Although rules should better rely on InClassNode and ClassReflection so that they don't have to make any differences between traditional and anonymous classes, in theory someone could ask about Class_::isAnonymous() and it's always better to give the right answer :) Please take a note and send a follow up PR :)


if (isset(self::$anonymousClasses[$className])) {
return self::$anonymousClasses[$className];
Expand All @@ -214,6 +214,14 @@ public function getAnonymousClassReflection(Node\Stmt\Class_ $classNode, Scope $
null,
);

/** @var int|null $classLineIndex */
$classLineIndex = $classNode->getAttribute(AnonymousClassVisitor::ATTRIBUTE_LINE_INDEX);
if ($classLineIndex === null) {
$displayName = sprintf('class@anonymous/%s:%s', $filename, $classNode->getStartLine());
} else {
$displayName = sprintf('class@anonymous/%s:%s:%d', $filename, $classNode->getStartLine(), $classLineIndex);
}
Comment on lines +217 to +223
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there was just a single class on the same line, the name generated by AnonymousClassNameHelper would not change. If there were two, the name would be line:order, so 24:1 and 24:2 for example.

The same would apply to the display name in ClassReflection.

I've done this now and am personally happy with it.

But a small notes on this:
E.g. path/to/file.php:12:34 would usually indicate that something is located in file path/to/file.php at line 12 at column / character 34
In that regard this seems slightly unusual and might potentially lead to some confusion. If we do care to solve this (I don't mind it), maybe $ as a separator, like the one PHP uses, might be a good alternative.
We could alternatively also use the character position of course, but that seems a bit more cumbersome to add in contexts without the reflection logic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware of this, but it's fine, the number should be low enough not to cause confusion with columns.


self::$anonymousClasses[$className] = new ClassReflection(
$this->reflectionProviderProvider->getReflectionProvider(),
$this->initializerExprTypeResolver,
Expand All @@ -227,7 +235,7 @@ public function getAnonymousClassReflection(Node\Stmt\Class_ $classNode, Scope $
$this->classReflectionExtensionRegistryProvider->getRegistry()->getAllowedSubTypesClassReflectionExtensions(),
$this->classReflectionExtensionRegistryProvider->getRegistry()->getRequireExtendsPropertyClassReflectionExtension(),
$this->classReflectionExtensionRegistryProvider->getRegistry()->getRequireExtendsMethodsClassReflectionExtension(),
sprintf('class@anonymous/%s:%s', $filename, $classNode->getStartLine()),
$displayName,
new ReflectionClass($reflectionClass),
$scopeFile,
null,
Expand Down
5 changes: 3 additions & 2 deletions src/Type/FileTypeMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use PHPStan\BetterReflection\Util\GetLastDocComment;
use PHPStan\Broker\AnonymousClassNameHelper;
use PHPStan\File\FileHelper;
use PHPStan\Parser\AnonymousClassVisitor;
use PHPStan\Parser\Parser;
use PHPStan\PhpDoc\PhpDocNodeResolver;
use PHPStan\PhpDoc\PhpDocStringResolver;
Expand Down Expand Up @@ -260,7 +261,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
}

$className = $this->anonymousClassNameHelper->getAnonymousClassName($node, $fileName);
} elseif ((bool) $node->getAttribute('anonymousClass', false)) {
} elseif ((bool) $node->getAttribute(AnonymousClassVisitor::ATTRIBUTE_ANONYMOUS_CLASS, false)) {
$className = $node->name->name;
} else {
if ($traitFound) {
Expand Down Expand Up @@ -451,7 +452,7 @@ function (Node $node) use ($fileName, $lookForTrait, $phpDocNodeMap, &$traitFoun
}

$className = $this->anonymousClassNameHelper->getAnonymousClassName($node, $fileName);
} elseif ((bool) $node->getAttribute('anonymousClass', false)) {
} elseif ((bool) $node->getAttribute(AnonymousClassVisitor::ATTRIBUTE_ANONYMOUS_CLASS, false)) {
$className = $node->name->name;
} else {
if ($traitFound) {
Expand Down
21 changes: 21 additions & 0 deletions tests/PHPStan/Analyser/AnalyserIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1416,6 +1416,27 @@ public function testBug11297(): void
$this->assertNoErrors($errors);
}

public function testBug5597(): void
{
if (PHP_VERSION_ID < 80000) {
$this->markTestSkipped('Test requires PHP 8.0.');
}

$errors = $this->runAnalyse(__DIR__ . '/data/bug-5597.php');
$this->assertNoErrors($errors);
}

public function testBug11511(): void
{
if (PHP_VERSION_ID < 80000) {
$this->markTestSkipped('Test requires PHP 8.0.');
}

$errors = $this->runAnalyse(__DIR__ . '/data/bug-11511.php');
$this->assertCount(1, $errors);
$this->assertSame('Access to an undefined property object::$bar.', $errors[0]->getMessage());
}

/**
* @param string[]|null $allAnalysedFiles
* @return Error[]
Expand Down
38 changes: 38 additions & 0 deletions tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8409,6 +8409,44 @@ public function testAnonymousClassNameInTrait(
);
}

public function dataAnonymousClassNameSameLine(): array
{
return [
[
'AnonymousClass0d7d08272ba2f0a6ef324bb65c679e02',
'$foo',
'$bar',
],
[
'AnonymousClass464f64cbdca25b4af842cae65615bca9',
'$bar',
'$baz',
],
[
'AnonymousClassa9fb472ec9acc5cae3bee4355c296bfa',
'$baz',
'die',
],
];
}

/**
* @dataProvider dataAnonymousClassNameSameLine
*/
public function testAnonymousClassNameSameLine(
string $description,
string $expression,
string $evaluatedPointExpression,
): void
{
$this->assertTypes(
__DIR__ . '/data/anonymous-class-name-same-line.php',
$description,
$expression,
$evaluatedPointExpression,
);
}

public function dataDynamicConstants(): array
{
return [
Expand Down
5 changes: 5 additions & 0 deletions tests/PHPStan/Analyser/data/anonymous-class-name-same-line.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?php

namespace AnonymousClassNameSameLine;

$foo = new class {}; $bar = new class {}; $baz = new class {}; die;
10 changes: 10 additions & 0 deletions tests/PHPStan/Analyser/data/bug-11511.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php declare(strict_types = 1); // lint >= 8.0

namespace Bug11511;

$myObject = new class (new class { public string $bar = 'test'; }) {
public function __construct(public object $foo)
{
}
};
echo $myObject->foo->bar;
25 changes: 25 additions & 0 deletions tests/PHPStan/Analyser/data/bug-5597.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php declare(strict_types = 1); // lint >= 8.0

namespace Bug5597;

interface InterfaceA {}

class ClassA implements InterfaceA {}

class ClassB
{
public function __construct(
private InterfaceA $parameterA,
) {
}

public function test() : InterfaceA
{
return $this->parameterA;
}
}

$classA = new class() extends ClassA {};
$thisWorks = new class($classA) extends ClassB {};

$thisFailsWithTwoErrors = new class(new class() extends ClassA {}) extends ClassB {};
105 changes: 105 additions & 0 deletions tests/PHPStan/Reflection/AnonymousClassReflectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
<?php declare(strict_types = 1);

namespace PHPStan\Reflection;

use PhpParser\Node;
use PhpParser\Node\Stmt\Class_;
use PHPStan\Analyser\Scope;
use PHPStan\Parser\AnonymousClassVisitor;
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Testing\RuleTestCase;
use function implode;
use function sprintf;

/**
* @extends RuleTestCase<Rule<Class_>>
*/
class AnonymousClassReflectionTest extends RuleTestCase
{

/**
* @return Rule<Class_>
*/
protected function getRule(): Rule
{
return new /** @implements Rule<Class_> */ class (self::createReflectionProvider()) implements Rule {

public function __construct(private ReflectionProvider $reflectionProvider)
{
}

public function getNodeType(): string
{
return Class_::class;
}

public function processNode(Node $node, Scope $scope): array
{
if (!(bool) $node->getAttribute(AnonymousClassVisitor::ATTRIBUTE_ANONYMOUS_CLASS)) {
return [];
}

$classReflection = $this->reflectionProvider->getAnonymousClassReflection($node, $scope);

return [
RuleErrorBuilder::message(sprintf(
"name: %s\ndisplay name: %s",
$classReflection->getName(),
$classReflection->getDisplayName(),
))->identifier('test.anonymousClassReflection')->build(),
];
}

};
}

public function testReflection(): void
{
$this->analyse([__DIR__ . '/data/anonymous-classes.php'], [
[
implode("\n", [
'name: AnonymousClass0c307d7b8501323d1d30b0afea7e0578',
'display name: class@anonymous/tests/PHPStan/Reflection/data/anonymous-classes.php:5',
]),
5,
],
[
implode("\n", [
'name: AnonymousClassa16017c480192f8fbf3c03e17840e99c',
'display name: class@anonymous/tests/PHPStan/Reflection/data/anonymous-classes.php:7:1',
]),
7,
],
[
implode("\n", [
'name: AnonymousClassd68d75f1cdac379350e3027c09a7c5a0',
'display name: class@anonymous/tests/PHPStan/Reflection/data/anonymous-classes.php:7:2',
]),
7,
],
[
implode("\n", [
'name: AnonymousClass75aa798fed4f30306c14dcf03a50878c',
'display name: class@anonymous/tests/PHPStan/Reflection/data/anonymous-classes.php:7:3',
]),
7,
],
[
implode("\n", [
'name: AnonymousClass4fcabdc52bfed5f8c101f3f89b2180bd',
'display name: class@anonymous/tests/PHPStan/Reflection/data/anonymous-classes.php:9:1',
]),
9,
],
[
implode("\n", [
'name: AnonymousClass0e77d7995f4c47dcd5402817970fd7e0',
'display name: class@anonymous/tests/PHPStan/Reflection/data/anonymous-classes.php:9:2',
]),
9,
],
]);
}

}
Loading
Loading