Avoid circular reference of issue #9039

[VincentLanglet]

This close phpstan/phpstan#9039 but maybe you'll want another strategy @ondrejmirtes (and have one in mind).

I tried to copy the CircularTypeAliasDefinitionException.

There is the following issue:
The first time we're calling ClassReflection::getParentClass, we doing:

• ClassReflection::getFirstExtendsTag
• ClassReflection::getExtendsTags
• ... We trying to solve @template-extends Voter<self::*> so we're solving the constant ...
• ClassReflection::getConstant
• PhpDocInheritanceResolver::resolvePhpDocForConstant
• ... We trying to solve the FOO constant by looking at the parent classes phpdoc
• PhpDocBlock:: getParentReflections
• ClassReflection::getParentClass
• And then again we're looking for extends tags...

What's annoying here is the fact that there is no need to look at the generic when resolving the parent phpdoc for the constant since it doesn't exist in the parent. So resolving the extends tag is not needed what's why try catching the circular reference is doing the job.

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

You're just catching and reacting to the circular problem. But this code:

/**
 * @template-extends Voter<self::*>
 */
class Test extends Voter
{
	public const FOO = 'Foo';
	private const RULES = [self::FOO];
}

is valid and should be allowed. So the circular reference should be prevented, not just caught and reported.

[VincentLanglet]

You're just catching and reacting to the circular problem. But this code:

/**
 * @template-extends Voter<self::*>
 */
class Test extends Voter
{
	public const FOO = 'Foo';
	private const RULES = [self::FOO];
}

is valid and should be allowed. So the circular reference should be prevented, not just caught and reported.

I'm not sure if I don't understand you or if my test wasn't explicit enough.
I added an assertion about the error.

The unused constant was reported, not the circular reference.
So to me, this code is now allowed and doesn't break phpstan or is reported as an error.

[ondrejmirtes]

I just tried it:

<?php declare(strict_types = 1);

namespace Bug9039;

use function PHPStan\Testing\assertType;

/**
 * @template T
 */
class Voter
{
	/** @return T */
	public function test()
	{

	}
}

/**
 * @template-extends Voter<self::*>
 */
class Test extends Voter
{
	public const FOO = 'Foo';
	private const RULES = [self::FOO];
}

function (Test $t): void {
	assertType("'Foo'|array{'Foo'}", $t->test());
};

Looks like it works and the type is correctly resolved, but I have no idea why. Looks a bit hacky to me. It'd be better for the circular problem to be avoided altogether.

The infinite circular chain is usually broken by stopping doing something that is not necessary.

What's annoying here is the fact that there is no need to look at the generic when resolving the parent phpdoc for the constant since it doesn't exist in the parent.

Yeah, so don't look at it.

PhpDocInheritanceResolver and PhpDocBlock could probably be rewritten so that they don't fetch parent classes at all if the thing (constant/property/method) isn't defined in them at all.

[VincentLanglet]

Looks like it works and the type is correctly resolved, but I have no idea why. Looks a bit hacky to me. It'd be better for the circular problem to be avoided altogether.

PhpDocInheritanceResolver and PhpDocBlock could probably be rewritten so that they don't fetch parent classes at all if the thing (constant/property/method) isn't defined in them at all.

How do I know if the constant/property/method is not defined in them if I don't compute them ?

• Should I introduce a "lazy" bool to getParentClass to just return

$this->reflectionProvider->getClass($this->reflection->getParentClass()->getName())

and then I can call hasConstant on it.

• Should I introduce a new method parentClassHasConstant ?

• Is there another way ?

[ondrejmirtes]

I don't know, I'd have to look deeper into it. I'd challenge this item first:

We trying to solve the FOO constant by looking at the parent classes phpdoc

I don't understand why this is done actually.

[VincentLanglet]

I don't know, I'd have to look deeper into it. I'd challenge this item first:

We trying to solve the FOO constant by looking at the parent classes phpdoc

I don't understand why this is done actually.

Looking at the code, ClassReflection::getConstant is looking for the resolvedPhpDoc of the constant to know if the constant is deprecated, internal and the phpdoc of the constant before instantiating

$this->constants[$name] = new ClassConstantReflection(
				$this->initializerExprTypeResolver,
				$declaringClass,
				$reflectionConstant,
				$phpDocType,
				$deprecatedDescription,
				$isDeprecated,
				$isInternal,
			);

Maybe for something like

/**
 * @template T of int|string
 */
class Base
{
	/**
	  * @deprecated
      * @var T
      */
    public const FOO = 'foo';
}

/**
 * @template-extends Base<int>
 */
class Test extends Base
{
	public const FOO = 42;
}

This way phpstan knows

• the const foo must be limited to an int value (don't know if generics make sens for constants...)
• the const foo is deprecated from the parent

[VincentLanglet]

Yeah, so don't look at it.

PhpDocInheritanceResolver and PhpDocBlock could probably be rewritten so that they don't fetch parent classes at all if the thing (constant/property/method) isn't defined in them at all.

I tried to implement this strategy @ondrejmirtes ; please take a new look at this PR :)

[ondrejmirtes]

I almost merged this but then I realized the fatal flaw here. You're filtering the interfaces and parent class, and regardless of what gets filtered, you cache that result. And when the result is already cached, you return the cached result regardless of the filter callback.

[VincentLanglet]

There is two things:

You're filtering the interfaces and parent class, and regardless of what gets filtered, you cache that result.

This was a big flaw indeed ; I should not cache the result when a filterCallback is used.
Nice catch.

And when the result is already cached, you return the cached result regardless of the filter callback.

Here, I was hesitating and I'd like to have your point of view.

Maybe the filterCallback name is not the best, the idea was the following:

• If the result is cached, use it because it require no computation
• If the result is not cached, use the filterCallback to early return null/less class/interface in order to avoid extra computation which are useless (like resolving parentClass/interface without the constant I look for)
• If the full result is computed without filter, cache it

So the filterCallack is not a way to guarantee less results, it a way to guarantee less computations.

I could change to

if ($this->cachedInterfaces !== null && $filterCallback === null) {
    return $this->cachedInterfaces;
}

but this would means that every time I run resolveParentPhpDocBlocks I would call

$classReflection->getParentClass($filterCallback)

and not use the cache, ending with lower performance for PHPStan tool.

Would it be better with another name than filterCallback ?

[ondrejmirtes]

I feel like I don't want to make ClassReflection API to be more complicated and potentially slower just for some bug avoidance in PhpDocBlock.

I'm gonna try something, wait a moment.

[ondrejmirtes]

Solved it a different and much easier way. Reused your test. Thank you!

[VincentLanglet]

Solved it a different and much easier way. Reused your test. Thank you!

I feel like you finally did all the job, sorry for this.
Thanks a lot for the bug fix ; I'll take time to understand it.