Skip to content

Narrow types based on non-strict equality with constant types#2889

Draft
staabm wants to merge 1 commit into
phpstan:1.10.xfrom
staabm:const-equal
Draft

Narrow types based on non-strict equality with constant types#2889
staabm wants to merge 1 commit into
phpstan:1.10.xfrom
staabm:const-equal

Conversation

@staabm

@staabm staabm commented Jan 25, 2024

Copy link
Copy Markdown
Contributor

@staabm
staabm changed the base branch from 1.11.x to 1.10.x January 25, 2024 13:34
@phpstan-bot

Copy link
Copy Markdown
Collaborator

You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x.

Comment on lines +31 to +42
if ($mixed == '1') {
assertType("mixed", $mixed);
}
if ($mixed == '0') {
assertType("mixed", $mixed);
}
if ($mixed == 1) {
assertType("mixed", $mixed);
}
if ($mixed == 0) {
assertType("mixed", $mixed);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

these 4 could be more precise

Suggested change
if ($mixed == '1') {
assertType("mixed", $mixed);
}
if ($mixed == '0') {
assertType("mixed", $mixed);
}
if ($mixed == 1) {
assertType("mixed", $mixed);
}
if ($mixed == 0) {
assertType("mixed", $mixed);
}
if ($mixed == '1') {
assertType("mixed~0|0.0|''|'0'|array{}|false|null", $mixed);
}
if ($mixed == '0') {
assertType("0|0.0|''|'0'|array{}|false|null", $mixed);
}
if ($mixed == 1) {
assertType("mixed~0|0.0|''|'0'|array{}|false|null", $mixed);
}
if ($mixed == 0) {
assertType("0|0.0|''|'0'|array{}|false|null", $mixed);
}

but I think its a unrelated problem for a followup?

}

if ($mixed == []) {
assertType("mixed", $mixed);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same here, could be

Suggested change
assertType("mixed", $mixed);
assertType("0|0.0|''|'0'|array{}|false|null", $mixed);

@ondrejmirtes

Copy link
Copy Markdown
Member

Hi, I'd like you to know that I welcome this effort, but I'm unlikely to merge it, because this area is more complex than this PR admits or implements. For example $mixed == 'backend' should not result in 'backend' but $mixed can also be true. And there are different strings that would be equal to false. This PR does not address that at all.

You've already tried a similar problem once (#2216) but didn't finish it.

@ondrejmirtes

Copy link
Copy Markdown
Member

Also the behaviour is PHP version-dependent: https://3v4l.org/rij8A

@staabm

staabm commented Jan 25, 2024

Copy link
Copy Markdown
Contributor Author

thanks for the input.

what do you think about turning the assertions into (for all truethy-constant comparisons) ?

if ($mixed == 'backend') {
			assertType("mixed~0|0.0|''|'0'|array{}|false|null", $mixed);
}

I feel we can't really get much better then do some blacklisting in this conditions for now?

@ondrejmirtes

Copy link
Copy Markdown
Member

I'm for doing as precise types as possible, but it'd be a substantial effort.

@staabm

staabm commented Jan 25, 2024

Copy link
Copy Markdown
Contributor Author

and would you be open to do it in small PRs, like e.g. doing "mixed vs. constant-type equal compare narrowing" in a single PR?
(or any other small step you are "ok" with would work for me - so I don't need to build up a multi month effort in a single PR)

@ondrejmirtes

Copy link
Copy Markdown
Member

We need to design a new method on Type for this. For example if you compare non-falsy-string against a different type, it tells us something about both left and right type if the comparison is true and something else about both types if the comparison is false.

The information from this method can be also used to figure out "is this always true or always false" comparison, similarly to what ImpossibleCheckTypeHelper figures out from the TypeSpecifier output.

Once we get this design right and have a proof of concept for a small number of Types, we can merge that and then implement the method in other types in smaller chunks, making the analysis more precise gradually.

@staabm

staabm commented Jan 25, 2024

Copy link
Copy Markdown
Contributor Author

my thinking is

example: non-falsey-string == int

-> take all candidate values out of the comparison table for the involved types

non-falsey-string -> "1", "-1" , "php"
int -> 1, 0, -1

-> based on the comparison table .. the results for these candidates are

php7:

"1" x 1 => true, "-1" x 1 => false , "php" x 1 => false

"1" x 0 => false, "-1" x 0 => false , "php" x 0 => true

"1" x -1 => false, "-1" x -1 => true , "php" x -1 => false

php8+:

"1" x 1 => true, "-1" x 1 => false , "php" x 1 => false

"1" x 0 => false, "-1" x 0 => false , "php" x 0 => false

"1" x -1 => false, "-1" x -1 => true , "php" x -1 => false

-> this means for the IF branch

php7: non-falsey-string stays non-falsey-string
php8: non-falsey-string gets narrowed to numeric-string

php7: stays int
php8: gets narrowed to int~0

-> this means for the ELSE branch

php7: non-falsey-string stays non-falsey-string
php8: non-falsey-string stays non-falsey-string

php7: staysint
php8: stays int


I think this means we need a type-method like Type->looseEqual(Type $other): Type - meaning the looseEqual method returns the resulting type of the comparison.

@ondrejmirtes

Copy link
Copy Markdown
Member

Yeah, something like that, the method also needs to have PhpVersion passed in.

@staabm

staabm commented Jan 27, 2024

Copy link
Copy Markdown
Contributor Author

Looking longer at the comparison table, I think it would be helpful to add a new non-numeric-string type. Do you agree?

@staabm

staabm commented Jan 27, 2024

Copy link
Copy Markdown
Contributor Author

Ahh just found phpstan/phpstan#10239 (comment)

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.

infer types based on non-strict equality with constant types

3 participants