[TypeDeclaration] Skip nullable self referencing param and return on AddArrayParamDocTypeRector and ReturnTypeDeclarationRector #1183
Conversation
samsonasik
force-pushed
the
close-1182
branch
2 times, most recently
from
b7dfc83 to
b85d018
Compare
|
All checks have passed 🎉 @TomasVotruba it is ready for review. |
| private function hasUnionWithThisType(UnionType $unionType): bool | ||
| { | ||
| $types = $unionType->getTypes(); | ||
| foreach ($types as $type) { | ||
| if ($type instanceof ThisType) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
IMO this would skip any union type with ThisType. Instead, the ThisType should match the existing object type and compare it.
If they are equal, it shold be skipped. If they are no the same, they type should be overriden.
These 2 fixtures should be covered and work too:
class NullableSelf
{
private ?self $parent = null;
- public function getParent(): ?int
+ public function getParent(): ?self
{
return $this->parent;
}
}
?>class NullableSelf
{
private ?self $parent = null;
- public function getParent(): ?AnotherObject
+ public function getParent(): ?self
{
return $this->parent;
}
}
?>There was a problem hiding this comment.
@TomasVotruba I think those mostly a userbase invalid code which possibly on purpose. I tried that, but that cause invalid changes in other places due to check return expr and the return stmts as well.
There was a problem hiding this comment.
It might be the case, but this asumption might be wrong. It is rather about fixing the self comparison object that now does not work.
I tried that, but that cause invalid changes in other places due to check return expr and the return stmts as well.
It should be fixed there too then. Could you send this proposal? I'll see what fails there.
There was a problem hiding this comment.
I'm trying to remove this method to see the change, gimme a sec...
TomasVotruba
force-pushed
the
close-1182
branch
from
3d35c92 to
b8b4b7b
Compare
TomasVotruba
force-pushed
the
close-1182
branch
2 times, most recently
from
eeb06a2 to
5371ee1
Compare
| return $inferedType->isSubTypeOf($currentType) | ||
| return $currentType->isSubTypeOf($inferedType) | ||
| ->yes(); |
There was a problem hiding this comment.
This was the buggy part. Current type can be stricter then resolved, so if it subtype, it should be kept.
|
@samsonasik I've posted the solution. Could you review & merge? |
| @@ -476,6 +476,9 @@ public function getNode(): \PhpParser\Node | |||
| return $this->node; | |||
| } | |||
|
|
|||
| /** | |||
| * @param \PhpDocTagValueNode|null $phpDocTagValueNode | |||
There was a problem hiding this comment.
this is invalid, there is no \PhpDocTagValueNode class, only PHPStan\PhpDocParser\Ast\PhpDoc\PhpDocTagValueNode, also, typed param already cover it.
|
rectify now got error /~https://github.com/rectorphp/rector-src/runs/4138103624?check_suite_focus=true#step:6:5794
[ERROR] Could not process
"rules/EarlyReturn/Rector/Return_/PreparedValueToEarlyReturnRector.php"
file, due to:
"Serialization of 'Closure' is not allowed". On line: 33 |
|
I've missed the Rector job, will check 👍 |
TomasVotruba
force-pushed
the
close-1182
branch
from
c1b4ff6 to
0b2ceb4
Compare
| return serialize($type) . $type->isExplicitMixed(); | ||
| return $type->describe(VerbosityLevel::precise()) . $type->isExplicitMixed(); |
There was a problem hiding this comment.
In some wierd cases, the MixedType contains closures that cannot be serialized.
|
CI is passing. Ready for review 👍 |
|
Thank you @TomasVotruba |
Closes #1182 Fixes rectorphp/rector#6789