Improving support for anonymous classes by mamazu · Pull Request #2613 · phpactor/phpactor

mamazu · 2024-03-22T19:54:14Z

Things that have been done

Completion for it works (tests added)
Check if there is a better way to generate the name of the anonymous classes

Limitations:

The current implementation uses the byteoffset of the class. Meaning that the classname could change if there are textedits that change the text before the class
This doesn't go into ClassDeclarationNodes so anoymous classes inside a class wouldn't be recognized?

lib/WorseReflection/Core/Inference/Resolver/ObjectCreationExpressionResolver.php

mamazu · 2024-03-22T19:59:06Z

lib/WorseReflection/Core/Reflection/Collection/ReflectionClassLikeCollection.php

+            }
+
+            if ($child instanceof ObjectCreationExpression) {
+                // TODO: come up with a good way to generate a class name


We need a better way to generate class names as the code is currently duplicated everywhere.

PHPStan does it like this: https://github.com/dantleech/phpstan-src/blob/f7b89d47cfa1fa636fb571b4315841a9af0e976f/src/Broker/AnonymousClassNameHelper.php#L22-L25

but I don't think this is the correct place to do it.

We can create the reflection class in the resolver?

Yeah, technically using the line number is not 100% garanteed to be unique but who has two anon classes on the same line. :D

But what you mean with "creating the reflection class in the resolver"? I wanted to try to avoid making a ReflectionAnonymousClass class as anon classes should be the same except for their weird name.

Yes you are right. A different approach for this would probably make more sense but that would require a lot of refactoring as the fromNode function is used quite a lot.

I don't think it needs refactoring and would also fix the issue Thomas raised below, but would need to show an example.

lib/WorseReflection/Core/Inference/Resolver/ObjectCreationExpressionResolver.php

lib/WorseReflection/Core/Reflection/Collection/ClassLikeReflectionMemberCollection.php

lib/WorseReflection/Core/Inference/Resolver/ObjectCreationExpressionResolver.php

przepompownia · 2024-03-25T22:07:48Z

From #2612 perspective this seems to be one step ahead but:

mamazu · 2024-03-25T22:12:11Z

What were you trying to do? Generate a class?

przepompownia · 2024-03-25T22:13:34Z

"Implement contracts" like in #2612

mamazu · 2024-03-25T23:02:05Z

Now it should work (at least the test for it is green) and it looks like the rest broke.

przepompownia · 2024-03-25T23:23:21Z

Works for me:

dantleech · 2024-03-29T09:02:37Z

lib/CodeBuilder/Adapter/TolerantParser/Updater/ClassMethodUpdater.php

-            return $classNode->classMembers;
+        if ($classNode instanceof ClassDeclaration || $classNode instanceof ObjectCreationExpression) {
+            $classNode = $classNode->classMembers;
+            Assert::isInstanceOf( $classNode, ClassMembersNode::class);


is it expected that this is NOT a ClassMembersNode? pm am ObjectCreationExpression it can be NULL, and for ClassDeclaration tolerant parser often lies about nullability. this will cause a runtime exception, which is probably not what we want.

if it should never happen then this should be an exception and I'd prefer not to use Assertion libraries.

Yeah, the problem in this case is that it could also be a "normal" Object creation with a class name which would then crash here. But in this case I'm not sure what the best way to go about this is.

dantleech · 2024-03-29T09:06:39Z

lib/WorseReflection/Core/Util/NodeUtil.php

        if ($nodeOrToken instanceof Token) {
            return (string)$nodeOrToken->getText($node->getFileContents());
        }
+        if ($nodeOrToken instanceof ObjectCreationExpression) {


this is method is explicitly about getting the name frm a Node or Token, it's not about annonymous classes. you can add a separate method or probably there is something better.

lib/WorseReflection/Bridge/TolerantParser/Reflection/ReflectionClass.php

dantleech · 2024-03-29T09:12:56Z

lib/WorseReflection/Bridge/TolerantParser/Reflection/ReflectionClass.php

    public function memberListPosition(): ByteOffsetRange
    {
+        $classMembers = $this->node->classMembers;
+        assert($classMembers instanceof ClassMembersNode, 'ObjectCreationExpression does not contain anonymous class');


presumably thid can happen and this will crash? can we return defaults instead based on the node position?

dantleech · 2024-03-29T09:15:20Z

lib/WorseReflection/Bridge/TolerantParser/Reflection/ReflectionMethod.php

+        $classDeclaration = $this->node->getFirstAncestor(ClassLike::class, ObjectCreationExpression::class);
+        if ($classDeclaration instanceof ObjectCreationExpression) {
+            return $this->class ?? $this->serviceLocator->reflector()
+                ->reflectClassLike(NodeUtil::nameFromTokenOrNode($classDeclaration, $classDeclaration));


the reflector has no way to know how to locate this, so this is working based on caching? does it work without the cache?

I don't know how the reflector does it. But I assume it cashes the classes that it has seen yes. Where would the logic be for finding it properly? Do we need to index anonymous classes for that?

lib/WorseReflection/Bridge/TolerantParser/Reflection/TraitImport/TraitImports.php

dantleech · 2024-03-29T09:20:25Z

lib/WorseReflection/Core/Inference/Resolver/ObjectCreationExpressionResolver.php

+            $node->getEndPosition(),
+            [
+                'symbol_type' => Symbol::CLASS_,
+                'type' => TypeFactory::reflectedClass($resolver->reflector(), NodeUtil::nameFromTokenOrNode($node, $node))


again, i'm not sure how this works, and assume it works based on the anonymous class previously being parsed? we should avoid depending on the cache.

see Roave/BetterReflection#300

probably the reflector should support accepting the Node instead of a "name".

lib/WorseReflection/Core/Reflection/Collection/ClassLikeReflectionMemberCollection.php

dantleech · 2024-03-29T09:23:32Z

lib/WorseReflection/Core/Reflection/Collection/ReflectionClassLikeCollection.php

+                continue;
+            }
+
+            if ($child instanceof ObjectCreationExpression && !($child->classTypeDesignator instanceof Node)) {


this may cause unexpected issues. "reflect classes in" reflects locatable symbols and also it should not need to deeply traverse the AST.

we could consider removing this and instead adding support to $reflector->reflect{Node,AnnonymousClass,Object}($node) for example.

The problem is this approach won't find all classes anyways. Because we actually have to traverse into class definitions as well. Otherwise we would miss anonymous classes inside other classes. But I don't have an idea how to do improve performance here.

yes, they need probably to be interpreted inline as types as with the above comment. doing this will mean that "implement contracts" will not automatically work, but at the same time it's only working because of the cache effect in this PR

I just saw that reflectClassesIn should probably better be named reflectClassLikeIn 'cause it also returns interfaces enums etc. which are completely irrelevant for the "Implement contracts". But that's not a problem of this PR.

However I get what you're saying, I'm not sure if its based on cache in this situation because the source code builder reflects the same structure again and should also build the same list of ClassLike nodes. On the other hand I'm quite familiar with the concept of inline types and how I would go about and implement this. Do we already have an example for this?

przepompownia · 2024-04-19T09:49:13Z

At the moment I have no occasion to work with anonymous classes, but have merged the recent version of this PR into my experimental branch and can report possible side effects.

dantleech · 2024-04-19T10:32:56Z

just to be clear, it's unlikely to be merged with it's current implementation. but hopefully I can find some time to look more into it.

przepompownia · 2024-10-18T20:46:00Z

Regardless of merging into master, this change stopped working after merge de10e22 here (incompatible declarartion of reflectClassesIn in lib/WorseReflection/Bridge/TolerantParser/Reflector/TolerantSourceCodeReflector.php).

mamazu · 2024-10-18T21:19:50Z

Should work again now.

przepompownia · 2024-10-18T21:23:27Z

Works again, thank you

mamazu · 2024-12-01T12:38:25Z

I had to rebase this because I messed up the merging of this branch.

mamazu marked this pull request as draft March 22, 2024 19:54

mamazu changed the title ~~First try at implementing it~~ Improving support for anonymous classes Mar 22, 2024

mamazu commented Mar 22, 2024

View reviewed changes

This was referenced Mar 22, 2024

Implement contracts inside anonymous classes #2612

Open

Diagnostics error in anonymous class #1320

Open

Anonymous class support #663

Open

dantleech reviewed Mar 23, 2024

View reviewed changes

lib/WorseReflection/Core/Inference/Resolver/ObjectCreationExpressionResolver.php Outdated Show resolved Hide resolved

dantleech reviewed Mar 23, 2024

View reviewed changes

lib/WorseReflection/Core/Reflection/Collection/ClassLikeReflectionMemberCollection.php Outdated Show resolved Hide resolved

dantleech reviewed Mar 23, 2024

View reviewed changes

lib/WorseReflection/Core/Inference/Resolver/ObjectCreationExpressionResolver.php Outdated Show resolved Hide resolved

mamazu force-pushed the anonymous_classes branch from f8c99ee to 8e24b48 Compare March 24, 2024 18:14

mamazu marked this pull request as ready for review March 24, 2024 23:15

mamazu force-pushed the anonymous_classes branch from 4235fc4 to f32ad1b Compare March 24, 2024 23:34

mamazu force-pushed the anonymous_classes branch from 681f9c8 to 7a53a6b Compare March 25, 2024 23:07

przepompownia mentioned this pull request Mar 27, 2024

$this incorrectly detected inside object creation expression #2618

Closed

dantleech reviewed Mar 29, 2024

View reviewed changes

lib/WorseReflection/Bridge/TolerantParser/Reflection/ReflectionClass.php Outdated Show resolved Hide resolved

dantleech reviewed Mar 29, 2024

View reviewed changes

lib/WorseReflection/Bridge/TolerantParser/Reflection/TraitImport/TraitImports.php Outdated Show resolved Hide resolved

dantleech reviewed Mar 29, 2024

View reviewed changes

lib/WorseReflection/Core/Reflection/Collection/ClassLikeReflectionMemberCollection.php Outdated Show resolved Hide resolved

dantleech reviewed Mar 29, 2024

View reviewed changes

mamazu force-pushed the anonymous_classes branch from b7acb99 to da577f9 Compare April 9, 2024 09:08

mamazu force-pushed the anonymous_classes branch from 95cf6a5 to e2fec03 Compare December 1, 2024 12:33

Improving anonymous class support

3a2108e

mamazu force-pushed the anonymous_classes branch from e2fec03 to 3a2108e Compare December 1, 2024 12:37

Merge branch 'master' into anonymous_classes

6fecf48

Uh oh!

Conversation

mamazu commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

przepompownia commented Mar 25, 2024

Uh oh!

mamazu commented Mar 25, 2024

Uh oh!

przepompownia commented Mar 25, 2024

Uh oh!

mamazu commented Mar 25, 2024

Uh oh!

przepompownia commented Mar 25, 2024

Uh oh!

dantleech Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dantleech Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantleech Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

przepompownia commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dantleech commented Apr 19, 2024

Uh oh!

przepompownia commented Oct 18, 2024

Uh oh!

mamazu commented Oct 18, 2024

Uh oh!

przepompownia commented Oct 18, 2024

Uh oh!

mamazu commented Dec 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mamazu commented Mar 22, 2024 •

edited

Loading

dantleech Mar 29, 2024 •

edited

Loading

dantleech Mar 29, 2024 •

edited

Loading

dantleech Mar 29, 2024 •

edited

Loading

dantleech Apr 9, 2024 •

edited

Loading

przepompownia commented Apr 19, 2024 •

edited

Loading