Fix issue with template of union

[VincentLanglet]

Closes phpstan/phpstan#11663

Cf phpstan/phpstan#11663 (comment) I dunno if there is a already-known better way to handle such situation.

[staabm]

I think whether this is a problem which should be solved with a Type method.

maybe we can use a Type method in which you pass in a callable and it returns a preserved union type (maybe such method even exists...?)

[VincentLanglet]

The whole method filterTypeWithMethod could be introduce as something like Type::filter if wanted but this would "only" be a refacto. (Which may be in another PR)

There is UnionType::traverse

public function traverse(callable $cb): Type	{	$types = [];	$changed = false;	foreach ($this->types as $type) {	$newType = $cb($type);	if ($type !== $newType) {	$changed = true;	}	$types[] = $newType;	}	if ($changed) {	return TypeCombinator::union(...$types);	}	return $this;	} 

but it doesn't do the exact same thing ; we can still notice a changed check is done the same way.

[herndlm]

don't want to disturb you guys too much, but another idea to throw in - does TypeTraverser::map() help in any way here maybe? e.g. the types you don't want you could replace with a NeverType potentially? that should leave the outer TemplateUnionType untouched at least. that could be a replacement for the foreach, AFAIK TypeCombinator::union() should keep the TemplateUnionType then and remove the never I suppose.

but not sure if that makes it even more complicated potentially 😅

[ondrejmirtes]

I get it, but the variable name should be different. Something like $unionTypeHasChanged.

Also it's a lot of repeated code. I'd welcome some refactoring first, and then the change should be done in a single place.

[VincentLanglet]

Hi, what do you think about this PR @ondrejmirtes ?

Thanks

[ondrejmirtes]

I have no idea how $oneTypeHasBeenRemoved solves a problem so I don't want to merge it right now.

[VincentLanglet]

I have no idea how $oneTypeHasBeenRemoved solves a problem so I don't want to merge it right now.

The idea was kinda similar to UnionType::traverse

public function traverse(callable $cb): Type	{	$types = [];	$changed = false;	foreach ($this->types as $type) {	$newType = $cb($type);	if ($type !== $newType) {	$changed = true;	}	$types[] = $newType;	}	if ($changed) {	return TypeCombinator::union(...$types);	}	return $this;	} 

the goal is to keep the same object when nothing change (here when no type is removed).

Because the existing code

if ($typeWithMethod instanceof UnionType) {	$newTypes = []; foreach ($typeWithMethod->getTypes() as $innerType) {	if (!$innerType->hasMethod($methodName)->yes()) {	continue;	}	$newTypes[] = $innerType;	}	if (count($newTypes) === 0) {	return null;	}	$typeWithMethod = TypeCombinator::union(...$newTypes); 

always change the instance of typeWithMethod when it's an UnionType, which means:

• BenevolentUnion become an UnionType
• TemplateUnion become an UnionType

But when oneTypeHasBeenRemoved = false we could keep the initial instance of UnionType since we keep all the subTypes. This way, for the existing issue, we keep the TemplateUnion we all the needed informations.

[ondrejmirtes]

I get it, but the variable name should be different. Something like $unionTypeHasChanged.

Also it's a lot of repeated code. I'd welcome some refactoring first, and then the change should be done in a single place.

[VincentLanglet]

PR is simplified now

[ondrejmirtes]

We could improve this further, right? Even if something changes in UnionType subtype, in this method we could make sure the correct class is created.

The subtypes from the top of my head are:

• BenevolentUnionType
• TemplateUnionType
• TemplateBenevolentUnionType

[VincentLanglet]

We could improve this further, right? Even if something changes in UnionType subtype, in this method we could make sure the correct class is created.

The subtypes from the top of my head are:

• BenevolentUnionType
• TemplateUnionType
• TemplateBenevolentUnionType

I understand the use case for BenevolentUnion (__benevolent(A|B|C) become __benevolent(A|B)), but it's complicated for me to see the use case for TemplateUnionType and TemplateBenevolentUnionType.
If we have T of A|B|C it feel wrong to change this to T of A|B isn't it ?

Do you have an example to add to the tests ?

[ondrejmirtes]

Examples:

• TemplateUnionType: https://phpstan.org/r/f7585d6f-e7e8-4a4f-ad70-52c8d5a28b2c (it should not report "Function foo() should return T of A|B but returns A.")
• TemplateBenevolentUnionType: https://phpstan.org/r/c9a5c400-94e9-4c67-9636-2fc35b219156

[ondrejmirtes]

I'd rather have this logic in the subtypes in overridden methods.

[VincentLanglet]

Done

[ondrejmirtes]

Thank you.