Normalize specified types before intersection #1016

herndlm · 2022-02-13T20:33:30Z

This PR is improving type specification with combined BooleanAnd and BooleanIf expressions in 2 ways. Therefore the title does not match anymore :) I think something like "Improve type specification for combined && and || expressions" would fit better.

It normalizes the SpecifiedTypes by removing the sureNotTypes from the relevant sureTypes which results in e.g. AccessoryNonEmptyStringType or NonEmptyArrayType. Without this, sureNotTypes might not be considered when intersecting instances of SpecifiedType, which leads to type information loss.

Details about the normalization

In order to narrow down the variable $a from mixed to non-empty-string|null the following code could (it's maybe a bit odd but this is not relevant) be used in an if (truthy context):

is_string($a) && '' !== $a || null === $a

This can be described with the expression

new Expr\BinaryOp\BooleanOr(
	new Expr\BinaryOp\BooleanAnd(
		new FuncCall(new Name('is_string'), [new Arg(new Variable('a'))])
		new NotIdentical(new String_(''), new Variable('a')),
	),
	new Identical(new Expr\ConstFetch(new Name('null')), new Variable('a')),
)

The interesting parts are the inner BooleanAnd and the outer BooleanOr. If the TypeSpecifier looks at this it will basically recursively determine the SpecifiedTypes. Without going into too much details, that the relevant people know better than me anyway, the following will be the simplified relevant sureTypes and sureNotTypes for $a of the relevant SpecifiedTypes instances:

BooleanAnd

left expression: StringType sureType
right expression: ConstantStringType with value '' sureNotType

Result

BooleanAnd uses SpecifiedTypes->unionWith for a truthy context like here which results in a new SpecifiedTypes having both of those

BooleanOr

left expression: the above union => StringType sureType, ConstantStringType with value '' sureNotType
right expression: NullType sureType

Result

=> BooleanOr uses SpecifiedTypes->intersectWith

Before

intersectWith only looks at the sureTypes and sureNotTypes of both SpecifiedTypes instances if their expression string occurs also in both. Meaning, it will union (for a truthy context like here) the StringType and NullType because both SpecifiedTypes instances have sureTypes for $a. But, it will not use the sureNotType if there is not a sureNotType in the other SpecifiedTypes for $a. Therefore it results in a SpecifiedTypes instance with a StringType|NullType sureTypes union only and basically looses the sureNotType information that is relevant for the left expression.

After

both SpecifiedTypes instances are normalized which "moves" the sureNotType into the sureType resulting in a StringType&AccessoryNonEmptyStringType intersection for the left expression basically. The other SpecifiedTypes instance is not affected here by normalization. The intersectWith logic then unionizes (for a truthy context like here) both instances resulting in a (StringType&AccessoryNonEmptyStringType)|NullType union sureType and therfore does not loose the sureNotType information which is coupled to the left expression but does not make any sense for the right one.

Without the BooleanOr the sureNotType is not lost and the MutatingScope can use it to determine that the outcome is non-empty-string. With BooleanOr we have to normalize this already here IMO before intersecting in order to not loose the information.

It makes use of the MutatingScope in BooleanAnd and BooleanOr expressions to filter by the specified types of the left espression and uses the resulting mutated scope in the right expression. This ensures that information gathered on the left side can be used on the right side. See Normalize specified types before intersection #1016 (comment)

src/Analyser/TypeSpecifier.php

tests/PHPStan/Analyser/data/bug-6329.php

tests/PHPStan/Analyser/TypeSpecifierTest.php

herndlm · 2022-02-14T06:55:36Z

The failing tests are either unrelated or, in case of webmozart-assert, actual fixes

Marking this as ready, but I have the feeling the 2 typespecifier workarounds should be reverted for now, just let me know

ondrejmirtes · 2022-02-14T07:17:25Z

Hi, thank you for the research. Can you please describe in more detail how does SpecifiedTypes look before and after the normalization and what does it achieve further down the line? Thanks.

herndlm · 2022-02-14T12:23:54Z

@ondrejmirtes I wrote a short novel hidden behind the Details section in the description. I hope this explains everything.

ondrejmirtes · 2022-02-14T15:47:56Z

Perfect explanation, I now understand the problem :) I'm gonna let it sit for a while to think about it, maybe I'll come up even with a better solution. What I'm worried about a bit is a falsy context where the meaning of sureTypes/sureNotTypes is switched and it should be doing the opposite I think. Anyway, it's now my turn to do something about it, thank you.

…ve hacky workarounds

herndlm · 2022-02-15T14:58:22Z

I added another commit and with that one I managed to

get rid of my hacky workarounds for strlen and count that I added in the first commit
fix what I described and worked-around in Remove array_filter mixed handling which breaks type specification #1012

It makes use of the MutatingScope when handling BooleanAnd and BooleanOr by filtering with the left expression. And that mutated scope is then only used for the right expression. I think this was needed because the left expression could narrow down a mixed variable via e.g. is_string to, well, string. And that information is currently not used in the right expression which can make problems with some type extensions that ask the scope via getType and use that information internally.

I think this change makes PHPStan understand some complex true and false context conditionals that it did not understand before 🎉 At least if I'm not completely mistaken. But I'll think about it a bit more. Maybe it also makes sense to add more complex test cases somewhere

ondrejmirtes · 2022-02-15T19:16:53Z

Are any of the new test cases and bugfixes about my concern?

What I'm worried about a bit is a falsy context where the meaning of sureTypes/sureNotTypes is switched and it should be doing the opposite I think.

herndlm · 2022-02-15T19:21:48Z

Are any of the new test cases and bugfixes about my concern?

There were already test cases that check the type in the else statement and I added some more. And AFAIK and could see via debugger, it uses a falsy context then to create the specified types and filter by them in the scope. So, I think this should be covered :)

ondrejmirtes · 2022-02-15T19:24:15Z

Alright, I'm gonna merge it :) Please send a fix to webmozart-assert extension afterwards, thanks.

ondrejmirtes · 2022-02-15T19:25:23Z

Thank you!

herndlm · 2022-02-15T19:28:14Z

Finally I can sleep tonight xD But seriously, this kept me thinking, busy and step debugging quite a few hours

Alright, I'm gonna merge it :) Please send a fix to webmozart-assert extension afterwards, thanks.

In order to do that I need a new phpstan release to specify as minimum supported version, I think. Shall I comment-out the two problematic lines for now and add them back in correctly after a new phpstan version was tagged?

ondrejmirtes · 2022-02-15T19:28:15Z

Also, feel free to continue thinking about it and have it in the back of your head, it's often that people come up with improvements afterwards and I love many little PRs :) Generics were implemented over dozens of small PRs :)

staabm · 2022-02-15T19:31:14Z

great job @herndlm !

ondrejmirtes · 2022-02-15T19:31:40Z

I need a new phpstan release to specify as minimum supported version

You don't - put ^1.4.7 or whatever is the next one to composer.json and the current dev-master will get installed. It's not going to have to change after the release and the stable version will be used automatically 😊

herndlm commented Feb 13, 2022

View reviewed changes

src/Analyser/TypeSpecifier.php Outdated Show resolved Hide resolved

src/Analyser/TypeSpecifier.php Outdated Show resolved Hide resolved

herndlm force-pushed the normalize-specified-types-before-intersection branch from 1dce3de to 67282d2 Compare February 13, 2022 20:47

Normalize specified types before intersection

949e5fa

herndlm force-pushed the normalize-specified-types-before-intersection branch from 67282d2 to 949e5fa Compare February 13, 2022 20:47

staabm reviewed Feb 13, 2022

View reviewed changes

tests/PHPStan/Analyser/data/bug-6329.php Show resolved Hide resolved

staabm reviewed Feb 13, 2022

View reviewed changes

tests/PHPStan/Analyser/TypeSpecifierTest.php Show resolved Hide resolved

herndlm marked this pull request as ready for review February 14, 2022 06:55

herndlm mentioned this pull request Feb 14, 2022

Improve falsey type specification with constant string comparison #1015

Closed

Let BooleanAnd and BooleanOr make use of the left scope / types, remo…

6888c1e

…ve hacky workarounds

Add a couple more interesting test cases

eae8cc0

ondrejmirtes merged commit f255672 into phpstan:master Feb 15, 2022

herndlm deleted the normalize-specified-types-before-intersection branch February 15, 2022 19:28

herndlm mentioned this pull request Feb 15, 2022

Require latest phpstan to fix a couple of things phpstan/phpstan-webmozart-assert#102

Merged

Normalize specified types before intersection #1016

Normalize specified types before intersection #1016

Uh oh!

Conversation

herndlm commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

BooleanAnd

Result

BooleanOr

Result

Before

After

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

herndlm commented Feb 14, 2022

Uh oh!

ondrejmirtes commented Feb 14, 2022

Uh oh!

herndlm commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Feb 14, 2022

Uh oh!

herndlm commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Feb 15, 2022

Uh oh!

herndlm commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Feb 15, 2022

Uh oh!

ondrejmirtes commented Feb 15, 2022

Uh oh!

herndlm commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Feb 15, 2022

Uh oh!

staabm commented Feb 15, 2022

Uh oh!

ondrejmirtes commented Feb 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

herndlm commented Feb 13, 2022 •

edited

Loading

herndlm commented Feb 14, 2022 •

edited

Loading

herndlm commented Feb 15, 2022 •

edited

Loading

herndlm commented Feb 15, 2022 •

edited

Loading

herndlm commented Feb 15, 2022 •

edited

Loading