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

UndefinedDocblockClass when using template in static method #2571

Closed
jenschude opened this issue Jan 8, 2020 · 15 comments
Closed

UndefinedDocblockClass when using template in static method #2571

jenschude opened this issue Jan 8, 2020 · 15 comments

Comments

@jenschude
Copy link

While trying to create factory methods I found out that templates are not correctly recognized in static methods.

<?php
  
/**
 * @template T
 */
abstract class Foo {
  /**
   * @psalm-param T $data
   */
  public static function bar($data) {
  }
}

See https://psalm.dev/r/6d29fb978b

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/6d29fb978b
<?php
  
/**
 * @template T
 */
abstract class Foo {
  /**
   * @psalm-param T $data
   */
  public static function create($data) {
  }
}
Psalm output (using commit 146dd46):

ERROR: UndefinedDocblockClass - 8:19 - Docblock-defined class or interface T does not exist

INFO: MissingReturnType - 10:26 - Method Foo::create does not have a return type, expecting void

@weirdan
Copy link
Collaborator

weirdan commented Jan 8, 2020

This is intentional: 797a059

Can you explain what you're trying to achieve? Most likely you want @template tag on the static method itself: https://psalm.dev/r/340c710984

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/340c710984
<?php

/** @template TT */
class Foo {
  /**
   * @template T
   * @psalm-param T $data
   * @return self<T>
   */
  public static function create($data) {
    return new self;
  }
}
atan(Foo::create(22));
Psalm output (using commit 146dd46):

ERROR: InvalidArgument - 14:6 - Argument 1 of atan expects float, Foo<int(22)> provided

ERROR: UnusedFunctionCall - 14:1 - The call to atan is not used

@jenschude
Copy link
Author

This was the minimum example which replicated the issue itself. But I was dealing with an abstract class which had a final factory method. The inheriting classes defined the type.

But yeah seems that I can solve it with your proposal. 👍

@staabm
Copy link
Contributor

staabm commented Jan 30, 2021

We experienced a similar problem in redaxo/redaxo#4359 in which we wanted to template a trait with a single type across all its methods

@kamazee
Copy link

kamazee commented Jul 4, 2021

Can you explain what you're trying to achieve?

I wanted to use a template on a param of a static method in Enum class from myclabs/php-enum. Template there defines a value type in a "enum"; template in expanded in child class; parent's from method gets a enum entry by value. Here is a commit where I though it would work: myclabs/php-enum@a128308 (full file can be seen from there; this is a one-class library).

Here is a minimal example if a file there is too big to read: https://psalm.dev/r/402a8f5b50

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/402a8f5b50
<?php

/**
 * @template T
 */
class Enum {
    /**
     * @param mixed $bar
     * @psalm-param T $value
     * @return static
     */
    public static function from($value): self
    {
        // Actually it looks up an enum entry by value but this is just a stub
        return new static();
    }
    
    // ...
    // Here go more magic methods that make methods from annotation below work
}

/**
 * @method static self RED()
 * @method static self GREEN()
 * @extends Enum<string>
 */
class TrafficLightSignals extends Enum {
    private const RED = 'red';
    private const GREEN = 'green';
}

TrafficLightSignals::from('red');
Psalm output (using commit 3abea66):

ERROR: InvalidArgument - 32:27 - Argument 1 of TrafficLightSignals::from expects T, "red" provided

ERROR: UndefinedDocblockClass - 9:21 - Docblock-defined class, interface or enum named T does not exist

INFO: MixedInferredReturnType - 10:16 - Could not verify return type 'Enum' for Enum::from

@orklah
Copy link
Collaborator

orklah commented Jul 4, 2021

You can't mix class templates and static templates by design. Would you mind providing a more detailed reproducer (where usage of $value is visible) on a new Issue if you have questions?

@christian-kolb
Copy link
Contributor

@weirdan @orklah Here is an example in my project where I need the behaviour you mentioned as being handled that way by design:

/** @psalm-immutable */
abstract class Id implements \Stringable
{
    final public function __construct(
        protected readonly string $value,
    ) {
        if (!uuid_is_valid($value)) {
            throw new InvalidId($value);
        }
    }
}

/** @psalm-immutable */
class UserId extends Id
{
}

/**
 * @template T extends Id
 *
 * @template-implements \IteratorAggregate<int, T>
 */
abstract class IdList implements \IteratorAggregate, \Countable
{
    /** @var array<int, T> */
    public readonly array $ids;

    /** @param array<int, T> $ids */
    final public function __construct(
        array $ids,
    ) {
        $this->ids = array_values($ids);
    }

    /** @return class-string<T> */
    abstract public static function handlesIdClass(): string;
}

/** @extends IdList<UserId> */
final class UserIdList extends IdList
{
    public static function handlesIdClass(): string
    {
        return UserId::class;
    }
}

The only workaround I found for it is adding the following:

/**
 * @template TT of T
 *
 * @return class-string<TT>
 */
abstract public static function handlesIdClass(): string;

But this then breaks PHPStan, because it interpretes the @psalm annontations.

What was the reason for disabling the template syntax for static methods? I only see the commit, but don't know why this was done. Would this be a valid example to reconsider the previous decision? 🙂

@orklah
Copy link
Collaborator

orklah commented Nov 21, 2022

Based on your example, let's say I create a class AdminUserId extends UserId.

I then make sure to make an array of AdminUserId and I make a new UserIdList($myArrayOfAdminUserId);

I have a UserIdList<AdminUserId>. Let's say I call handlesIdClass on that. The docblock says it should return AdminUserId::class when in fact, it will return UserId::class.

What you did is not completely safe from a type POV. You lied on the template you're returning because the information you have on a static method is not enough to fully describe the template of what's inside your List. To be able to describe it accurately, you'd need to have access to the list itself, so you can only do that from an instance, not from a static method.

This is this kind of confusion that lead Psalm to stop allowing class level template on static methods.

I think this could be solved in two ways for your specific example:

  • Psalm could understand that if UserId is declared final there is no risk of getting anything else and we could assume static methods are allowed to have insight on the template type because of that
  • We could invent some way of declaring a template that specifically says you can't push childs into it (maybe a new keyword aside from as and of?) and again, because the template type is certain, we could allow static methods to know/use the template type

However, I don't have enough hindsight to know if it would be enough to solve the confusion for all situations here

@christian-kolb
Copy link
Contributor

christian-kolb commented Nov 21, 2022

Thanks for your extensive reply. I'm not sure whether I can follow your logic 100%.

I like the example of yours but I think there are two relevant notes:

  • When I want to store AdminUserId objects, I would create an AdminUserIdList with a handlesIdClass of AdminUserId.
  • Even if I wouldn't create an AdminUserIdList but put AdminUserId objects into my UserIdList, they are technically also UserId objects. So the typing is still valid. It's less precise, but I might want that because I there might also be a ManagerUserId (extending UserId) objects in that same list.

You are correct, that using UserIdList<AdminUserId> would be incorrect. But isn't that simply a wrong annotation? I could also write the following

/** @var int $test */
$test = 'bla';

and Psalm won't complain there.

At the moment I can't use Psalm to configure a valid behaviour.

Aren't there more cases where it is useful than harmful?

@orklah
Copy link
Collaborator

orklah commented Nov 21, 2022

While trying to make a working example, I realized two mistakes I made:

  • you didn't create a new template on UserIdList because you pushed UserId in the extends so UserIdList<AdminUserId>.I guess it would be a third way to hint at Psalm that the class knows the type of T.
  • Your example seems to work already: https://psalm.dev/r/dc8b18bcbd I'm confused, I'd have expected Psalm to raise an error on using T on the static method here

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/dc8b18bcbd
<?php

/** @psalm-immutable */
abstract class Id
{
    final public function __construct(
        protected readonly string $value,
    ) {

    }
}

/** @psalm-immutable */
class UserId extends Id
{
}

/**
 * @template T extends Id
 *
 * @template-implements \IteratorAggregate<int, T>
 */
abstract class IdList implements \IteratorAggregate, \Countable
{
    /** @var array<int, T> */
    public readonly array $ids;

    /** @param array<int, T> $ids */
    final public function __construct(
        array $ids,
    ) {
        $this->ids = array_values($ids);
    }

    /** @return class-string<T> */
    abstract public static function handlesIdClass(): string;
}

/** @extends IdList<UserId> */
final class UserIdList extends IdList
{
    public static function handlesIdClass(): string
    {
        return UserId::class;
    }
}

$b = UserIdList::handlesIdClass();
/** @psalm-trace $b */;
Psalm output (using commit 7869cb5):

INFO: Trace - 49:23 - $b: class-string<UserId>

INFO: UnusedVariable - 48:1 - $b is never referenced or the value is not used

ERROR: UnimplementedInterfaceMethod - 40:13 - Method getiterator is not defined on class UserIdList

@christian-kolb
Copy link
Contributor

Wow that is strange, it actually seems to work on @return. Maybe it always did and I found it on @param and just assumed it's the same for @return. I adapted the example, the problem is when using it with @param: https://psalm.dev/r/56b205a8cd

Would you see the same kind of issues? Shouldn't this be a valid case even when using AdminUserId and ManagerUserId objects on it?

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/56b205a8cd
<?php

/** @psalm-immutable */
abstract class Id
{
    final public function __construct(
        protected readonly string $value,
    ) {

    }
}

/** @psalm-immutable */
class UserId extends Id
{
}

/**
 * @template T extends Id
 *
 * @template-implements \IteratorAggregate<int, T>
 */
abstract class IdList implements \IteratorAggregate, \Countable
{
    /** @var array<int, T> */
    public readonly array $ids;

    /** @param array<int, T> $ids */
    final public function __construct(
        array $ids,
    ) {
        $this->ids = array_values($ids);
    }

    /** @return class-string<T> */
    abstract public static function handlesIdClass(): string;
    
    /** @param array<int, T> $ids */
	final public static function fromIds(array $ids): static
	{
    	return new static($ids);
	}
    
    /** @return \Iterator<int, T> */
	public function getIterator(): \Iterator
	{
    	return new \ArrayIterator($this->ids);
	}
    
    public function count(): int
    {
        return count($this->ids);
    }
}

/** @extends IdList<UserId> */
final class UserIdList extends IdList
{
    public static function handlesIdClass(): string
    {
        return UserId::class;
    }
}

$b = UserIdList::handlesIdClass();
/** @psalm-trace $b */;
Psalm output (using commit 7869cb5):

INFO: Trace - 66:23 - $b: class-string<UserId>

INFO: UnusedVariable - 65:1 - $b is never referenced or the value is not used

ERROR: UndefinedDocblockClass - 38:16 - Docblock-defined class, interface or enum named T does not exist

INFO: MixedInferredReturnType - 39:52 - Could not verify return type 'IdList' for IdList::fromIds

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

No branches or pull requests

6 participants