Skip to content

add failing tests#1330

Closed
rajyan wants to merge 1 commit intophpstan:1.7.xfrom
rajyan:fix/issue-6402
Closed

add failing tests#1330
rajyan wants to merge 1 commit intophpstan:1.7.xfrom
rajyan:fix/issue-6402

Conversation

@rajyan
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Development

Successfully merging this pull request may close these issues.

2 participants