Skip to content
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

[TypeDeclaration] Handle regression multiple params no longer working on AddMethodCallBasedStrictParamTypeRector #3681

Merged
merged 5 commits into from
Apr 24, 2023

Conversation

samsonasik
Copy link
Member

@internalsystemerror this is I try to cover regression that happen when dealing with infinite loop on PR:

which multiple params which one of them is Union Type no longer working now.

@samsonasik
Copy link
Member Author

Call:

$this->phpStanStaticTypeMapper->mapToPhpParserNode($unionType, $typeKind);

On union type narrow bool seems was the source why cause infinite loop

https://github.com/rectorphp/rector-src/pull/3681/files#diff-88741beb0f7f0d97e0197e1e8903ababaacf70ec8c75e4d785367837e9265db9

Let's try...

return $this->phpStanStaticTypeMapper->mapToPhpParserNode($unionType, $typeKind);
return null;
Copy link
Member Author

Choose a reason for hiding this comment

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

this seems cause error test:

There was 1 failure:

1) Rector\Tests\Php80\Rector\FunctionLike\UnionTypesRector\UnionTypesRectorTest::test with data set #23
Failed on fixture file "narrow_bool_false2.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 
 class NarrowBoolFalse2
 {
-    public function go(bool|int $param): bool|int
+    /**
+     * @param bool|int|false $param
+     * @return bool|int|false
+     */
+    public function go($param)

I will check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's check that it has bool type before try to apply narrow bool 3199d38

Copy link
Member Author

Choose a reason for hiding this comment

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

skipping re-map on union type is safer, see comment #3681 (comment)

@samsonasik
Copy link
Member Author

samsonasik commented Apr 24, 2023

@TomasVotruba while this bool type is working 3199d38, is working, I think re-check by

$this->phpStanStaticTypeMapper->mapToPhpParserNode($unionType, $typeKind);

is still dangerous for passing union type in union type mapper, as that kind of usage can be vary in multiple types with same sub, eg: add IntegerRangeType, I am going to just skip when doctype types count is not equal with union type node as this can happen again.

like this :

class SkipNarrowBoolFalseAnotherType
{
    /**
     * @param bool|int|false|int<0, max> $param
     * @return bool|int|false
     */
    public function go($param)
    {
        if (rand(0, 1)) {
            return rand(0, 1) ? true : false;
        }

        return 1;
    }
}

got type again https://3v4l.org/jfN4m

so:

@param skip narrow bool|false|<OtherTypeHere>

will be skipped, see b3ac267

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba I am merging it ;)

@samsonasik samsonasik merged commit 2efca73 into main Apr 24, 2023
@samsonasik samsonasik deleted the regression-multi-arg branch April 24, 2023 17:07
@samsonasik
Copy link
Member Author

@eugeniya-v FYI, bool|false|AnotherType docblock is now just skipped on UnioTypesRector as may cause invalid type again when docblock extended, eg: bool|int|false|int<0, max> that goes to bool|false|int again and may cause infinite loop on no bool type when used on AddMethodCallBasedStrictParamTypeRector, only bool|false that converted to bool that converted

see :

samsonasik added a commit that referenced this pull request May 8, 2023
… on AddMethodCallBasedStrictParamTypeRector (#3681)

* [TypeDeclaration] Handle regression multiple params no longer working on AddMethodCallBasedStrictParamTypeRector

* rollback tweak

* try fixing

* try with check has bool type

* skip narrow bool|false|<OtherTypeHere>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant