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

add failing tests #1330

Closed
wants to merge 1 commit into from
Closed

add failing tests #1330

wants to merge 1 commit into from

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented May 19, 2022

Just to start with phpstan/phpstan#6402

I'm not sure all these test are solvable or not.

@ondrejmirtes
Copy link
Member

Hi, I have ideas about this, but it needs a much bigger picture - I'll introduce to the whole picture in this comment :)

So, there are some related bugs/feature requests:

  • The linked issue False positive "Readonly property is already assigned" in if-else phpstan#6402 - shouldn't report an error but currently does
  • Being able to detect when the readonly property is really assigned multiple times (like inside a while loop) - currently not reported
  • Being able to detect when the readonly property is only assigned in an if but not else - currently not reported
  • Being able to detect unused variables (it was written/created but later it's not read)
  • Being able to detect unused function/method parameters
  • Verify that function body really does what its conditional return type says
  • Verify that function body really does what its @phpstan-assert says (PHPDoc-based type narrowing #1317)

What all of these have in common is that you need to construct a graph of nodes with relations between them - where values are written and when they are read. Psalm has some of these features and this article (https://psalm.dev/articles/better-unused-variable-detection) talks about it.

I've kindly asked @arnaud-lb to work on this and right now it's in a form of this package: https://github.com/arnaud-lb/php-sema which should somehow be adopted in PHPStan. Not sure if we can use it directly, most likely we'll adapt the algorithms and apply them here.

The priority for me is to get rid of the false positives about readonly properties. At the same time we should be able to detect more situations where readonly property assignment is missing or there is a risk of multiple assignments.

After that is all figured out and released, we can work on the dead variable rules etc. :)

Are you interested in continuing this work? Thanks :)

@rajyan
Copy link
Contributor Author

rajyan commented May 20, 2022

Wow! The picture was much bigger than I thought.
This sounds really interesting too.
I'm currently working on #1270 (comment) mainly, I think I can look into this one after it 😄

@ondrejmirtes
Copy link
Member

Oh wow, you're definitely not picking the easy stuff 😊 I'm looking forward to that too! Thank you very much.

And remember - more smaller PRs is better than a single huge one 😊

@ondrejmirtes
Copy link
Member

Hi, I'm cleaning up old and stale PRs. I don't think we need to keep this one open :)

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