qossmic / deptrac

Keep your architecture clean.
https://qossmic.github.io/deptrac
MIT License
2.64k stars 134 forks source link

`@template-covariant` type always appears in Uncovered #1399

Open janedbal opened 8 months ago

janedbal commented 8 months ago

When analysing following file

<?php

namespace Debug;

/**
 * @template-covariant TEntityId
 */
interface TemplateInterface
{
    /**
     * @return TEntityId
     */
    public function getId(): mixed;

}

With this depfile:

deptrac:
  analyser:
    types:
      - "class"
  paths:
    - ./debug
  layers:
    - name: Foo
      collectors:
        - type: directory
          value: ./debug

I'm getting

 ----------- --------------------------------------------------------------------- 
  Reason      Foo                                                                  
 ----------- --------------------------------------------------------------------- 
  Uncovered   Debug\TemplateInterface has uncovered dependency on Debug\TEntityId  
              /app/debug/TemplateInterface.php:10                                  
 ----------- --------------------------------------------------------------------- 

Using just @template seems to work.

janedbal commented 8 months ago

The same problem occurs (arrayAlias reported) when using PHPStan type like this:

/**
 * @phpstan-type arrayAlias array<string, mixed>
 */
class SomeClass
{
    /**
     * @return arrayAlias
     */
    private function someMethod(): array
    {
        return [];
    }
}
patrickkusebauch commented 8 months ago

The same problem occurs (arrayAlias reported) when using PHPStan type like this:

/**
 * @phpstan-type arrayAlias array<string, mixed>
 */
class SomeClass
{
    /**
     * @return arrayAlias
     */
    private function someMethod(): array
    {
        return [];
    }
}

To be fair in this case it really should be @phpstan-return as you are defining @phpstan-type and therefore the referenced return should also only be scoped to @phpstan.

janedbal commented 8 months ago

@psalm-import-type has the same problem

patrickkusebauch commented 8 months ago

@psalm-import-type has the same problem

And I would propose the same solution. You should reference it with @psalm-return. This is a case where you would be always behind. As there is a potentially infinite number of tools that can define their prefixed tags. Deptrac does this as well with @deptrac-internal.

There is a larger argument that we should be able to fully support Psalm and PHPStan.

janedbal commented 8 months ago

The prefix is not really scoping, tools like PHPStan often introduce some features with prefixed annotation (like @phpstan-return), but later other tools (like PHPStorm) includes such feature in regular annotation (like @return). I have not met a single issue when combining prefixed and non-prefixed stuff since every tool reads annotations of the others. So we use non-prefixed version wherever possible.

But I understand this might be hard for deptrac to be in-sync with all the tools.

DanielBadura commented 8 months ago

I also encountered the same problem. But i just swooped them into the baseline. I mean, it would be nice if deptrac could understand it and work with it, but not a must have i think. Also, there are cases where you define phpstan-return and psalm-return due to different syntax approaches or missing feature in one tool (if you are using both, which i do). So this would also add extra complexity: which return are we then taking into account, or maybe even both?

janedbal commented 8 months ago

Currently, I'm using this to avoid this issue in general (as we use only class tokens):

class ExcludeUncoveredHandler implements EventSubscriberInterface
{

    public function handleUncovered(PostProcessEvent $event): void
    {
        $result = $event->getResult();

        /** @var Uncovered $uncovered */
        foreach ($result->allOf(Uncovered::class) as $uncovered) {
            if ($this->isUncoveredIgnored($uncovered)) {
                $result->remove($uncovered);
            }
        }
    }

    private function isUncoveredIgnored(Uncovered $uncovered): bool
    {
        /** @var class-string<object> $class */
        $class = $uncovered->getDependency()->getDependent()->toString();

        try {
            /** @throws ReflectionException */
            $reflectionClass = new ReflectionClass($class);
        } catch (ReflectionException $e) {
            return true; // prevent deptrac bugs like https://github.com/qossmic/deptrac/issues/1399
        }

        return false;
    }

    /**
     * @return array<string, array{string}>
     */
    public static function getSubscribedEvents(): array
    {
        return [
            PostProcessEvent::class => ['handleUncovered'],
        ];
    }

}
services:
  excludeUncoveredHandler:
    class: App\EventHandler\ExcludeUncoveredHandler
    tags:
      - { name: kernel.event_subscriber }
DanielBadura commented 8 months ago

This could catch also other errors no? I would suggest just to utilize the baseline feature. I mean, this is exactly meant for such cases.

janedbal commented 8 months ago

We have other tooling to detect non-existing classes in codebase, so this should be safe in our case. If deptrac finds non-class token, it clearly does something wrong and I dont want that to be reported.