Phpstan: Improve PHPDoc inheritance

[JanTvrdik]

There are two main issues, I'm aware of

1) PHPDoc should be inherited without explicitly specifying {@inheritdoc}
2) The parameter names may differ in child from parent class / interface, so you need to remap them based on position.

[SignpostMarv]

re 1: yes, it's cleaner to have class members without having {@inheritdoc} peppered everywhere (especially with the proliferation of interfaces etc.)

re 2: position-based remapping should take care to go via the original parameter order, not the phpdoc def order- I can foresee the following code causing issues if this isn't taken into account:

interface Foo {
    /**
    * @param string $bat this is a parameter that does some stuff
    * @param int $baz this is a parameter that does some stuff before $bat does some stuff
    */
    public function bar($baz, $bat) : Foo;
}

class Thing implements Foo
{
    public function bar($totallyImportantThing, $passItInButItWillBeIgnored) : Foo
    {
    }
}

with $baz being defined before $bat, but $bat being documented before $baz, an implied-mode {@inheritdoc} PHPDoc parser shouldn't map Foo::bar::$bat to Thing::bar::$totallyImportantThing

[muglug]

Interfaces cannot be inherited from without running into issues.

For example:

interface I1 {
  /** @var int $bar */
  public static function foo($bar) {}
}

interface I2 {
  /** @var string $bar */
  public static function foo($bar) {}
}

class A implements I1, I2 {
  // which interface should be inherited here?
  public static function foo($bar) {}
}

But obviously subclasses don't have this problem

[iluuu1994]

@muglug I'd say, either:

• Merge the types (return type int|string)
• Throw an error and require an explicit PHPDoc

I don't actually think this is such a common thing though.

[iluuu1994]

I just realized merging is not a good idea since I1 and I2 promise that the types will be strictly int or string respectively.

[Majkl578]

Wanted to say that this has pretty obvious solution: follow PHP.
But it does not. When you implement two interfaces with same methods that match (i.e. typeless like yours), the latter interface is athoritative for method signatures: https://3v4l.org/s0juS
But when they have different signature, pretty strange Fatal errors are emitted:
https://3v4l.org/oWPT3 vs. https://3v4l.org/5ILHi
The error is correct of course (implementation can't satisfy both interfaces), but the wording is pretty strange - it doesn't check for conflicts, only validates inheritance.

Otherwise, proposal with merging violates LSP - subtype may never return more wider value.

[iluuu1994]

@Majkl578 Exactly.

IMO PHPStan should treat PHPDoc types the same as type hints. So an error is probably appropriate.

[soullivaneuh]

Typical case with Doctrine.

Interface: https://github.com/doctrine/collections/blob/ba63bf914b56e21989eeb99d36bed219900f37c1/lib/Doctrine/Common/Collections/Collection.php#L200-L208

ArrayCollection implementing the interface: https://github.com/doctrine/collections/blob/ba63bf914b56e21989eeb99d36bed219900f37c1/lib/Doctrine/Common/Collections/ArrayCollection.php#L286-L291

On this kind of code (the property is initialized as ArrayCollection):

/**
 * @return Organization[]|ArrayCollection
 */
public function getOrganizations()
{
    return $this->organizationMembers->map(function (OrganizationMember $organizationMember) {
        return $organizationMember->getOrganization();
    });
}

I have:

Method AppBundle\Entity\User::getOrganizations() should return Doctrine\Common\Collections\ArrayCollection&iterable<AppBundle\Entity\Organization> but returns Doctrine\Common\Collections\Collection.

This does not make any sense, the inheritdoc overrides must be handled IMO.

[ondrejmirtes]

@Soullivaneuh I don't see how your post relates to this issue. PHPStan is telling you that you're promising ArrayCollection (more specific) to be returned, but instead you're returning Collection. Change your phpDoc to Organization[]|Collection and see if it helps.

If it does not help, you should write a dynamic return type extension to define what exactly map method does - it'd help you also to add a return typehint to the closure...

[muglug]

@ondrejmirtes it definitely relates to this issue.

Reproduced more succinctly here:

• https://phpstan.org/r/0cf66a8babde94e707aa0d19c61a2ef2 (errors)
• https://phpstan.org/r/a714ae8a1b14eaf891b470b50f0035eb (no errors)

The only difference is the {@inheritDoc} annotation.

[soullivaneuh]

@muglug @ondrejmirtes exactly. The map method I use come from ArrayCollection and really returns a ArrayCollection object. It's specified on the PHPDoc I linked.

[localheinz]

Also see http://docs.phpdoc.org/guides/inheritance.html#the-inheritdoc-tag>

Important Currently some applications have DocBlocks containing just the {@inheritDoc} inline tag to indicate that their complete contents should be inherited. This usage breaks with the PHPDoc Standard as summaries cannot contain inline tags and inheritance is automatic; you do not need to define a special tag for it.

Also, though

However, it does make clear that an element has been explicitly documented (and thus not forgotten). As such we are working to include a new (normal) tag in the PHPDoc Standard @inheritDoc that will serve that purpose.

As we know, work on the proposed PSR-5 has stopped, somehow. However, I think it would be nice if phpstan/phpstan was smart enough to also automatically inherit DocBlocks.

🤓

[Stadly]

I have opened a separate issue concerning handling of @inheritDoc and {@inheritDoc}: https://github.com/phpstan/phpstan/issues/1332

The reason I created a separate issue is that my issue is concerning the handling of those two annotations, while this issue seems to cover implicit inheritance of annotations.

[iluuu1994]

This will become much more evidence once #1292 is merged.

class BaseClass
{
    /**
     * @return int
     */
    public function foo()
    {
        return 42;
    }
}

class SubClass extends BaseClass
{
    public function foo()
    {
        return 43;
    }
}

Declaration of SubClass::foo(): mixed should be compatible with BaseClass::foo(): int

[Majkl578]

@iluuu1994 I am very strongly against making _no doc comment_ imply _mixed_. This also violates phpDoc specification:

When a superclass of the current class contains a method with the same name (hence, this method is re-defined) then the following information is inherited from that overridden method:

• Summary
• Description
• The following tags:
  
  
  
  • author
  
  
  • copyright
  
  
  • version
  
  
  • param
  
  
  • return
  
  
  • throws
  
  

As with classes, each of the above will only be inherited if the redefined method’s DocBlock does not have the element that is to be inherited.

Until this behaves correctly, there should be no rule that builds on errorneous behavior.

[iluuu1994]

@Majkl578 That's what PHPStan already does.

[Majkl578]

@iluuu1994 Then I don't understand your previous comment. You are presenting a subclass without the doc block and this error reported (which indicated it does not follow phpDoc rules):

_Declaration of SubClass::foo(): mixed should be compatible with BaseClass::foo(): int_

[iluuu1994]

There's a new rule coming (hopefully, there's a bit of controversy) that checks that overridden methods stay compatible with their parent counterparts. PHPStan assumes that methods without a return type return mixed today. But there's no rule to inform you when that happens.

[Majkl578]

PHPStan assumes that methods without a return type return mixed today.

Well that's wrong of course (hence this issue) and should become a blocker for landing the new rule, otherwise it becomes a serious bug. You can't introduce a properly working rule for inheritance as long as this issue is not solved.

[Majkl578]

Btw Doctrine contains a lot of methods that only specify:

/**
 * {@inheritDoc}
 */

I believe we should drop all these blocks as this behavior is implied and is also against phpDoc standard (see the red warning):

Important: Currently some applications have DocBlocks containing just the {@inheritDoc} inline tag to indicate that their complete contents should be inherited. This usage breaks with the PHPDoc Standard as summaries cannot contain inline tags and inheritance is automatic; you do not need to define a special tag for it.

I think we should drop all these occurences in Doctrine. But that would mean PHPStan will start treating tons of the inheritance-related code as _mixed_, right?

[iluuu1994]

Well that's wrong of course (hence this issue)

When the method has no parent that is not wrong. You cannot deduce a return type without looking at the methods implementation. mixed is appropriate here. But obviously when the method does have a parent we should assume it has the same signature.

should become a blocker for landing the new rule, otherwise it becomes a serious bug

Exactly. Which is why I'm saying we should fix this issue. I'm on your side buddy 😆

[muglug]

I agree with @majkl578 - any subclass should be assumed to return the same exact thing. This is Psalm’s default behaviour because I don’t think violating LSP is cool

[Majkl578]

When the method has no parent that is not wrong.

Of course, we are still talking about overriden methods. :smiley:

Which is why I'm saying we should fix this issue.

:+1: Maybe you can also add it explicitly to #1292 as a blocker?

Btw I opened https://github.com/slevomat/coding-standard/issues/399 to start mitigating useless {@inheritDoc}. :blush:

[iluuu1994]

@Majkl578 Sure, unfortunately I don't have the privileges to do that.
@ondrejmirtes Can you add #671 as a dependency of #1292?

[ondrejmirtes]

Solved with #1533. If there's a need for the parameter name remapping, it can be submitted separately.

[ondrejmirtes]

And a little bit more than a year later, parameter name remapping is also implemented :) https://github.com/phpstan/phpstan-src/commit/ae238a12a14880da07c33e6817161ece59e685c0

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

[ondrejmirtes]

Another improvement to PHPDoc inheritance - definitions from parents are now merged with child classes! So even if there's a PHPDoc for @param B $b in the child class, PHPStan will also see@param A $a from the parent class, etc.

Implemented by @alexeyinkin in https://github.com/phpstan/phpstan-src/pull/196. Awesome work!