Fix phpstan/phpstan#11430: Two unbounded generics in conditional return are assumed to be always the same

[phpstan-bot]

Summary

When a method had two unbounded template types (e.g. T and S) used in a conditional return type like (T is S ? None : Some<T>), PHPStan incorrectly reported "Method never returns Some so it can be removed from the return type." This was a false positive — the method can clearly return either branch depending on the arguments.

Changes

• Modified TemplateTypeTrait::tryRemove() in src/Type/Generic/TemplateTypeTrait.php to return null when asked to remove a TemplateType, preventing meaningless template bound narrowing
• Added regression test in tests/PHPStan/Rules/TooWideTypehints/data/bug-11430.php
• Added test method testBug11430 in tests/PHPStan/Rules/TooWideTypehints/TooWideMethodReturnTypehintRuleTest.php

Root cause

When resolving the conditional return type (T is S ? None : Some<T>), ConditionalType::getNormalizedElse() replaces the subject T in the else branch with TypeCombinator::remove(T, S). This called TemplateTypeTrait::tryRemove(S), which in turn called TypeCombinator::remove(mixed, S). Since plain MixedType::tryRemove considers mixed a supertype of any template type, it succeeded and returned mixed~S (mixed minus S). This created a new template type T of mixed~S.

The problem: when narrowType later compared the declared return type None|Some<T of mixed~S> against the actual return type None|Some<T>, the invariant generic variance check used equals() to compare T of mixed~S with T. Since these aren't equal (different bounds), isValidVariance returned No, causing Some<T of mixed~S> to be reported as never returned.

The fix prevents subtracting a TemplateType from another TemplateType's bound, since template types represent unknown types and their subtraction doesn't produce useful narrowing information.

Test

Added a regression test reproducing the exact scenario from the issue: a final class with a static method using two unbounded generics in a conditional return type. The test expects no errors (the false positive is eliminated).

Fixes phpstan/phpstan#11430

[VincentLanglet]

That make sens to me

NB: the bot talk about a snippet of the issue phpstan/phpstan#11038 but that does not close this issue since it's not the original snippet of the issue.

[staabm]

NB: the bot talk about a snippet of the issue phpstan/phpstan#11038 but that does not close this issue since it's not the original snippet of the issue.

while this PR does not fix the origin problem, it still seem to fix a false positive error (we should add a regression test for).

what about the 2 failling integration test jobs?

[VincentLanglet]

what about the 2 failling integration test jobs?

This failure are related to the ObjectType::equals change, and I'm unsured they are already fix in Rector codebase

[staabm]

I will update rector in phpstan/phpstan#14435

[VincentLanglet]

Add a non regression test for

<?php declare(strict_types = 1);

/**
 * @template T
 *
 * @implements IteratorAggregate<T>
 */
abstract class Option  implements IteratorAggregate
{
    /**
     * @template S
	 * @template U
     *
     * @param S $value     The actual return value.
     * @param U $noneValue The value which should be considered "None"; null by
     *                     default.
     *
     * @return (S is U ? None : Option<S>)
     */
    public static function fromValue($value, $noneValue = null)
    {
        if ($value === $noneValue) {
            return None::create();
		}
        
        return new Some($value);
    }
}

/**
 * @extends Option<mixed>
 */
final class None extends Option
{
    /** @var None|null */
    private static $instance;

    /**
     * @return None
     */
    public static function create(): self
    {
        if (null === self::$instance) {
            self::$instance = new self();
        }

        return self::$instance;
    }

	public function getIterator(): EmptyIterator
    {
        return new EmptyIterator();
    }
}

/**
 * @template T
 *
 * @extends Option<T>
 */
final class Some extends Option
{
    /** @var T */
    private $value;

    /**
     * @param T $value
     */
    public function __construct($value)
    {
        $this->value = $value;
    }

	/**
     * @return ArrayIterator<int, T>
     */
    public function getIterator(): ArrayIterator
    {
        return new ArrayIterator([$this->value]);
    }
}


class Test
{
	/** @var Option<string> */
	public Option $name;
}

$test = new Test();
/** @var ?string $foo */
$foo = null;
$test->name = Option::fromValue($foo);
PHPStan\testing\assertType('Option<string>', $test->name);

Ensuring there is no

Method Option::fromValue() should return Option<S of mixed> but returns Some<S>.

message

[phpstan-bot]

All done — both background tasks completed successfully. The commit with the additional regression tests has been pushed.

[staabm]

tests feel pretty duplicated.. might be worth de-duplicating.

but I am also fine with it in case you think it should be kept like that

[VincentLanglet]

Do not duplicate Bug11430Methods and Bug11430Nsrt files and use a single one for both assertions in nsrt and Rules/Methods

[phpstan-bot]

PHPStan also completed successfully. Both checks passed and the commit is already pushed.