[DX] Make ClassAnnotationMatcher differentiate between known and unknown classes #2319

dritter · 2022-05-15T12:47:32Z

Before this change, a call to ClassAnnotationMatcher->resolveTagFullyQualifiedName() could not distinguish between a known and unknown class. This is relevant for rules that transform @var annotations to type hints (@var annotation may point to an unknown class). Internally resolveTagFullyQualifiedName() delegates to UseImportNameMatcher->matchNameWithUses(), which simply returns the original string if not found. If this happens, the rule cannot tell, if this is a global class, or an unknown one.

This PR changes ClassAnnotationMatcher->resolveTagFullyQualifiedName() to return null if the class is not known.
There is one caveat: If the unknown class was imported anyway, we do not detect this case (see ClassAnnotationMatcher/Fixture/fixture.php.inc#L18-L19). IMHO such a case is better handled in a separate rule..

Background:
I stumbled upon this when working on Add Rule to convert Doctrine String Classes in targetEntity to Class Constant , where a targetEntity might point to an unknown (or global) class.

.../BetterPhpDocParser/PhpDocParser/ClassAnnotationMatcher/ResolveTagFullyQualifiedNameTest.php

dritter · 2022-05-28T20:43:02Z

@samsonasik Tests are green now. Could you take another look?

packages/BetterPhpDocParser/PhpDocParser/ClassAnnotationMatcher.php

samsonasik · 2022-05-28T21:02:53Z

packages-tests/BetterPhpDocParser/PhpDocParser/ClassAnnotationMatcher/Fixture/fixture.php.inc

+    /** @var \My\Namespace\UnknownClass */
+    private $unknownNamespacedClass;
+//    /** @var OtherUnknownClass */
+//    private $unknownUsedClass;


why it is commented?

This is a testcase we currently cannot cover because the OtherUnknownClass is imported (= used). So, this is a case we cannot detect properly. As far as I know, PHP ignores use statements pointing to non existant classes.
I could have removed this case, but I think it is worth mentioning. I'll write a helpful comment..

How about create a test case for direct rector rule usage? eg, for DoctrineTargetEntityStringToClassConstantRector usage.

I think that will be clearer for the purpose and use case.

You can add the test case to tests/Issues directory as the rule is from rector-doctrine

Well, the test is now specific for name resolution. I mean, yes, DoctrineTargetEntityStringToClassConstantRector will benefit from this feature, but other users of ClassAnnotationMatcher->resolveTagFullyQualifiedName() could benefit as well. So, I suggest to keep this test at a central place (here).

I am not sure without real failing test case example in a rule, docblock is depends on the statement next to it, so it will be still read by next stmt (as property), possibly by children of VarTagValueNode

Alright. I changed the code to check if the class really exists and added some more tests. WDYT?

Could you add test for usage in rule, eg: on DoctrineTargetEntityStringToClassConstantRector?

You can add the test case to tests/Issues directory as the rule is from rector-doctrine

Ah. Now I understood what you meant in the first place. Apologies for getting you wrong and thanks for your patience. I added a Testcase under tests/Issues/DoNotReplaceUnknownTargetEntities. This test fails, because the DoctrineTargetEntityStringToClassConstantRector needs to use ClassAnnotationMatcher->resolveTagToKnownFullyQualifiedName() now.

could you make test passed then?

I made the tests reflect the status quo for now, so they pass. IMHO, it would be better to mark them as skipped, but I couldn't figure out how to do that.
I'll create other PRs in rector-doctrine to fix them properly.

tests/Issues/DoNotReplaceUnknownTargetEntities/Fixture/unknown_target_entity.php.inc

samsonasik

Ok for me 👍

TomasVotruba · 2022-06-02T16:36:33Z

packages-tests/BetterPhpDocParser/PhpDocParser/ClassAnnotationMatcher/Fixture/fixture.php.inc

+    private $unknownAliasedUsedClass;
+    /** @var \UnknownGlobalClass */
+    private $unknownGlobalClass;
+}


Could you split the test case by shared topic?
It is easier to debug, when one of them breaks.

Feel free to nest those in /Fixture/NonExistingClass directory.

Not sure why phpstan fails. I get no error, when run locally..

@dritter see deprecated-packages/symplify#4110

I rebased to the latest main and phpstan passes now.

…lasses

# Conflicts: # packages/BetterPhpDocParser/PhpDocParser/ClassAnnotationMatcher.php

…stantRector

…_target_entity.php.inc Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>

TomasVotruba · 2022-06-06T08:32:29Z

Thank you @dritter for extensive work on this 👏

samsonasik reviewed May 15, 2022

View reviewed changes

.../BetterPhpDocParser/PhpDocParser/ClassAnnotationMatcher/ResolveTagFullyQualifiedNameTest.php Outdated Show resolved Hide resolved

dritter force-pushed the fix_unknown_classes_not_recognized branch 3 times, most recently from 45fb95a to 259d044 Compare May 28, 2022 20:36

dritter commented May 28, 2022

View reviewed changes

packages/BetterPhpDocParser/PhpDocParser/ClassAnnotationMatcher.php Outdated Show resolved Hide resolved

samsonasik reviewed May 28, 2022

View reviewed changes

dritter requested a review from TomasVotruba as a code owner May 30, 2022 17:35

dritter force-pushed the fix_unknown_classes_not_recognized branch from 19489bc to be78bf6 Compare May 30, 2022 20:13

samsonasik reviewed May 31, 2022

View reviewed changes

tests/Issues/DoNotReplaceUnknownTargetEntities/Fixture/unknown_target_entity.php.inc Outdated Show resolved Hide resolved

samsonasik reviewed May 31, 2022

View reviewed changes

tests/Issues/DoNotReplaceUnknownTargetEntities/Fixture/unknown_target_entity.php.inc Outdated Show resolved Hide resolved

samsonasik reviewed May 31, 2022

View reviewed changes

tests/Issues/DoNotReplaceUnknownTargetEntities/Fixture/unknown_target_entity.php.inc Outdated Show resolved Hide resolved

samsonasik approved these changes Jun 1, 2022

View reviewed changes

TomasVotruba reviewed Jun 2, 2022

View reviewed changes

dritter added 5 commits June 5, 2022 20:07

Make ClassAnnotationMatcher differentiate between known and unknown c…

4332366

…lasses

Make use of NodeNameResolver

c73c7fb

Avoid BC break by adding an optional parameter in ClassAnnotationMatcher

ca1c0be

Add helpful comment

5173d2a

Resolve class only if it exists

ec287f5

dritter force-pushed the fix_unknown_classes_not_recognized branch from 7205dd1 to 80f91f2 Compare June 5, 2022 18:12

dritter added 10 commits June 5, 2022 20:13

Detect Class Aliases pointing to unknown Classes

9dee932

# Conflicts: # packages/BetterPhpDocParser/PhpDocParser/ClassAnnotationMatcher.php

Reduce complexity of ClassAnnotationMatcher

663991a

Do not enforce resolve to null on unknown classes

6edbf40

Add more test cases

82defda

Avoid boolean switch in exposed methods

10accf5

Handle absolute classes in ClassAnnotationMatcher

67b52b5

Rename test to reflect tested method

ea3cf21

Add test for sibling class without explicit namespace

0c8bc6c

Add failing integration Test for DoctrineTargetEntityStringToClassCon…

a5f3c60

…stantRector

Improve naming of Test

79d4a48

dritter and others added 7 commits June 5, 2022 20:13

Update tests/Issues/DoNotReplaceUnknownTargetEntities/Fixture/unknown…

392cb66

…_target_entity.php.inc Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>

Update tests/Issues/DoNotReplaceUnknownTargetEntities/Fixture/unknown…

ce67d9b

…_target_entity.php.inc Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>

Rename fixtures

b174b3e

Rename Test

4e9e6da

Add another test case

43b9094

Make tests reflect the status quo for now

bb9c74d

Split up tests for easier debugging

80f91f2

TomasVotruba merged commit df5bd5f into rectorphp:main Jun 6, 2022

This was referenced Jul 13, 2022

Resolve Doctrine Target Entities to known Classes only rectorphp/rector-doctrine#111

Merged

Fix test to only resolve to known classes #2663

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DX] Make ClassAnnotationMatcher differentiate between known and unknown classes #2319

[DX] Make ClassAnnotationMatcher differentiate between known and unknown classes #2319

dritter commented May 15, 2022

dritter commented May 28, 2022

samsonasik May 28, 2022

dritter May 28, 2022

samsonasik May 28, 2022

dritter May 29, 2022

samsonasik May 29, 2022

dritter May 31, 2022

samsonasik May 31, 2022

dritter May 31, 2022

samsonasik May 31, 2022

dritter Jun 1, 2022

samsonasik left a comment

TomasVotruba Jun 2, 2022

dritter Jun 2, 2022

dritter Jun 2, 2022

samsonasik Jun 2, 2022

dritter Jun 6, 2022

TomasVotruba commented Jun 6, 2022

[DX] Make ClassAnnotationMatcher differentiate between known and unknown classes #2319

[DX] Make ClassAnnotationMatcher differentiate between known and unknown classes #2319

Conversation

dritter commented May 15, 2022

dritter commented May 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsonasik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba commented Jun 6, 2022