GetClassToInstanceOfRector invalid fix to instanceof for strict type check

[jeremyVignelles]

Bug Report

Subject	Details
Rector version	v1.0.4

I'm using getclass($object) === A::class to test if my variable is exactly of type A (and not a subclass). Rector tries to edit this into an instanceof check, due to the GetClassToInstanceOfRector rule

Minimal PHP Code Causing Issue

<?php

class A {}
class B extends A {}

$c = new B();

if (get_class($c) === A::class) {
    echo 'KO - it is exactly of type A';
} else {
    echo 'OK - it is not exactly of type A';
}

Code gets "fixed" to $c instanceof \A

Expected Behaviour

I should be able to express that I am willing to keep that get_class call to check that exact type.

[jeremyVignelles]

Seems like $c::class === A::class is OK for rector, is that the recommended thing to do ?

[samsonasik]

@TomasVotruba @jeremyVignelles I am thinking that this rule should only cover !== instead of ===, as it seems valid? it requires some verification tho...

<?php

class A {}
class B extends A {}

$c = new B();

-if (get_class($c) !== A::class) {
+if (! $c instanceof A) {
   
}

wdyt?

[samsonasik]

Oh, the result seems different https://3v4l.org/7cbU6

[samsonasik]

If the type is exactly of what it defined (sub type), it can be checked by something like:

        $varType = $this->nodeTypeResolver->getNativeType($varNode);
        $objectType = new ObjectType($className);

        if ($varType instanceof ObjectType
            && ! $this->typeComparator->areTypesEqual($varType, $objectType)
            && $this->typeComparator->isSubtype($varType, $objectType)) {
            return null;
        }

some verification on dynamic variable may be needed. I guess for your use case, you can skip the rule:

https://getrector.com/documentation/ignoring-rules-or-paths

as it may be usefull to improve type check, but on some very specific use case, it may cause a bug.

[TomasVotruba]

Oh, the result seems different 3v4l.org/7cbU6

I'm looking at this rule, it seems it can produce invalid code quite easily.

It should only pass on classes that are final.

[TomasVotruba]

Looking at the code again, it can only work for already known types:

final class OnlyFinal
{
    public function test(SomeFinalClass $other)
    {
        return get_class($other) !== SomeFinalClass::class;
    }
}

Which is kind of non-sense check. This rule can break code and should be handled explicitly.

On the other hand, it's been working fine for past 6 years, so we might keep it despite this side effect. https://github.com/rectorphp/rector-src/commit/aee1866dee8116d9e4ee6eb5a84f7551f95250e8

[samsonasik]

@TomasVotruba get_class() is PHP 7.x syntax, this rule is contradictory with ClassOnObjectRector as PHP 8.x rule, see this demo:

PHP 7.x GetClassToInstanceOfRector become instanceof:

• https://getrector.com/demo/67f35314-d225-4397-8402-81d46edd204d

PHP 8.x ClassOnObjectRector become ===

• https://getrector.com/demo/bd945bc6-6531-4182-bc92-2750c61e7f5d

Once it changed to ::class, it no longer changed to instanceof, see

(no change as using ::class already when back to use GetClassToInstanceOfRector ):

https://getrector.com/demo/d6755a3f-05a9-491c-bb24-fb67d3bf1c4f

The solution can be:

• check both get_class() and ::class usage
• verify both A and B the class is final by keyword

The type can be checked via its native type:

        $varType = $this->nodeTypeResolver->getNativeType($varNode);
        $objectType = new ObjectType($className);

        if (! $varType instanceof ObjectType) {
              // nothing can't be test
             return null;
        }
       
       $classVar = $this->reflectionProvider->getClass($varType->getClassName();
       $targetVar = $this->reflectionProvider->getClass($className);

       if ($classVar->isFinalByKeyword() && $targetVar->isFinalByKeyword()) {
             // process ...
      }

[TomasVotruba]

@samsonasik Looking at GetClassToInstanceOfRector checks, not sure it brings any value now. What seems the best option for you?

[samsonasik]

I think the rule can be deprecated/removed.

But if we want to keep it, we need to:

• check both get_class() and ::class usage for php 7.x and php 8.x support
• verify both A and B are both objects compared type and final, when not object, we skip it as we don't know what it is.

that will make it consistent.

[TomasVotruba]

Thanks :+1:

I'll take the deprecation then. The case A and B will be both objects and final seems super rare. These cases are better to check with PHPStan and upgrade with care.