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

Retain left and right types in equal expression #1046

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Mar 3, 2022

This is not really changing much.. It's an adaption very similar to #1021

Equal expressions are, among other things, converted to Identical expressions if possible for the left and right types. If not, then no types at all are specified. This change modifies that, so that at least the left and right types are retained.

The first new test case was already working (because $int == (int) $int) is safe to be converted to an Identical expression. The second new test case is affected by this change, previously no types at all were specified. I'm mostly doing this so that we have more type information and strict impossible checks, as reported in e.g. phpstan/phpstan-webmozart-assert#32, do not fail

@ondrejmirtes
Copy link
Member

Maybe I'm too dumb today but I need to see what this changes via a NodeScopeResolverTest case :)

@herndlm
Copy link
Contributor Author

herndlm commented Mar 3, 2022

I'm also not sure yet if this makes sense, I'm still playing around with it and need some more time. Should have waited with the PR a bit longer

@ondrejmirtes ondrejmirtes force-pushed the retain-left-and-right-types-in-equal-expression branch from 3dc9167 to 3341fde Compare March 3, 2022 16:03
@herndlm herndlm force-pushed the retain-left-and-right-types-in-equal-expression branch from f266748 to ade7898 Compare March 3, 2022 16:53
@herndlm herndlm force-pushed the retain-left-and-right-types-in-equal-expression branch from fbf6c86 to 9a37023 Compare March 3, 2022 16:59
@ondrejmirtes
Copy link
Member

Alright, why does this work? 😂 Where's the exception to treat == and === differently for objects?

@herndlm
Copy link
Contributor Author

herndlm commented Mar 3, 2022

Yeah, let me get back to that at a later time, I can't figure this out today I'm afraid
maybe it is an indication that this all is horribly wrong? :D

@ondrejmirtes
Copy link
Member

Maybe first open a PR that just adds the test cases that already work on master (and comment those that do not work). Then the situation will be more transparent maybe.

@herndlm
Copy link
Contributor Author

herndlm commented Mar 3, 2022

I'm not quite sure I understand what the problem is? What do you think should be different here in regad to == vs ===? To me it still makes sense, but maybe I'm overlooking something.
The current difference to master are those 2 *NEVER* per file.

@ondrejmirtes
Copy link
Member

It makes sense but I have no idea why it works.

@herndlm
Copy link
Contributor Author

herndlm commented Mar 3, 2022

AFAIK $a === $a and $a == $a return no sure and sureNotTypes on master at all. But here it returns at least that $a is stdClass. Which is already known in the scope of course, but now it can be used in a falsey scope for the if and turned into sureNotTypes. I think. I'd need to double-check with the debugger again. But I'm pretty sure I saw something like that.

@ondrejmirtes
Copy link
Member

Ah, so the same expression on left/right creates a contradiction inside the SpecifiedTypes leading to NeverType in the else branch?

@ondrejmirtes
Copy link
Member

That'd make sense :)

@herndlm
Copy link
Contributor Author

herndlm commented Mar 3, 2022

I did confirm this now.

For the true scope / if branch we get the stdClass ObjectType as left and right sureTypes. And those are unionized. No surprises there.

For the false scope / else branch we get the stdClass ObjectType as left and right sureNotTypes. And those are normalized to NeverTypes (using your improved normalization which makes use of the scope and removes stdClass from stdClass) which are then intersected.

@ondrejmirtes
Copy link
Member

Great, I'll merge this if you mark it as ready :)

@herndlm herndlm marked this pull request as ready for review March 3, 2022 22:05
@ondrejmirtes ondrejmirtes merged commit a84465b into phpstan:master Mar 3, 2022
@ondrejmirtes
Copy link
Member

Thank you.

@herndlm herndlm deleted the retain-left-and-right-types-in-equal-expression branch March 3, 2022 22:05
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