Incorrect behavior of AddMethodCallBasedStrictParamTypeRector about rector HOT 11 CLOSED

Already typed param is now skipped at latest rector/rector:dev-main https://getrector.com/demo/cdd077c2-cdc9-40a3-b65f-5834bdcff7f8

I am closing it, feel free to create PR for another improvement you think it needs.

from rector.

If accepted as a bug, and the proposed solution is accepted as well, I can also create a PR to fix this :).

from rector.

Reproducer for the issue with inheritance I'm seeing: https://getrector.com/demo/f1bf72fb-736a-4f24-9cb9-acd28c0ade86

The do(FooInterface $foo) typehint is replaced here. This, most likely, because FooInterface doesn't accept a MockObject (based on reading the code for the rector there seems to be some logic in "only replace if the set typehint doesn't accept the passed in type), while in reality it would accept it. It obviously would be better for Rector to recognize this (it might do so with phpstan/phpstan-phpunit installed / enabled?), but I (also) think that it shouldn't replace typehints, and as stated, I think it doesn't do so for many rules (i.e.: it will leave type hints already in the code, even if they are wrong / lead to bugs).

from rector.

Reproducer for the other issue I'm seeing: https://getrector.com/demo/4bd39e82-cd8b-4db0-a70b-25714e173f23

Here the parameter is based on a value in an array. And as the array doesn't have any typing information it (Rector or PHPStan?) assumes the type would be a string (I guess PHPStan just assumes it will be a array<array-key, string>?). While the actual value is a stdClass. So the new string typehint will make the code fail, as an stdClass is passed which obviously isn't accepted by the string type. While the pre-existing typehint of object worked fine.

from rector.

I see, if param->type already a \PhpParser\Node instance, then it should be skipped. Feel free to create a PR for it 👍

from rector.

I'll try to work on this over the weekend, and already had a quick look at the code when I created the ticket. But it seems there is a lot of logic involved to handle the "should param type be updated or not?" (i.e.: is the existing type compatible with the "new" / determined type). Which makes me wonder whether it wasn't intended as a feature to (sometimes) override the param type.

So I already have the basic change including a test written. But I think a lot of the existing code could / should be removed, which is this:
https://github.com/rectorphp/rector-src/blob/76714b95f240409a6fad92725ea19c870ed3ff14/rules/TypeDeclaration/NodeAnalyzer/ClassMethodParamTypeCompleter.php#L108-L132

Haven't tested that yet though (so see what the impact is on existing tests / behavior).

from rector.

I see, in that case, that seems need check that existing type is inside narrowed type or its subtype, if that totally different, just skip it.

from rector.

I think it's currently the other way around. If you have class Child extends Parent and Rector determines Child is passed and the type set in code is Parent it will leave it as is, but if the determined type is something entirely else (like string) it goes for "these types aren't compatible, so change it to what actually is passed".

But in this case the determined type is wrong. In case of the array example it might be possible that PHPStan / Rector "shows" it's unconfined about the type, i.e.: just guessing the type of the array value? (possibly a maybe instead of a yes / no). For the other scenario, involving an intersection type, I'm not sure whether PHPStan / Rector knows that they at least partially overlap?

from rector.

Or well, actually, in case of the intersection type I think the "is a" kinda test actually fails 🤔

When I have:

interface FooInterface {}
class Foo implements FooInterface {}

Then some param type hinted with FooInterface will always allow a value of type Foo (because "Foo is a FooInterface"). So it should / must also allow something which is Foo&MockObject, because the additional type doesn't matter. If I would have class Foo implements FooInterface, MockObject (/class Foo extends MockObject implements FooInterface) then it should (and might already) pass anyway. So when an intersection type is passed and the param type is a normal (singular / non compound) type it is enough to check whether one of the types of the intersection type are allowed by the set typehint.

So in the example where the determined type is Foo&MockObject and the actual typehint is FooInterface it should check "is a Foo allowed to be passed to a FooInterface param? Or is a MockObject allowed to be passed to a FooInterface param?". To which the answer of the first question is "yes", and thus the type shouldn't / mustn't be updated.

from rector.

You might be able to scratch my previous comment. I misremembered, I thought the updated type was an intersection, while it actually is a union.

Not sure though, because both the Rector stubs as the actually included (/vendored) PHPUnit describe it as an intersection:
Stub:

        /**
         * @psalm-template RealInstanceType of object
         *
         * @psalm-param class-string<RealInstanceType> $originalClassName
         *
         * @psalm-return MockObject&RealInstanceType
         */
        protected function createMock(string $originalClassName): MockObject
        {
        }

Actual:

    /**
     * Creates a mock object for the specified interface or class.
     *
     * @psalm-template RealInstanceType of object
     *
     * @psalm-param class-string<RealInstanceType> $originalClassName
     *
     * @psalm-return MockObject&RealInstanceType
     *
     * @throws \PHPUnit\Framework\MockObject\Exception
     * @throws InvalidArgumentException
     * @throws NoPreviousThrowableException
     */
    protected function createMock(string $originalClassName): MockObject

So I don't understand where the union is coming from, because the PHPDoc clearly describes an intersection of MockObject & RealInstanceType.

from rector.

The stub is checked with class_exists, so that's seems fine. The fix can be in multiple PR steps, eg:

• first PR: skip totally different current type with narrowed type
• next PR: improve verify unnecessary union
• etc

from rector.