Do not merge `BenevolentUnionType` with `UnionType`

[herndlm]

This was inspired by https://github.com/phpstan/phpstan/issues/8458#issuecomment-1336402081

Closes https://github.com/phpstan/phpstan/issues/7049 Closes https://github.com/phpstan/phpstan/issues/7279 Closes https://github.com/phpstan/phpstan/issues/7423 Closes https://github.com/phpstan/phpstan/issues/8268

Regarding the performance impact (https://github.com/phpstan/phpstan-src/pull/2058#issuecomment-1340500218)

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 https://github.com/phpstan/phpstan/issues/8268#issuecomment-1301834601 (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]

Hello @herndlm,

I don't think this is closing #742, the output is different for sure, but it's not fixed actually.

[herndlm]

@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? :)

[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: https://github.com/phpstan/phpstan/issues/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.

[drupol]

Oh if it's unrelated, please ignore my message then.

Sorry for the noise.

[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?