-
Notifications
You must be signed in to change notification settings - Fork 564
Fix phpstan/phpstan#11073: Nullsafe operator chaining #5407
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
Changes from all commits
2bedf83
20a8328
d7ab184
97f0294
ed3922a
3240d14
1ede898
35c0fd3
53a43c1
4579639
c94bf83
dc65ae7
ea795df
1e9c190
904c51c
d2b04a7
68cc394
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| use PHPStan\DependencyInjection\AutowiredService; | ||
| use PHPStan\DependencyInjection\Type\DynamicReturnTypeExtensionRegistryProvider; | ||
| use PHPStan\Reflection\ParametersAcceptorSelector; | ||
| use PHPStan\Type\ObjectType; | ||
| use PHPStan\Type\Type; | ||
| use PHPStan\Type\TypeCombinator; | ||
| use function count; | ||
|
|
@@ -52,7 +53,9 @@ | |
| } | ||
|
|
||
| $resolvedTypes = []; | ||
| foreach ($typeWithMethod->getObjectClassNames() as $className) { | ||
| $allClassNames = $typeWithMethod->getObjectClassNames(); | ||
| $handledClassNames = []; | ||
| foreach ($allClassNames as $className) { | ||
| if ($normalizedMethodCall instanceof MethodCall) { | ||
| foreach ($this->dynamicReturnTypeExtensionRegistryProvider->getRegistry()->getDynamicMethodReturnTypeExtensionsForClass($className) as $dynamicMethodReturnTypeExtension) { | ||
| if (!$dynamicMethodReturnTypeExtension->isMethodSupported($methodReflection)) { | ||
|
|
@@ -65,6 +68,7 @@ | |
| } | ||
|
|
||
| $resolvedTypes[] = $resolvedType; | ||
| $handledClassNames[] = $className; | ||
| } | ||
| } else { | ||
| foreach ($this->dynamicReturnTypeExtensionRegistryProvider->getRegistry()->getDynamicStaticMethodReturnTypeExtensionsForClass($className) as $dynamicStaticMethodReturnTypeExtension) { | ||
|
|
@@ -82,11 +86,29 @@ | |
| } | ||
|
|
||
| $resolvedTypes[] = $resolvedType; | ||
| $handledClassNames[] = $className; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (count($resolvedTypes) > 0) { | ||
| if (count($allClassNames) !== count($handledClassNames)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this change need testing outside of the DateTime* use-case? |
||
| $remainingType = $typeWithMethod; | ||
| foreach ($handledClassNames as $handledClassName) { | ||
| $remainingType = TypeCombinator::remove($remainingType, new ObjectType($handledClassName)); | ||
| } | ||
| if ($remainingType->hasMethod($methodName)->yes()) { | ||
|
Check warning on line 100 in src/Analyser/ExprHandler/Helper/MethodCallReturnTypeHelper.php
|
||
| $remainingMethod = $remainingType->getMethod($methodName, $scope); | ||
| $remainingParametersAcceptor = ParametersAcceptorSelector::selectFromArgs( | ||
| $scope, | ||
| $methodCall->getArgs(), | ||
| $remainingMethod->getVariants(), | ||
| $remainingMethod->getNamedArgumentsVariants(), | ||
| ); | ||
| $resolvedTypes[] = $remainingParametersAcceptor->getReturnType(); | ||
| } | ||
| } | ||
|
|
||
| return VoidToNullTypeTransformer::transform(TypeCombinator::union(...$resolvedTypes), $methodCall); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,8 +11,11 @@ | |
| use PHPStan\Type\Constant\ConstantBooleanType; | ||
| use PHPStan\Type\DynamicMethodReturnTypeExtension; | ||
| use PHPStan\Type\NeverType; | ||
| use PHPStan\Type\ObjectType; | ||
| use PHPStan\Type\Type; | ||
| use PHPStan\Type\TypeCombinator; | ||
| use PHPStan\Type\TypeTraverser; | ||
| use PHPStan\Type\UnionType; | ||
| use Throwable; | ||
| use function count; | ||
|
|
||
|
|
@@ -39,11 +42,12 @@ | |
|
|
||
| public function getTypeFromMethodCall(MethodReflection $methodReflection, MethodCall $methodCall, Scope $scope): ?Type | ||
| { | ||
| if (count($methodCall->getArgs()) < 1) { | ||
| $args = $methodCall->getArgs(); | ||
| if (count($args) < 1) { | ||
| return null; | ||
| } | ||
|
|
||
| $valueType = $scope->getType($methodCall->getArgs()[0]->value); | ||
| $valueType = $scope->getType($args[0]->value); | ||
| $constantStrings = $valueType->getConstantStrings(); | ||
|
|
||
| $hasFalse = false; | ||
|
|
@@ -77,7 +81,25 @@ | |
|
|
||
| return null; | ||
| } elseif ($hasDateTime) { | ||
| return $scope->getType($methodCall->var); | ||
| $callerType = $scope->getType($methodCall->var); | ||
|
|
||
| $dateTimeInterfaceType = new ObjectType(DateTimeInterface::class); | ||
| if ($dateTimeInterfaceType->isSuperTypeOf($callerType)->yes()) { | ||
|
Check warning on line 87 in src/Type/Php/DateTimeModifyReturnTypeExtension.php
|
||
| return $callerType; | ||
| } | ||
|
|
||
| return TypeTraverser::map( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw: the fact that we need such a hack here makes me ferl we likely need it somewhere else, as this logic does not look specific for DateTimeInterface
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's related to the fact we're trying to return Most of the dynamic returntype extension are doing and then remove the unwanted return type (like False) here. But it breaks a non regression test about Template for DateTime, because someone use modify on
And other extensions seems to handle such cases (with sometimes simpler checks) |
||
| $callerType, | ||
| static function (Type $type, callable $traverse) use ($dateTimeInterfaceType): Type { | ||
| if ($type instanceof UnionType) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need support for IntersectionType in the TypeTraverser callable?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because if you get a type
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All done — both the final test run (11775 tests passing) and static analysis (no errors) have completed. The branch is rebased on |
||
| return $traverse($type); | ||
| } | ||
| if ($dateTimeInterfaceType->isSuperTypeOf($type)->yes()) { | ||
| return $type; | ||
| } | ||
| return new NeverType(); | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| if ($this->phpVersion->hasDateTimeExceptions()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| <?php // lint >= 8.0 | ||
|
|
||
| declare(strict_types = 1); | ||
|
|
||
| namespace Bug11073Nsrt; | ||
|
|
||
| use DateTimeImmutable; | ||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| class HelloWorld | ||
| { | ||
| public function sayHello(?DateTimeImmutable $date): void | ||
| { | ||
| assertType('DateTimeImmutable|null', $date?->modify('+1 year')->setTime(23, 59, 59)); | ||
| } | ||
| } | ||
|
|
||
| class Foo | ||
| { | ||
| public function getCode(): bool { return false; } | ||
| } | ||
|
|
||
| class HelloWorld2 | ||
| { | ||
| public function sayHello(\Throwable|Foo $foo): void | ||
| { | ||
| assertType('bool|int|string', $foo->getCode()); | ||
| } | ||
|
|
||
| public function sayHello2(\LogicException|Foo $foo): void | ||
| { | ||
| assertType('bool|int', $foo->getCode()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,5 +45,27 @@ function (\DateTimeImmutable $dt, string $s): void { | |
| }; | ||
|
|
||
| function (?\DateTimeImmutable $d): void { | ||
| assertType('DateTimeImmutable|null', $d->modify('+1 day')); | ||
| assertType('DateTimeImmutable', $d->modify('+1 day')); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an assertType on
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests background task confirmed complete as well — 11733 tests passing. All done. |
||
| }; | ||
|
|
||
| function (?\DateTimeImmutable $d): void { | ||
| assertType('DateTimeImmutable|null', $d?->modify('+1 day')); | ||
VincentLanglet marked this conversation as resolved.
Show resolved
Hide resolved
staabm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }; | ||
|
|
||
| class Foo extends \DateTimeImmutable {} | ||
| class Bar { | ||
| /** @return string */ | ||
| public function modify($string) {} | ||
| } | ||
| class Bar2 { | ||
| /** @return string|false */ | ||
| public function modify($string) {} | ||
| } | ||
|
|
||
| function foo(Foo|Bar $d): void { | ||
| assertType('DateFormatReturnType\Foo|string', $d->modify('+1 day')); | ||
| }; | ||
|
|
||
| function foo2(Foo|Bar2 $d): void { | ||
| assertType('DateFormatReturnType\Foo|string|false', $d->modify('+1 day')); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| <?php // lint >= 8.1 | ||
|
|
||
| namespace MethodCallReturnTypeFallback; | ||
|
|
||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| enum Suit: string { | ||
| case Hearts = 'hearts'; | ||
| case Diamonds = 'diamonds'; | ||
| } | ||
|
|
||
| class MyClass { | ||
| /** @return self */ | ||
| public static function from(string $value): self { | ||
| return new self(); | ||
| } | ||
| } | ||
|
|
||
| /** @param class-string<Suit>|class-string<MyClass> $class */ | ||
| function testStaticCallOnUnionWithConstant(string $class): void { | ||
| assertType('MethodCallReturnTypeFallback\MyClass|MethodCallReturnTypeFallback\Suit::Hearts', $class::from('hearts')); | ||
| } | ||
|
|
||
| /** @param class-string<Suit>|class-string<MyClass> $class */ | ||
| function testStaticCallOnUnionWithVariable(string $class, string $value): void { | ||
| assertType('MethodCallReturnTypeFallback\MyClass|MethodCallReturnTypeFallback\Suit', $class::from($value)); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| <?php // lint >= 8.0 | ||
|
|
||
| declare(strict_types = 1); | ||
|
|
||
| namespace Bug11073; | ||
|
|
||
| use DateTimeImmutable; | ||
|
|
||
| class HelloWorld | ||
| { | ||
| public function sayHello(?DateTimeImmutable $date): ?DateTimeImmutable | ||
| { | ||
| return $date?->modify('+1 year')->setTime(23, 59, 59); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this change have also an impact on StaticCall which needs tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Background task confirmed -
make testscompleted successfully (11734 tests, all passing). Everything was already committed and pushed.