Skip to content

Make ParametersAcceptorSelector::selectSingle() return union-ed conditional types #1159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Apr 2, 2022
Merged
10 changes: 8 additions & 2 deletions src/Reflection/ParametersAcceptorSelector.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use PHPStan\Type\NullType;
use PHPStan\Type\Type;
use PHPStan\Type\TypeCombinator;
use PHPStan\Type\TypeUtils;
use PHPStan\Type\UnionType;
use function array_key_last;
use function array_slice;
Expand All @@ -32,7 +33,7 @@ class ParametersAcceptorSelector
/**
* @template T of ParametersAcceptor
* @param T[] $parametersAcceptors
* @return T
* @return T|SingleParametersAcceptor
*/
public static function selectSingle(
array $parametersAcceptors,
Expand All @@ -48,7 +49,12 @@ public static function selectSingle(
throw new ShouldNotHappenException('Multiple variants - use selectFromArgs() instead.');
}

return $parametersAcceptors[0];
$parametersAcceptor = $parametersAcceptors[0];
if (TypeUtils::containsConditional($parametersAcceptor->getReturnType())) {
return new SingleParametersAcceptor($parametersAcceptor);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't immediately get tests to pass without guarding here. I could whip up a helper class that can convert a given ParametersAcceptor into the same type but with the return type adjusted? That way the contract still holds.


return $parametersAcceptor;
}

/**
Expand Down
58 changes: 58 additions & 0 deletions src/Reflection/SingleParametersAcceptor.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<?php declare(strict_types = 1);

namespace PHPStan\Reflection;

use PHPStan\Type\ConditionalType;
use PHPStan\Type\ConditionalTypeForParameter;
use PHPStan\Type\Generic\TemplateTypeMap;
use PHPStan\Type\Type;
use PHPStan\Type\TypeTraverser;

class SingleParametersAcceptor implements ParametersAcceptor
{

private ?Type $returnType = null;

public function __construct(private ParametersAcceptor $acceptor)
{
}

public function getTemplateTypeMap(): TemplateTypeMap
{
return $this->acceptor->getTemplateTypeMap();
}

public function getResolvedTemplateTypeMap(): TemplateTypeMap
{
return $this->acceptor->getResolvedTemplateTypeMap();
}

/**
* @return array<int, ParameterReflection>
*/
public function getParameters(): array
{
return $this->acceptor->getParameters();
}

public function isVariadic(): bool
{
return $this->acceptor->isVariadic();
}

public function getReturnType(): Type
{
if ($this->returnType === null) {
return $this->returnType = TypeTraverser::map($this->acceptor->getReturnType(), static function (Type $type, callable $traverse) {
Copy link
Contributor

@staabm staabm Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style-nit - the line is already long enough, and will return on the outer block immediately with the same value

Suggested change
return $this->returnType = TypeTraverser::map($this->acceptor->getReturnType(), static function (Type $type, callable $traverse) {
$this->returnType = TypeTraverser::map($this->acceptor->getReturnType(), static function (Type $type, callable $traverse) {

while ($type instanceof ConditionalType || $type instanceof ConditionalTypeForParameter) {
$type = $type->getResult();
}

return $traverse($type);
});
}

return $this->returnType;
}

}
2 changes: 1 addition & 1 deletion src/Type/ConditionalType.php
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ private function resolve(): Type
}

if ($this->isResolved()) {
return TypeCombinator::union($this->if, $this->else);
return $this->getResult();
}

return $this;
Expand Down
2 changes: 1 addition & 1 deletion src/Type/Traits/ConditionalTypeTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ public function isGreaterThanOrEqual(Type $otherType): TrinaryLogic
return $otherType->isSmallerThanOrEqual($result);
}

private function getResult(): Type
public function getResult(): Type
{
if ($this->result === null) {
return $this->result = TypeCombinator::union($this->if, $this->else);
Expand Down
18 changes: 18 additions & 0 deletions src/Type/TypeUtils.php
Original file line number Diff line number Diff line change
Expand Up @@ -328,4 +328,22 @@ public static function containsCallable(Type $type): bool
return false;
}

public static function containsConditional(Type $type): bool
{
if ($type instanceof ConditionalType || $type instanceof ConditionalTypeForParameter) {
return true;
}

$contains = false;
TypeTraverser::map($type, static function (Type $type, callable $traverse) use (&$contains): Type {
if ($type instanceof ConditionalType || $type instanceof ConditionalTypeForParameter) {
$contains = true;
}

return !$contains ? $traverse($type) : $type;
});

return $contains;
}

}
2 changes: 1 addition & 1 deletion tests/PHPStan/Analyser/AnalyserIntegrationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,7 @@ public function testBug6896(): void
}

$errors = $this->runAnalyse(__DIR__ . '/data/bug-6896.php');
$this->assertCount(4, $errors);
$this->assertCount(2, $errors);
}

public function testBug6940(): void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ public function dataAsserts(): iterable
{
yield from $this->gatherAssertTypes(__DIR__ . '/data/dynamic-method-return-types.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/dynamic-method-return-compound-types.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/dynamic-method-return-getsingle-conditional.php');
}

/**
Expand Down
20 changes: 20 additions & 0 deletions tests/PHPStan/Analyser/data/TestDynamicReturnTypeExtensions.php
Original file line number Diff line number Diff line change
Expand Up @@ -189,3 +189,23 @@ public function getTypeFromMethodCall(MethodReflection $methodReflection, Method
}

}


class ConditionalGetSingle implements DynamicMethodReturnTypeExtension {

public function getClass(): string
{
return \DynamicMethodReturnGetSingleConditional\Foo::class;
}

public function isMethodSupported(MethodReflection $methodReflection): bool
{
return $methodReflection->getName() === 'get';
}

public function getTypeFromMethodCall(MethodReflection $methodReflection, MethodCall $methodCall, Scope $scope): Type
{
return ParametersAcceptorSelector::selectSingle($methodReflection->getVariants())->getReturnType();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

namespace DynamicMethodReturnGetSingleConditional;

use function PHPStan\Testing\assertType;

abstract class Foo
{
/**
* @return ($input is 1 ? true : false)
*/
abstract public function get(int $input): mixed;

public function doFoo(): void
{
assertType('bool', $this->get(0));
assertType('bool', $this->get(1));
assertType('bool', $this->get(2));
Comment on lines +16 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we expect a more precise type here?

Suggested change
assertType('bool', $this->get(0));
assertType('bool', $this->get(1));
assertType('bool', $this->get(2));
assertType('false', $this->get(0));
assertType('true', $this->get(1));
assertType('false', $this->get(2));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - there is return type extension that just returns ParametersAcceptorSelector::selectSingle() to verify that it provides the resulting union type instead of the conditional type.

}
}
4 changes: 4 additions & 0 deletions tests/PHPStan/Analyser/dynamic-return-type.neon
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,7 @@ services:
class: PHPStan\Tests\FooGetSelf
tags:
- phpstan.broker.dynamicMethodReturnTypeExtension
-
class: PHPStan\Tests\ConditionalGetSingle
tags:
- phpstan.broker.dynamicMethodReturnTypeExtension