Skip to content
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

Phpstan parser #12

Open
wants to merge 19 commits into
base: 2.0.x
Choose a base branch
from

Conversation

patrickkusebauch
Copy link
Contributor

@patrickkusebauch patrickkusebauch commented Mar 15, 2024

The current run on deptrac itself finds new dependencies not found with the old method when calling parent::method() by resolving the parent to the actual class.

Big thanks to Tomas Votruba for Rector and his blog as it allowed me by example to integrate PHPStan DIC to deptrac.

While it does not add much on its own, I see how this might allow us to resolve many open issues in the future, like the oldest open issue qossmic/deptrac#449.

Missing:

  • Documentation
  • Feature flag to switch between the old Nikic parser and PHPStan parser.
  • PHPStan PHAR (re)scoping

patrickkusebauch and others added 7 commits March 29, 2024 18:39
Current run on deptrac itself finds new dependencies not found with the old method when calling `parent::method()` by resolving the `parent` to the actual class.

There is 10_000 things surely wrong with the implementation that needs fixing in the next passes, there are no tests, BUT, it works.

Big thanks to Tomas Votruba for Rector and his blog as it allowed me by example to integrate PHPStan DIC to deptrac.
…ing and leave creating references to dedicated extractors.

As a consequence the FileReferenceVisitor code is way more readable now.
Allows for much cleaner extractor implementations. Similar to how PHPStan custom rules work. Also allows us to call extractors only when they apply to the node in question.
Fix: composer-dependency-analyser: improve config, update

* Fix filepath

* 1.4.0
…covering symbols via PHPStan.

This seems to be working?
@patrickkusebauch patrickkusebauch marked this pull request as ready for review March 29, 2024 18:41
@patrickkusebauch
Copy link
Contributor Author

I am ready to tentatively declare this PR as ready for review. There are still some small things missing (see main comment), but the business logic is implemented.

'Foo\Bar:30 on Foo\SomeClass',
'Foo\Bar:32 on Foo\SomeClass',
'Foo\Bar:36 on Foo\string2',
'Foo\Bar:38 on string', // todo: This is a bug in the parser for FQ "\string"
Copy link
Contributor Author

@patrickkusebauch patrickkusebauch Mar 29, 2024

Choose a reason for hiding this comment

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

Here we have an undiscovered bug where \string is treated as class string instead of the scalar type string.

@patrickkusebauch
Copy link
Contributor Author

I am at a loss with regards to how to scope PhpStan. Any help would be appreciated.

@gennadigennadigennadi
Copy link
Member

I am at a loss with regards to how to scope PhpStan. Any help would be appreciated.

Right now I can not help with this, but I have seen that rector does not scoped phpstan.

@patrickkusebauch
Copy link
Contributor Author

I am at a loss with regards to how to scope PhpStan. Any help would be appreciated.

Right now I can not help with this, but I have seen that rector does not scoped phpstan.

As far as I have discerned, Rector does things differently than we do. It does not scope itself, It only scopes its dependencies. And then from those it excludes stuff like PHPStan and PHPParser.

Furthermore, Rector is not distributed as a PHAR, only as a composer dependency, so there is not one PHIVE installable file to download.

We can look into going that route, but it will change how we distribute Deptrac.

@olsavmic
Copy link

@patrickkusebauch Is the main issue with rescoping the PHPStan the composer autoloader configuration or have you encountered something else?

Wild idea: what about releasing the PHPStan parser variant as a deptrac "extension"?

This would mean that the default distribution is not affected, the design of deptrac is still correctly modular, yet we can experiment with the benefits of PHPStan parser. Such extension would include dependency on phpstan, and to use it, you would have to reconfigure a few services in the configuration file.

@patrickkusebauch
Copy link
Contributor Author

@olsavmic it is strictly with rescoping PHPStan and getting it to work with Deptrac's own scoping.

Thinking about an extension - I might work. I would require to still merge some of the refactoring done in this PR as well as Deptrac exposing a lot of classes that are currently internal into the Contract namespace. Not sure what would be the benefit. It would create two parallel development paths to keep the functionality consistent between the two. I do not see this as an alternative approach, but strictly as an upgrade to Deptrac. The only reason that I implemented it as a feature flag and kept the original implementation is because I wanted to run them alongside for some time to iron out any kinks in the implementation.

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

Successfully merging this pull request may close these issues.

4 participants