Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@template-covariant type always appears in Uncovered #1399

Open
janedbal opened this issue Mar 11, 2024 · 9 comments
Open

@template-covariant type always appears in Uncovered #1399

janedbal opened this issue Mar 11, 2024 · 9 comments

Comments

@janedbal
Copy link

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

@psalm-import-type has the same problem

@patrickkusebauch
Copy link
Collaborator

patrickkusebauch commented Mar 11, 2024

@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
Copy link
Author

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
Copy link
Contributor

DanielBadura commented Mar 18, 2024

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants