Fix issue #7755 about lost @phpstan-type

[VincentLanglet]

Reproducer for https://github.com/phpstan/phpstan/issues/7755 with a proposal of fix.

Prior to this PR, only the stub was used if found. Now I try to merge both the ResolvedPhpDocBlock from the stub and the ResolvedPhpDocBlock from the file.

For tag like @param, @return, @throws, @var, I follow the same strategy than the merge method. So I tried to use the stub definition and if not found, I use the original file (like it was a parent).

For tag like @type, @import-type, @extends, @implements, @method, @property (and mixin but dunno what it is), I merge the tags. For duplicates, the priority is for the stub.

For tag like @final, @internal, I considered this as defined as long as one of the two files (stub or original) has it. IMHO, if the original file is defining his class as final, we shouldn't be able to override this in the stub (same idea for others).

[ondrejmirtes]

I imagined that https://github.com/phpstan/phpstan/issues/7755 should be fixed together with https://github.com/phpstan/phpstan/issues/5091, looks like a similar problem. I'm not sure if merging everything like that is the right fix, feels too heavy-handed to me, it could have some unwanted side effects for a bugfix...

[ondrejmirtes]

I think the actual bug is that this place looks into the stub file but it shouldn't: https://github.com/phpstan/phpstan-src/blob/73127d1b271011c9a552cde7f4cfd694a26cd831/src/Type/UsefulTypeAliasResolver.php#L95-L96

But when we remove that possibility, type aliases will stop working in stub files which also isn't great...

[ondrejmirtes]

Maybe @jiripudil could look into this? The context is these two bugs:

1. Type aliases don't work correctly in traits: https://github.com/phpstan/phpstan/issues/5091
2. Missing @phpstan-type X in a stub file (overriden PHPDoc) breaks references to X in the original file when there's @phpstan-type X in the original file: https://github.com/phpstan/phpstan/issues/7755

I imagine we should do the aliases resolving sooner, doing it in FileTypeMapper would surely solve this, but maybe it'd introduce other problems, like circular and infinite resolving. This looks pretty similar to how @template tags are gathered and resolved in FileTypeMapper, down to how it works for traits, maybe we could get some inspiration there...

[VincentLanglet]

I'm not sure if merging everything like that is the right fix, feels too heavy-handed to me, it could have some unwanted side effects for a bugfix...

Yeah, I understand this could be considered as an important change. I added test but I agree we would need more.

I think the actual bug is that this place looks into the stub file but it shouldn't:

https://github.com/phpstan/phpstan-src/blob/73127d1b271011c9a552cde7f4cfd694a26cd831/src/Type/UsefulTypeAliasResolver.php#L95-L96

But when we remove that possibility, type aliases will stop working in stub files which also isn't great...

Investigating on https://github.com/phpstan/phpstan/issues/7755 makes me find some similar issues and I'm not sure looking for a fix for this special case is the best idea.

---

Class annotation

If the file is

/**
 * @final 
 */
class Foo extends Bar {}

but the stub is

/**
 * @phpstan-extends Bar<\DateTime>
 **/
class Foo extends Bar {}

currently since you're using only the stub phpdoc, the class won't be reported as final.

Same for @internal.

These issues are similar to the @phpstan-type ones.

---

Method annotation

If the file is

/** @param array<string> $array */
function foo($int, $array) {}

but the stub is

/** @param positive-int $int */
function foo($int, $array) {}

then $array will be reported as mixed by phpstan.

We can ask to copy-paste the @param definition in the stub ; but if one day the vendor update the phpdoc to

/** @param array{name: string, firstname: string} $array */
function foo($int, $array) {}

the stub will need to be updated of the array will still be considered as string[] because of the stub.

So it might be better to allow updating the phpdoc of only one param in the phpdoc for stubs.

And similar to the class annotations, if @final is added for a method in one file and we have phpdoc in our stub, phpstan won't report the method as final. (or pure, or internal...).

---

So IMHO before starting to solve https://github.com/phpstan/phpstan/issues/7755, you should decide about the situation:

• for which tag, the fact we lose the annotation from the original file when there is a stub is a bug.
• for which tag, the fact we lose the annotation from the original file when there is a stub is a wanted behavior.

Because if it's considered as a bug for a lot of tag, a general way to fix it like I did seems better.

[VincentLanglet]

I'm not sure if merging everything like that is the right fix, feels too heavy-handed to me, it could have some unwanted side effects for a bugfix...

If you like the idea of merging but you're affraid of all the unwanted side effects, we can go slowly.

fillWith could be flagged as internal temporary and will take everything on $this except merging the type and import-type, with a specific test. Then in another PR, we could add the merge of the param/return/throw in the implementation of the fillWith with specific tests. And then some others tags, etc.

Since everything is done inside fillWith it can be done step by step, one tag at a time.

[ondrejmirtes]

There should be the principle of least surprise applied here. I was surprised that https://github.com/phpstan/phpstan-src/pull/1603/commits/57b37222fe0e3142a0e2f2717c229ab1b8ad4713 doesn't work as expected because @phpstan-type should be applied as local substitution. Meaning that if we have this original file:

/**
 * @phpstan-type X = int
 */
class Foo
{
    /** @return X */
    public function doFoo()
    {
    }

    public function doBar()
    {
    }
}

And this stub file:

/**
 * @phpstan-type X = string
 */
class Foo
{

    /** @return X */
    public function doBar()
    {
    }
}

In the end the return type of doFoo should be int and the return type of doBar should be string.

That's why I think that merging tags doesn't help at all, it just makes things more complex and behave even less likely than intended here...

[VincentLanglet]

There should be the principle of least surprise applied here. I was surprised that 57b3722 doesn't work as expected because @phpstan-type should be applied as local substitution. Meaning that if we have this original file:

I see your point. But I'm not sure about the "local substitution".

With your example if I do @phpstan-import-type X, in another file ; I assume it will use the type of the stub. So writing

/**
 * @phpstan-import-type X from Foo
 */
class SuperFoo extends Foo
{
    /** @return X */
    public function doFoo()
    {
    }
}

will throw a phpstan error doFoo() returns string but returns int (like defined in his parent).

This might be confusing too. That's why I thought that considering that doFoo() returns an string in your example could be less confusing.

Or maybe you meant that @phpstan-import-type X from Foo should consider the type of the real file Foo, and not the stub Foo ?

[VincentLanglet]

Is there anything I can do to move this PR forward @ondrejmirtes

I found another issue similar to @phpstan-type but for @throws tag. Cf https://github.com/phpstan/phpstan-symfony/pull/302

I see three solutions here:

• "Fixing" the behavior for all the tags, like I tried to do in this PR. But you were not convinced about it.
• "Fixing" the behavior for only a set of specific tag like @phpstan-type, @throws and maybe some others.
• Only fixing @phpstan-type because you considered as expected for @throws tag.

[ondrejmirtes]

This really isn't urgent as we fixed the problem in https://github.com/phpstan/phpstan-doctrine/pull/355.

I'd accept a PR that implements this: https://github.com/phpstan/phpstan-src/pull/1603#issuecomment-1213045346

And it should fix https://github.com/phpstan/phpstan/issues/5091 at the same time because it's a related problem.