Support multiple anonymous class definitions on the same line #3328
Conversation
tscni
force-pushed
the
fix/issue-11511
branch
2 times, most recently
from
c5f05a6
to
70a8982
Compare
|
It would probably be suitable to adjust this as well: But adding the file position there isn't very helpful to users. Maybe a stateful node visitor might be a good approach for that. |
|
The problem here is that the My idea for the fix: a node visitor that would add some kind of attribute on 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 The same would apply to the display name in ClassReflection. |
|
This pull request has been marked as ready for review. |
| /** @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); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm aware of this, but it's fine, the number should be low enough not to cause confusion with columns.
| @@ -201,7 +202,6 @@ public function getAnonymousClassReflection(Node\Stmt\Class_ $classNode, Scope $ | |||
| $scopeFile, | |||
| ); | |||
| $classNode->name = new Node\Identifier($className); | |||
| $classNode->setAttribute('anonymousClass', true); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
| /** @var array<int, non-empty-list<Node\Stmt\Class_>> */ | ||
| private array $nodesPerLine = []; |
There was a problem hiding this comment.
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().
| 'AnonymousClass%s', | ||
| md5(sprintf('%s:%s', $filename, $classNode->getStartLine())), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is almost perfect. I admire that as your first contribution, you really found your way around this complex codebase so easily. You didn't need any hand-holding and figured everything by yourself. Thanks!
I have one more request: there isn't really any test proving the anonymous class display name with ATTRIBUTE_LINE_INDEX. We just see the resulting hash.
A solution would be to create a dummy rule just for test purposes, like the one in VirtualNullsafeMethodCallTest. Its goal would be to report anonymous class display name, and to show that a standalone class has just :line and more classes with :line:index.
Thank you once more!
|
Thanks for the kind words :) I've now added the test. |
|
Awesome, thank you! |
Fixes phpstan/phpstan#5597, phpstan/phpstan#11511