Do not merge BenevolentUnionType with UnionType

[herndlm]

This was inspired by phpstan/phpstan#8458 (comment)

Closes phpstan/phpstan#7049
Closes phpstan/phpstan#7279
Closes phpstan/phpstan#7423
Closes phpstan/phpstan#8268

After this https://phpstan.org/r/498cd1d0-4568-4cbf-8b0b-11ed722552e8 becomes green.

Regarding the performance impact (#2058 (comment))

before:

Benchmark 1: make phpstan
  Time (mean ± σ):     56.978 s ± 10.578 s    [User: 338.941 s, System: 26.704 s]
  Range (min … max):   49.723 s … 82.109 s    10 runs

after:

Benchmark 1: make phpstan
  Time (mean ± σ):     51.790 s ±  5.459 s    [User: 330.922 s, System: 25.508 s]
  Range (min … max):   48.548 s … 66.973 s    10 runs

not sure if that is enough, the system is a bit trashy I guess. but looks like things don't really change perf.-wise. I'm pretty sure things also don't get faster, the variance is just too big on my system, I think we can go with 49s before and after.

Regarding the 3 failures

• PocketMine: Cannot access offset (float|int<1, max>) on mixed. https://github.com/pmmp/PocketMine-MP/blob/e0b07ff3087b652407439a29c941f3b66ca92c86/src/pocketmine/CrashDump.php#L314 / https://github.com/pmmp/PocketMine-MP/blob/e0b07ff3087b652407439a29c941f3b66ca92c86/src/pocketmine/CrashDump.php#L111
• Prestashop: modification of a ArrayObject<*NEVER*, *NEVER*> https://github.com/PrestaShop/PrestaShop/blob/ca9d81aae0bb17f3e767bb5d835348ed144a3ab5/src/Adapter/Presenter/AbstractLazyArray.php#L98
• PHPUnit: modification of a ArrayObject<*NEVER*, *NEVER*>, e.g. https://github.com/sebastianbergmann/phpunit/blob/93d4bf4c37aec6384bb9e5d390d9049a463a7256/tests/unit/Framework/AssertTest.php#L153

all of those are expected IMO, but I can create PRs for Prestashop and PHPUnit to add PHPDoc types to the collection if wanted

[herndlm]

OK close, better than I thought. I guess I deleted too much and am loosing the benevolent template object..

[staabm]

You should measure whether these changes have a perf impact

[VincentLanglet]

Since A|(B|C) would be now a possible type, does it require a test to check that a benevolent union
(A|(B|C)|D) is simplified to (A|B|C|D) ?

[herndlm]

Good question, I'm not sure if it should be tbh. If we explicitly want a union, shouldn't it be non-benevolent? 🤔 I guess what I'm saying is, are we expecting this to be simplified if we call TypeCombinator::union()?
UPDATE: ah sorry, got your point now. Those should be simplified I suppose, yeah

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

I'm struggling a bit with this one. What is your thinking behind this PR? I see that it fixes a bunch of issues, but to me it looks like it's just a matter of luck, not an intention (please take no offense :)).

What's a typical situation that this PR improves/fixes?

Also, I think we need some more tests around this. Let's say for example we have (int|string) and we want to add some types into it by calling TypeCombinator::union. What's the expected result?

• When we add a ConstantStringType like '1'.
• When we add a StringType.
• When we add ConstantBooleanType like true.

[herndlm]

The main intention behind this is that I believe, and I think the linked issues confirm it, that union with a benevolent union should not suddenly get rid of the benevolence. E.g. https://phpstan.org/r/498cd1d0-4568-4cbf-8b0b-11ed722552e8

Calling TypeCombinator::union() will always add a real union and not get rid of or extend the benevolence. More tests make sense for sure :) some already existed though I believe, and I just adapted their result. Your mentioned cases are good ones that worry me that the edges are still rough here hehe

But it's basically what you suggested at phpstan/phpstan#8268 (comment) (which I just found later via issue bot).

[herndlm]

Added the test cases for union with (int|string) you requested, we get the following

When we add a ConstantStringType like '1'.

(int|string)

When we add a StringType.

(int|string)

When we add ConstantBooleanType like true.

(int|string)|true

that looks OK to me 😊

[ondrejmirtes]

I'd argue that TypeCombinator::union of (int|string) and string should result in non-benevolent union type.

Another idea for a test - if we have (1|2) and string, it should result in string (which it probably will).

Also we can test what happens with (1|2|3) and int<2, 3>.

Also (InvalidArgumentException|LengthException|stdClass) and LogicException.

[herndlm]

Ok, added your mentioned cases again, the result is a bit different, I got

• (1|2)|string UPDATE: but ('1'|'2')|string does result in string, not sure if I misunderstood that or not, anyway, also added it as a test
• (1|2|3)
• (InvalidArgumentException|LengthException|stdClass)|LogicException

so most likely there's still work needed / it can be improved? I pushed them in this state, but need to think about this another time. Such benevolent cases are still hard for me to think through properly

[VincentLanglet]

What's a typical situation that this PR improves/fixes?

https://phpstan.org/r/9be20c04-429f-4dc0-b245-9d23628ae2db

Both $key and $bool are accepted by the method, but the union of both type is not accepted.
The current behavior doesn't feel natural.

I'd argue that TypeCombinator::union of (int|string) and string should result in non-benevolent union type.

Technically, it should be (int)|string which is not really a possible type currently.

Because if it's transformed into int|string

		$key = current(array_keys($array));
		$this->acceptString($key); // valid
		$this->acceptString($string); // valid
		
		$keyOrString = rand(0,1) ? $key : $string;
		$this->acceptString($keyOrString); // won't be but should still be valid

[drupol]

@drupol are you referring to the bot message you got in your issue (which is unrelated to this PR) or https://github.com/phpstan/phpstan-src/actions/runs/3910108845? :)

Sorry, here's the proper link: phpstan/phpstan#7423 I also fixed it in my previous message.

[herndlm]

Sorry, here's the proper link: phpstan/phpstan#7423 I also fixed it in my previous message.

yeah and why do you mean this PR does not fix it? the bot message there is unrelated. but in https://github.com/phpstan/phpstan-src/actions/runs/3910108845 I can see no errors for PHP 8+ because the type should be correct now? and that is also checked via regression test here.

[herndlm]

Did we come to a conclusion about this one? :) I rebased it on 1.10.x in the meantime. Thought the behavior change might be better there. In case it gets merged. So far it still makes sense to me

[herndlm]

Going to rebase this one tomorrow morning. Is there anything else I can do here? This still makes sense to me :)

[ondrejmirtes]

@herndlm I have some issues with it. For example I've seen a type (X)|Y mentioned somewhere and I don't think that one-item union type makes sense... So I'm not sure how you're handling it. Last time I looked at this PR it felt a bit undertested.

For example if you compare int|stdClass|string and (int|string)|stdClass - when is the first one going to be accepted and the second one won't?

[herndlm]

Naming this PR "Do not merge" was a stupid idea, that way it will never get merged... 😂

jokes aside, I try to get this updated and tests added soon. would be great for 2.x I suppose.

[ondrejmirtes]

I'm sorry, I don't see this being merged :/

[herndlm]

I'm sorry, I don't see this being merged :/

that's ok, no worries. is there anything I can do to change that or is it mostly based on experience/gut feeling (don't meant that in a bad way)? To me this still makes sense and kind of is the only way IMO to deal with that the linked issues show, also #2058 (comment) and the example there.

but maybe there are edge cases I'm just not seeing yet and already have in mind or are worried about :)

[ondrejmirtes]

I don't want to screw around https://phpstan.org/developing-extensions/type-system#type-normalization, and keeping benevolent union type separate from non-benevolent union type does that.

[herndlm]

I don't want to screw around phpstan.org/developing-extensions/type-system#type-normalization, and keeping benevolent union type separate from non-benevolent union type does that.

sorry, don't want to be too annoying here, but right now they are merged and not merging them keeps them more separate, no? just want to fully understand what might cause issues here.

[ondrejmirtes]

I don't want to keep them separate.

[herndlm]

ok, thx! I'll close this for now, not sure atm, but maybe there's a better way of dealing with the linked issues 😊

[herndlm]

what about, yeah I know, I can't leave it alone 😂, the other way around? merging more aggressively: (int|string)|float => (float|int|string). my current reasoning: if benevolence is involved type checks are less strict and maybe this should be kept like that if additional types are added as in the (outdated) original example of https://phpstan.org/r/498cd1d0-4568-4cbf-8b0b-11ed722552e8

if stricter type checks are needed checkBenevolentUnionTypes could be enabled. obviously, I don't know yet what exact impacts this will have, less errors I suppose, so nobody is annoyed. there is of course also phpstan/phpstan#8458 (comment) which sounds like it would miss more potential runtime errors then I suppose :(

UPDATE: the more I think about it, the less I like it. Being less strict sounds bad :/ can we deprecate BenevolentUnionType? 😂