is_a() usage in the codebase

[ondrejmirtes]

Hi, I see many instances of is_a() usages in the codebase. It's not compatible with static reflection as it needs the classes loaded in runtime. It's fine when you ask about stuff that's already loaded (like PhpParser nodes, PHPStan classes, phpdoc-parser classes), but it shouldn't be used for analysed code.

[samsonasik]

@ondrejmirtes should instanceof be used? The instanceof seems only working if left value is an object, not string, ref https://3v4l.org/mpvp2

[ondrejmirtes]

instanceof and is_a are runtime concepts. But here you're dealing with classes that aren't loaded at runtime, that aren't available through an autoloader. You should use PHPStan's facilities like PHPStan\Reflection\ClassReflection and PHPStan\Type\Type::isSuperTypeOf().

[ondrejmirtes]

For example this piece of code:

    /**
     * @see https://doc.nette.org/en/3.0/components#toc-components-with-dependencies
     */
    public function isComponentFactoryInterface(Interface_ $interface): bool
    {
        foreach ($interface->getMethods() as $classMethod) {
            $returnType = $this->returnTypeInferer->inferFunctionLike($classMethod);
            if (! $returnType instanceof TypeWithClassName) {
                return false;
            }

            $className = $this->nodeTypeResolver->getFullyQualifiedClassName($returnType);
            if (is_a($className, 'Nette\Application\UI\Control', true)) {
                return true;
            }

            if (is_a($className, 'Nette\Application\UI\Form', true)) {
                return true;
            }
        }

        return false;
    }

Should be refactored like that:

    /**
     * @see https://doc.nette.org/en/3.0/components#toc-components-with-dependencies
     */
    public function isComponentFactoryInterface(Interface_ $interface): bool
    {
        foreach ($interface->getMethods() as $classMethod) {
            $returnType = $this->returnTypeInferer->inferFunctionLike($classMethod);
            $controlOrForm = new UnionType([
                new ObjectType('Nette\Application\UI\Control'),
                new ObjectType('Nette\Application\UI\Form'),
            ]);
            
            return $controlOrForm->isSuperTypeOf($returnType)->yes();
        }

        return false;
    }

[ondrejmirtes]

Instead of ObjectTypeComparator::isCurrentObjectTypeSubType() you should also use Type::isSuperTypeOf for the same purpose. It works automatically between all types, and you don't have to worry about runtime/static reflection either...

[ondrejmirtes]

Instead of:

        foreach ($parametersAcceptor->getParameters() as $position => $parameterReflection) {
            $parameterType = $parameterReflection->getType();
            if (! $parameterType instanceof TypeWithClassName) {
                continue;
            }

            if (! is_a($parameterType->getClassName(), Throwable::class, true)) {
                continue;
            }

            return $position;
        }

You should do:

        foreach ($parametersAcceptor->getParameters() as $position => $parameterReflection) {
            $parameterType = $parameterReflection->getType();
            if (!(new ObjectType(\Throwable::class))->isSuperTypeOf($parameterType)->yes()) {
                continue;
            }

            return $position;
        }

[samsonasik]

@ondrejmirtes Thank you 👍

[TomasVotruba]

@ondrejmirtes Thanks for real examples. I though the ClassReflection was always needed to compare with parents, so good to know simple ObjectType will do 👍

I tried to remove all those func calls in #5665, but missed a few.

We should have a PHPStan rule for that, so we can avoid missing it in the code-reviews. The place when we found out in bug reports is too late.

[ondrejmirtes]

You should also be careful with more runtime concepts like class_exists and also use ReflectionProvider for that. The best way to be sure that Rector fully works with static reflection would probably be to figure out how to run the whole test suite a second time, but with a disabled autoloader so that the static reflection source locators will need to pick up all the sources.

[TomasVotruba]

Re-opening to keep track of this, untill PHPStan rule checks are added and we're sure there are no leftovers.