[AddArrayReturnDocTypeRector] sets a less specific type in child method (mixed[]) than is defined in parent method (SomeObject[])

[gnutix]

I'm curious to have your opinion / expertise on this one. I personally don't use {@inheritdoc} nor duplicate PHPDoc between parents and children, as PHPStorm and PHPStan both perfectly understand parent definitions. So here Rector is trying to add the PHPDoc to the child, and I don't think it should.

Not sure how to implement the fix though, if you've got a clue it might get me going (now that I'm getting the hang of it ;) ).

[TomasVotruba]

Don't depend on tooling too much :)

Missing doc opens possible human bug:

 public function someMethod(): array
 {
-    return ['test', 'test2'];
+    return ['test', 2];
 }

That's why fix is correct here

[gnutix]

PHPStan would catch this, as it's related to type checking.

In my understanding, the purpose of this Rector isn't to prevent bug (type checking), but to automate / ease adding tons of phpdoc where they were missing. If it's already set on the parent, it should most likely be the same on the children (I tried adding /** {@inheritdoc} */ and it was replaced too).

Maybe this could be an optional behavior using an option ? (skip_if_parent_definition_exists: true)

[gnutix]

don't rely too much on tooling

PHP won't ever break on a phpdoc ; so there's no other way but to rely on tools for this... Not sure I get your point. 🤔

[TomasVotruba]

Rectors' purpose is to ease every day development. Having look at other file to understand current one, is not easing users work.

[gnutix]

Then it is overlapping with PhpStan here, right ? Is that something you want ? (I'm more and more feeling the need to have a clearer vision of Rector's purpose and boundaries as I get involved in fixes)

[TomasVotruba]

The vision is best desribed here: https://getrector.org/

This feature is just one of many that can help with that. Many tool are overlapping, coding standard upgrade code, Rector completes types, that PHPStan and PHPStorm use for better static analysis. It's the complete ecosystem, that makes every developer that uses PHP super powerful. Each of them is very important.

In this specific case, the added annotation helps user to write code that is expected. Not manually, but deduced from the code from static analysis, a completed for the sure. That's the main goal of Rector.

[gnutix]

Thanks! I realize now that I should have tried to get a more representative failing test (simplifying it allowed to have it failed, but we lost the context, which is probably key for this issue).

Here's what's really happening in my project :

1. The parent (Entity1RepositoryInterface) sets a return type phpdoc of @return Entity1[] on a method findByEntity2Id(Uuid $id): array;.
2. The child (Entity1DoctrineRepository) implements the method without phpdoc

When running Rector's AddArrayReturnDocTypeRector, it adds @return mixed[] to the child's findByEntity2Id method.

So my problem here isn't really the fact that the phpdoc gets written both in the Interface and the children (though it does to some extends - I'd rather not have it twice for maintainability reason, favoring a "single source of truth")... but what really bothers me is that the added type hint in the child is less specific than the parent (mixed[] vs Entity1[]) : which breaks auto-completion.

I tried adding a failing test case with my code sample, but as often with code depending on third party libraries (Doctrine here), I couldn't get it to trigger. I'll try one more time in src/Whatever...

[gnutix]

I had to set it up in my "playground" repository : https://github.com/gnutix/rector-playground/pull/2/files

Output :

Rector dev-master@a21b805
Config file: rector.yaml

[parsing] src/DoctrineRepository.php
[parsing] src/Entity1.php
[parsing] src/Entity1DoctrineRepository.php
[parsing] src/Entity1RepositoryInterface.php
[parsing] src/Entity2.php
[refactoring] src/DoctrineRepository.php
[refactoring] src/Entity1.php
[refactoring] src/Entity1DoctrineRepository.php
[refactoring] src/Entity1RepositoryInterface.php
[refactoring] src/Entity2.php
[printing] src/DoctrineRepository.php
[printing] src/Entity1.php
[printing] src/Entity1DoctrineRepository.php
[printing] src/Entity1RepositoryInterface.php
[printing] src/Entity2.php

1 file with changes
===================

1) src/Entity1DoctrineRepository.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -9,6 +9,9 @@
  */
 final class Entity1DoctrineRepository extends DoctrineRepository implements Entity1RepositoryInterface
 {
+    /**
+     * @return mixed[]
+     */
     public function findByEntity2(Entity2 $entity2): array
     {
         return $this->repository->findBy(['entity2Id' => $entity2->getId()]);
    ----------- end diff -----------

Applied rules:

 * Rector\TypeDeclaration\Rector\ClassMethod\AddArrayReturnDocTypeRector
 
 [OK] Rector is done! 1 file would have changed (dry-run).                           

[TomasVotruba]

I'm getting lost in this. What is the issue now?

This should be closed/merged with fix, before we get into something else.

[gnutix]

My last two comments explain the real issue I'm having (which seems very similar to #2229). Ignore the PR's changes, it's an overly simplified version which doesn't really illustrate it.

Seems like the fix might be something very similar to #2236, though I don't know how to adapt this exactly.

[TomasVotruba]

I didn't understand that comment. Diff of what you expect vs. what happen would help me more.

Also, be sure to have doctrine extension for phpstan registered in phpstan.neon, as it affects PHPStan stat inferrs the types.

[gnutix]

Let's put it this way :

Given

https://github.com/gnutix/rector-playground/pull/2/files#diff-fca573ab6299edf8fd35919f85924a1eR7-R12

<?php
interface Entity1RepositoryInterface
{
    /**
     * @return Entity1[]
     */
    public function findByEntity2(Entity2 $entity2): array;

and https://github.com/gnutix/rector-playground/pull/2/files#diff-02998c9ee6503ef2b7470bba2d3e78b7R10-R15

<?php
final class Entity1DoctrineRepository extends DoctrineRepository implements Entity1RepositoryInterface
{
    public function findByEntity2(Entity2 $entity2): array
    {
        return $this->repository->findBy(['entity2Id' => $entity2->getId()]);
    }

What Rector does

--- Original
+++ New
@@ -9,6 +9,9 @@
  */
 final class Entity1DoctrineRepository extends DoctrineRepository implements Entity1RepositoryInterface
 {
+    /**
+     * @return mixed[]
+     */
     public function findByEntity2(Entity2 $entity2): array
     {
         return $this->repository->findBy(['entity2Id' => $entity2->getId()]);

What I would prefer (solution 1)

No changes whatsoever.

What I would find acceptable (solution 2)

--- Original
+++ New
@@ -9,6 +9,9 @@
  */
 final class Entity1DoctrineRepository extends DoctrineRepository implements Entity1RepositoryInterface
 {
+    /**
+     * @return Entity1[]
+     */
     public function findByEntity2(Entity2 $entity2): array
     {
         return $this->repository->findBy(['entity2Id' => $entity2->getId()]);

[TomasVotruba]

Btw, tip for syntax highlight. "diff" after the ``` makes it nice and clear for the eyes

[gnutix]

@TomasVotruba Is there a Rector that has some "parent aware" logic for types ? (given a method, find if it has a parent and analyze it) If you point me out to an example, I could try to implement a fix (given you decide on which solution to go for).

[TomasVotruba]

Sorry for delaying answer, I had to think hard about feature you needed.

I don't think there is anything that does what you need, but you can get inspiration at VendorLockResolver - https://github.com/rectorphp/rector/blob/7c8a8618fe947d30baef655affa7ea9fb48655c1/packages/vendor-locker/src/VendorLockResolver.php

[TomasVotruba]

I'll look on it

[TomasVotruba]

The test case now shows correctly added string[] in #2866

Possibly fixed by type refactoring