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

Fix Identical type specification of constant types #1493

Merged
merged 2 commits into from Jul 10, 2022

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Jul 2, 2022

Closes phpstan/phpstan#7257

This was not working correctly because e.g. a ConstantArrayType cannot be safely used to strictly specify the other (left or right) type in the Identical comparison without further checks.

The problem with the linked issue was that it would have the same types on the left and right side and basically cancel out the types in a falsey scope (the right side of the || for e.g. $sorted leading to a NeverType), see https://phpstan.org/r/f560aac6-602f-45a0-8115-bb94f7142f79. Which explains why it works if only truthy scopes (via variable assignments) work around that.

@herndlm herndlm marked this pull request as ready for review July 2, 2022 20:28
@herndlm herndlm marked this pull request as draft July 2, 2022 20:45
@herndlm herndlm marked this pull request as ready for review July 2, 2022 21:50
src/Analyser/TypeSpecifier.php Show resolved Hide resolved
src/Analyser/TypeSpecifier.php Show resolved Hide resolved
@ondrejmirtes ondrejmirtes merged commit 135bc57 into phpstan:1.8.x Jul 10, 2022
@ondrejmirtes
Copy link
Member

Thank you very much!

@herndlm herndlm deleted the fix-7257 branch July 10, 2022 13:10
@ondrejmirtes
Copy link
Member

There are bunch of code examples in current issues where the results changed because of this PR:

Can you please go through them and see:

  • If it's A) fixing a bug or B) introducing a new bug
  • A): please send a regression test :)
  • B): please reproduce the problem in a test and fix the test :)

Thank you very much!

@herndlm
Copy link
Contributor Author

herndlm commented Jul 10, 2022

Looking good, do you have preferences in the regression tests? All in one PR/commit OK? Looks like I'll also have to create two tests for an issue or two.
I'll open them later today.

@ondrejmirtes
Copy link
Member

Each closed issue should have a separate test case, meaning testBugXyz for rule test cases, or separate file in NodeScopeResolverTest :) They can be a single commit "Regression tests".

@herndlm herndlm mentioned this pull request Jul 10, 2022
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