vimeo / psalm

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

Templates don't work with `instanceof self` #5370

Open webmozart opened 3 years ago

webmozart commented 3 years ago

In this particular case, Psalm reports an error:

https://psalm.dev/r/602a69dcbb

It works if I tell Psalm manually that the variable is of type self<S> after the instanceof check. However, I think that should be unnecessary:

https://psalm.dev/r/20b5a7c847

psalm-github-bot[bot] commented 3 years ago

I found these snippets:

https://psalm.dev/r/602a69dcbb ```php message = $message; } /** * @template S of object * * @param S|self $message * * @return self */ public static function wrap(object $message): self { if ($message instanceof self) { return new self($message->message); } return new self($message); } } ``` ``` Psalm output (using commit 3817193): ERROR: InvalidReturnStatement - 31:20 - The inferred type 'Envelope<(S:fn-envelope::wrap as object)|object>' does not match the declared return type 'Envelope' for Envelope::wrap ERROR: InvalidReturnType - 26:16 - The declared return type 'Envelope' for Envelope::wrap is incorrect, got 'Envelope<(S:fn-envelope::wrap as object)|object>|(Envelope)' ```
https://psalm.dev/r/20b5a7c847 ```php message = $message; } /** * @template S of object * * @param S|self $message * * @return self */ public static function wrap(object $message): self { if ($message instanceof self) { /** @var self $message */ return new self($message->message); } return new self($message); } } ``` ``` Psalm output (using commit 3817193): No issues! ```
webmozart commented 3 years ago

Apparently it works when I change T of object to T extends object. I guess that's a candidate for the docs. :)

weirdan commented 3 years ago

@webmozart you're second person mentioning this unsupported and ignored syntax today. May I ask where you got the idea of it, so I could try to prevent further confusion?

weirdan commented 3 years ago

Envelope matches both branches of that union, so if ($message instanceof Envelope) cannot rule out either.

webmozart commented 3 years ago

@weirdan Thanks for the feedback, that's not a coincidence. I found that other ticket, tried the solution and assumed it fixed my problem too.

I had assumed that the more specific of the two types in the union matches. I.e. if the union is object|Envelope<object> and I'm passing an Envelope<stdClass>, it would limit the type down to the more specific one (Envelope<object>) and rule out the more generic one (object). Is that something that can/should be fixed in your opinion?

webmozart commented 3 years ago

Also, I'm having a follow-up issue now: https://psalm.dev/r/a737ed289e

When I'm using a generic in an interface that doesn't care about the type, I'm receiving an error unless I explicitly specify template annotations again: https://psalm.dev/r/e89f740e03

But the latter cascades through the application. I suddenly need to add @template annotations in all kinds of places even though they are completely irrelevant. Am I doing something wrong?

psalm-github-bot[bot] commented 3 years ago

I found these snippets:

https://psalm.dev/r/a737ed289e ```php message = $message; } /** * @template S of object * * @param S|self $message * * @return self */ public static function wrap(object $message): self { if ($message instanceof Envelope) { /** @var self $message */ return new self($message->message); } return new self($message); } } interface Serializer { public function serialize(Envelope $envelope): string; public function deserialize(string $messageBody): Envelope; } class MyService { private Serializer $serializer; public function __construct(Serializer $serializer) { $this->serializer = $serializer; } public function doSomething(): string { $envelope = Envelope::wrap(new stdClass()); return $this->serializer->serialize($envelope); } } ``` ``` Psalm output (using commit 3817193): ERROR: InvalidArgument - 59:45 - Argument 1 of Serializer::serialize expects Envelope, Envelope provided ```
https://psalm.dev/r/e89f740e03 ```php message = $message; } /** * @template S of object * * @param S|self $message * * @return self */ public static function wrap(object $message): self { if ($message instanceof Envelope) { /** @var self $message */ return new self($message->message); } return new self($message); } } interface Serializer { /** * @template T of object * * @param Envelope $envelope */ public function serialize(Envelope $envelope): string; /** * @template T of object * * @return Envelope */ public function deserialize(string $messageBody): Envelope; } class MyService { private Serializer $serializer; public function __construct(Serializer $serializer) { $this->serializer = $serializer; } public function doSomething(): string { $envelope = Envelope::wrap(new stdClass()); return $this->serializer->serialize($envelope); } } ``` ``` Psalm output (using commit 3817193): No issues! ```
AndrolGenhald commented 3 years ago

This looks like another case where template invariance causes issues, but @template-covariant won't work because it needs to be used in method parameters. I need to spend more time looking at type invariance when I have the time, but the best explanation I've seen so far is from Rust. Unfortunately for us Rust's ownership, mutability, and lifetime rules let them deal with it in a way other languages can't. Psalm's documentation on @template-covariant is also helpful for understanding the issue. (Also, sorry for the confusion @webmozart!)

Edit: Perhaps adding extends as valid syntax could solve the issue, and it would work the way Java does?

webmozart commented 3 years ago

@AndrolGenhald Thanks a lot for the quick feedback, actually you solved my problem! Using @template-covariant in combination with @psalm-immutable worked. Thanks also for those links. So:

  • MyClass<T of BaseType> (not covariant) can be created with any subtype, but can then only be assigned to MyClass<SubType> and not to MyClass<BaseType>, while
  • MyClass<T extends BaseType> (your proposition) would allow the latter as well.

Did I understand that correctly?

If yes, then I think that would be a good replacement for @template-covariant, which (to me) is a bit obscure.

weirdan commented 3 years ago

Envelope matches both branches of that union, so if ($message instanceof Envelope) cannot rule out either.

Interestingly enough, TypeScript is able to figure out Envelope<S> in the equivalent code.

AndrolGenhald commented 3 years ago

@webmozart It's a bit more complicated than that. My understanding is still developing, so take this with a grain of salt, but I think you'd need @template T extends Dog to allow Dog or a subtype of Dog, but there are restrictions with what you can do with it.

/**
 * @template T extends Dog
 * @param Collection<T> $dogs
 */
function everyoneBark($dogs)
{
    foreach ($dogs as $dog) {
        $dog->bark();
    }
}

allows calling Dog methods on $dogs, but you can't add a Dog to $dogs, because maybe $dogs is actually a Collection<Poodle>.

You'd also probably want to have @template T super Cat meaning that T is either Cat or a supertype of Cat, so you can do

/**
 * @template T super Cat
 * @param Collection<T> $cats
 */
function addCat($cats)
{
    $cats->add(new Cat());
}

but you can't call any Cat functions on members of $cats, because maybe it's a Collection<Animal>.

Edit: Basically (if I'm getting this correct)

  • with @template T of Type T is invariant over Type
  • with @template T extends Type T is covariant over Type
  • with @template T super Type T is contravariant over Type