Skip to content

Fix FileVisitor issues with wrong class detection #508

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hgraca
Copy link
Contributor

@hgraca hgraca commented May 20, 2025

It seems the php parser is using phpdocs and deciding that
something like
/**

  • @return array<int, string|int>
  • /
    is a class with FQCN My\Namespace\array<int, string|int> which then ofc fails to create a FullyQualifiedClassName.

@micheleorselli
Copy link
Member

Hey @hgraca, thank you for reporting that 🙇. I guess this depends on the new DocblockParser and related classes. We'll try to look into it asap, in the meantime, if you want to give it a try the fix should happen in the DocblockParser and/or DocblockTypesResolver classes.

@micheleorselli
Copy link
Member

#510 should fix the problem

Copy link
Member

@AlessandroMinoccheri AlessandroMinoccheri left a comment

Choose a reason for hiding this comment

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

LGTM

@hgraca
Copy link
Contributor Author

hgraca commented May 28, 2025

@micheleorselli thank you for fixing the bug, is this PR still worth merging or shall I close it?

It seems the php parser is using phpdocs and deciding that
 something like
/**
 * @return array<int, string|int>
 */
is a class with FQCN `My\Namespace\array<int, string|int>` which
then ofc fails to create a FullyQualifiedClassName.
@hgraca
Copy link
Contributor Author

hgraca commented May 28, 2025

@micheleorselli
Actually I tested now with your latest and without my commit and I still get this error

 2731/3413 [======================>-----]  80%Parse Error: PHPUnit\Framework\Attributes\psalm-var is not a valid namespace definition#0 /app/no-vcs/lib/arkitect/src/Analyzer/ClassDependency.php(17): Arkitect\Analyzer\FullyQualifiedClassName::fromString('PHPUnit\\Framewo...')
#1 /app/no-vcs/lib/arkitect/src/Analyzer/FileVisitor.php(251): Arkitect\Analyzer\ClassDependency->__construct('PHPUnit\\Framewo...', 25)
#2 /app/no-vcs/lib/arkitect/src/Analyzer/FileVisitor.php(57): Arkitect\Analyzer\FileVisitor->handleTypedProperty(Object(PhpParser\Node\Stmt\Property))
#3 /app/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php(196): Arkitect\Analyzer\FileVisitor->enterNode(Object(PhpParser\Node\Stmt\Property))
#4 /app/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php(98): PhpParser\NodeTraverser->traverseArray(Array)
#5 /app/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php(227): PhpParser\NodeTraverser->traverseNode(Object(PhpParser\Node\Stmt\Class_))
#6 /app/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php(98): PhpParser\NodeTraverser->traverseArray(Array)
#7 /app/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php(227): PhpParser\NodeTraverser->traverseNode(Object(PhpParser\Node\Stmt\Namespace_))
#8 /app/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php(76): PhpParser\NodeTraverser->traverseArray(Array)
#9 /app/no-vcs/lib/arkitect/src/Analyzer/FileParser.php(72): PhpParser\NodeTraverser->traverse(Array)

I fixed the merge conflicts and force pushed.

@hgraca hgraca force-pushed the fix/fix-file-vistor-not-a-class branch from 0d65be6 to 9376332 Compare May 28, 2025 14:32
Copy link

Copy link

codecov bot commented May 28, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 97.53%. Comparing base (12f0a78) to head (9376332).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/Analyzer/FileVisitor.php 50.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #508      +/-   ##
============================================
- Coverage     97.75%   97.53%   -0.22%     
- Complexity      615      619       +4     
============================================
  Files            80       80              
  Lines          1778     1786       +8     
============================================
+ Hits           1738     1742       +4     
- Misses           40       44       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@micheleorselli
Copy link
Member

Hey @hgraca thank you for testing it! Based on the error it seems this is related to something different rather than the @return tag. It seems the problem is caused by @psalm-var, right? Anyway we should exclude all the docblock tags we are not interested in parsing

@micheleorselli
Copy link
Member

I'm trying to reproduce it but with no luck, would you mind sharing the snippet of the code causing problems? thanks!

@hgraca
Copy link
Contributor Author

hgraca commented Jun 8, 2025

@micheleorselli

I finally had some time to look into what situations still give problems, I found only this:

https://github.com/ramsey/uuid/blob/4.x/src/Uuid.php#L232-L234

We updated our project from php8.2 to 8.4 this week, including some libraries ofc so I guess that's why this is not exactly the same issue as before, but at the root it seems to be the same, it looks like it interprets @my-custom-doc-tag as a class FQCN.

Unfortunately I dont know what shoulod be done to solve this on phparkitect side, except for what is in this PR already :(

let me know if I can be of any further assistance.

@micheleorselli
Copy link
Member

I was able to reproduce it, this #517 should fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants