Improve falsey type specification with constant string comparison #1015
Conversation
|
The related failing webmozart tests seem to be the fix for https://github.com/phpstan/phpstan-webmozart-assert/blob/1.0.9/tests/Type/WebMozartAssert/data/type.php#L25 :) |
| @@ -234,6 +235,13 @@ public function specifyTypesInCondition( | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| if ($context->falsey() && $constantType instanceof ConstantStringType) { | |||
There was a problem hiding this comment.
I did not understand the change fully, but would this also make sense for other constant types here?
There was a problem hiding this comment.
Tbh I'm also not a 100% sure if this is the right/best fix. But I thought the same, yeah. Will take a look later. Also thought of getting an opinion from Ondrej first maybe
|
|
||
| function foo($a): void | ||
| { | ||
| if (is_string($a) && '' !== $a) { |
There was a problem hiding this comment.
do you need the same test, with switched arguments?
| assertType('non-empty-string', $a); | ||
| } | ||
|
|
||
| if (is_string($a) && '' !== $a || null === $a) { |
There was a problem hiding this comment.
Same for is_string($a) && strlen($a) > 0 || null === $a ?
|
ok, forget this PR for now.. While trying to make it work with Anyways, I think I need to adapt something in the BooleanOr handling of Well, I'll keep this PR open a bit longer but promise to not add commits and comments as long as I don't have a full fix ;) I already have another idea |
|
My other approach is simpler, cleaner, makes sense and works 🎉 I'll clean it up and create a new PR in the evening. UPDATE: superseded by #1016 |
herndlm
deleted the
improve-falsey-context-type-specification-with-constant-string-comparison
branch
Closes phpstan/phpstan#6329
This makes use of type subtraction to add information about constant strings that cannot be part of the type.
E.g.
is_string($a) && '' !== $a || null === $awith$abeingmixedwould lead to aMixedTypesureType with the empty constant string''substracted for the inner BooleanAnd expression. And that leads to a intersection ofStringTypeandAccessoryNonEmptyStringType.Previously, it would determine that the empty constant string
''is a sureNotType but loose that information on merging left and right types for the outer BooleanOr.But I'm not sure if this is the best fix :) Initially I tried adapting the left/right type merging, particularly the intersection for BooleanOr, but that made things even worse by breaking other things.