vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.56k stars 660 forks source link

False positive UnusedParam in Doctrine Repository method #9216

Open ndench opened 1 year ago

ndench commented 1 year ago

I'm unsure if this issue belongs here, or the psalm-plugin-doctrine, or the doctrine/doctrine-bundle or doctrine/orm but I'll explain what's going on and hopefully get a steer as to where the issue actually lays.

I'm currently using and getting no psalm issues being raised.

vimeo/psalm: 4.30.0
weirdan/doctrine-psalm-plugin: 2.8.1
doctrine/orm: 2.14.1
doctrine/dbal: 3.5.3
doctrine/doctrine-bundle: 2.8.0

Upgrading from doctrine/doctrine-bundle:2.8.0 to either 2.8.1 or 2.8.2 causes every repository method to report either UnusedParam or PossiblyUnusedParam.

Downgrading back to 2.8.0 causes these issues to go away.


For example, here is a simple repository which reports a PossiblyUnusedParam for the $user parameter in the findForUser method.

<?php

declare(strict_types=1);

namespace App\Repository;

use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
use Doctrine\Persistence\ManagerRegistry;
use App\Entity\ConditionSet;
use App\Entity\User;

/**
 * @extends ServiceEntityRepository<ConditionSet>
 */
class ConditionSetRepository extends ServiceEntityRepository
{
    public function __construct(ManagerRegistry $registry)
    {
        parent::__construct($registry, ConditionSet::class);
    }

    /**
     * @return ConditionSet[]
     */
    public function findForUser(User $user): array
    {
        $qb = $this->createQueryBuilder('cs');

        /** @var ConditionSet[] $result */
        $result = $qb->where('cs.user = :user')
            ->setParameter('user', $user)
            ->getQuery()
            ->getResult()
        ;

        return $result;
    }
}

And here is another example that reports UnusedParam for both parameters in the findForProjectInSourceSystem method.

<?php

declare(strict_types=1);

namespace App\Repository;

use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
use Doctrine\Persistence\ManagerRegistry;
use App\Entity\ProjectLink;
use App\Entity\Project;
use App\Enum\SourceSystem;

/**
 * @extends ServiceEntityRepository<ProjectLink>
 */
final class ProjectLinkRepository extends ServiceEntityRepository
{
    public function __construct(ManagerRegistry $registry)
    {
        parent::__construct($registry, ProjectLink::class);
    }

    public function findForProjectBySourceSystem(Project $project, SourceSystem $sourceSystem): ?ProjectLink
    {
        /** @var null|ProjectLink $result */
        $result = $this->createQueryBuilder('pl')
            ->where('pl.project = :project')
            ->andWhere('pl.sourceSystem = :sourceSystem')
            ->setParameter('project', $project)
            ->setParameter('sourceSystem', $sourceSystem)
            ->getQuery()
            ->getOneOrNullResult()
        ;

        return $result;
    }
}

Any help tracking this down would be greatly appreciated :heart:

ndench commented 1 year ago

No sorry, psalm.dev doesn't have doctrine installed so it just reports UndefinedClass: https://psalm.dev/r/6ed9f511f1

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/6ed9f511f1 ```php */ final class ExternalProjectLinkRepository extends ServiceEntityRepository { public function __construct(ManagerRegistry $registry) { parent::__construct($registry, ProjectLink::class); } public function findForProjectBySourceSystem(Project $project, SourceSystem $sourceSystem): ?ProjectLink { /** @var null|ProjectLink $result */ $result = $this->createQueryBuilder('pl') ->where('pl.project = :project') ->andWhere('pl.sourceSystem = :sourceSystem') ->setParameter('project', $project) ->setParameter('sourceSystem', $sourceSystem) ->getQuery() ->getOneOrNullResult() ; return $result; } } ``` ``` Psalm output (using commit 54cd529): ERROR: UndefinedClass - 16:51 - Class, interface or enum named Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository does not exist ```
weirdan commented 1 year ago

What happens if you get rid of fluent calls? E.g.

public function findForProjectBySourceSystem(Project $project, SourceSystem $sourceSystem): ?ProjectLink
{
        $a = $this->createQueryBuilder('pl');
        /** @psalm-trace $a */; 
        $b = $a->where('pl.project = :project');
        /** @psalm-trace $b */;
        $c = $b->andWhere('pl.sourceSystem = :sourceSystem');
        /** @psalm-trace $c */;
        $d = $c->setParameter('project', $project);
        /** @psalm-trace $d */;
        $e = $d->setParameter('sourceSystem', $sourceSystem);
        /** @psalm-trace $e */;
        $f = $e->getQuery();
        /** @psalm-trace $f */;
        $g = $f->getOneOrNullResult();
        /** @psalm-trace $g */;

        $result = $g;
        return $result;
}

This is just to narrow down the issue, I'm not implying that you should normally write your code this way.

ndench commented 1 year ago

The UnusedParam issues are still reported. Although for some reason the @psalm-trace annotation doesn't seem to be doing anything. I've run it with --show-info=true and it doesn't seem to print any trace information.

weirdan commented 1 year ago

I think this is caused by https://github.com/doctrine/DoctrineBundle/pull/1599/files

With it, the code becomes essentially this: https://psalm.dev/r/a09e8d74db

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/a09e8d74db ```php f($a); /** @psalm-trace $a */; } } new D; ``` ``` Psalm output (using commit 234787b): ERROR: DuplicateClass - 6:11 - Class C has already been defined in /var/www/vhosts/psalm.dev/httpdocs/src/somefile.php INFO: UnusedClass - 4:11 - Class C is never used INFO: UnusedClass - 12:13 - Class D is never used INFO: UnusedParam - 13:29 - Param #1 is never referenced in this method ```
weirdan commented 1 year ago

I think the fix (at least partially) belongs in https://github.com/psalm/psalm-plugin-doctrine

Related:

ndench commented 1 year ago

Nice work on the reproducer! Is Psalm becoming confused by the duplicate class definitions? I've seen this pattern a few times before I. Symfony and Doctrine.

Would a fix in the doctrine plugin just overwrite the class definition? Should Psalm perhaps handle the duplicate definition better?

MihailSerov commented 11 months ago

you can temporally fix it by this:

<?php

use Doctrine\Bundle\DoctrineBundle\Repository\LazyServiceEntityRepository;
use Doctrine\Persistence\ManagerRegistry;

/**
 * @template T of object
 * @template-extends LazyServiceEntityRepository<T>
 *
 * @psalm-suppress InternalClass
 */
class ServiceEntityRepository extends LazyServiceEntityRepository
{
    /**
     * @psalm-param class-string $entityClass
     *
     * @psalm-suppress InternalMethod
     */
    public function __construct(ManagerRegistry $registry, string $entityClass)
    {
        parent::__construct($registry, $entityClass);
    }
}

class MyRepository extends ServiceEntityRepository
{
}