Skip to content

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

Merged
samsonasik merged 5 commits intomainfrom
regression-multi-arg
Apr 24, 2023
Merged

[TypeDeclaration] Handle regression multiple params no longer working on AddMethodCallBasedStrictParamTypeRector#3681
samsonasik merged 5 commits intomainfrom
regression-multi-arg

Conversation

@samsonasik
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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