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 false positive when type casting in IF statement #3431

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 10, 2024

@staabm staabm marked this pull request as ready for review September 10, 2024 19:33
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@@ -63,6 +63,12 @@ function castInt($x, string $s, bool $b) {
assertType('false', $b);
}

if ((int) $s) {
assertType('string', $s); // could be *NEVER*
Copy link
Contributor

@mvorisek mvorisek Sep 10, 2024

Choose a reason for hiding this comment

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

reachable for 1 or 01, numeric-string&non-falsy-string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you

Copy link
Contributor Author

@staabm staabm Sep 10, 2024

Choose a reason for hiding this comment

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

we reach ELSE for '0.1' which is a numeric string - https://3v4l.org/NeugB

Copy link
Contributor

@mvorisek mvorisek Sep 10, 2024

Choose a reason for hiding this comment

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

and not for 1.5 - https://3v4l.org/VBPn2 - so string is best or something like string~(''|'0')?

also notice 1x - https://3v4l.org/PGs9f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

string~(''|'0') is non-falsy-string. but I think we are in crazy territory land where string should be good enough ;)

@@ -134,10 +134,6 @@ public function testBug11674(): void
'Elseif condition is always false.',
28,
],
[
'Elseif condition is always false.',
36,
Copy link
Member

Choose a reason for hiding this comment

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

Took me a while but I found the bad boy for which the elseif can be false: https://3v4l.org/p1sON

@ondrejmirtes
Copy link
Member

I think this logic in TypeSpecifier makes more sense. Before the issue we didn't know that we can't compare all the casts to != false. So it makes sense to separate the conditions like this.

We lost precision in a few cases but now it's up to == / != resolution logic in TypeSpecifier to make them more precise again. We don't need to do that now of course.

@ondrejmirtes ondrejmirtes merged commit 05ce763 into phpstan:1.12.x Sep 11, 2024
397 of 413 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

@staabm staabm deleted the b11674 branch September 11, 2024 07:45
@staabm
Copy link
Contributor Author

staabm commented Sep 11, 2024

thank you!

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.

None yet

4 participants