search

vimeo / psalm

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

Type inheritance not considered in templated values #7739

Open gmazzap opened 2 years ago

gmazzap commented 2 years ago

Consider the simplest templated class:

/** @template T */
class Value
{
    /** @param T $value */
    public function __construct(private mixed $value) { }

    /** @return T */
    public function value()
    {
        return $this->value;
    }
}

now consider the function:

/** @return Value<Iterator> */
function test(int $num): Value {
     if ($num > 0 && $num <= 100) {
         return new Value(new ArrayIterator(range(0, $num)));
     }

    return new Value(new EmptyIterator());
 }

Of course, both ArrayIterator and EmptyIterator implement Iterator, so in theory there's nothing wrong with the function. Still what I get is:

ERROR: InvalidReturnStatement - The inferred type 'Value<ArrayIterator<int, int>>' does not match the declared return type 'Value' for test ERROR: InvalidReturnStatement - The inferred type 'Value' does not match the declared return type 'Value' for test ERROR: InvalidReturnType - The declared return type 'Value' for test is incorrect, got 'Value<ArrayIterator<int, int>>|Value'

The only way to have no issues with the function is:

/** @return Value<ArrayIterator<int, int>|Value<EmptyIterator> */
function testInvariant(int $num): Value {
     if ($num > 0 && $num <= 100) {
         return new Value(new ArrayIterator(range(0, $num)));
     }

    return new Value(new EmptyIterator());
}

But declaring type like this is much more complicated.

Even because the following won't work either:

The strictness of templated values even go further.

See things like:

/**  @return Value<string|int>  */
function testScalar(int $num): Value {
     if ($num > 0 && $num <= 100) {
         return new Value($num);
     }

    return new Value("Too big");
}

/**  @return Value<string>  */
function testString(string $string): Value {
     if ($string === '') {
         throw new Error();
     }

    return new Value($string);
}

Fails with:

ERROR: InvalidReturnStatement - The inferred type 'Value<int<1, 100>>' does not match the declared return type 'Value<int|string>' for testScalar ERROR: InvalidReturnType - The declared return type 'Value<int|string>' for testScalar is incorrect, got 'Value<'Too big'>|Value<int<1, 100>>'

ERROR: InvalidReturnStatement - The inferred type 'Value' does not match the declared return type 'Value' for testString ERROR: InvalidReturnType - The declared return type 'Value' for testString is incorrect, got 'Value'

So int does not suffice, I've to use int<1, 100>; and string does not suffice, I've to use non-empty-string.

I understand supprt for such strictness, but forcing it does not seem correct. With these strictnesss declaring generics is much more difficult, and generic as a feature is much less usable.

See https://psalm.dev/r/a427924638

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

I found these snippets:

https://psalm.dev/r/a427924638 ```php value; } } /** * @return Value */ function test(int $num): Value { if ($num > 0 && $num <= 100) { return new Value(new ArrayIterator(range(0, $num))); } return new Value(new EmptyIterator()); } /** * @return Value>|Value */ function testInvariant(int $num): Value { if ($num > 0 && $num <= 100) { return new Value(new ArrayIterator(range(0, $num))); } return new Value(new EmptyIterator()); } /** * @return Value|Value> */ function testInvariantAlt(int $num): Value { if ($num > 0 && $num <= 100) { return new Value(new ArrayIterator(range(0, $num))); } return new Value(new EmptyIterator()); } /** * @return Value */ function testScalar(int $num): Value { if ($num > 0 && $num <= 100) { return new Value($num); } return new Value("Too big"); } /** @return Value */ function testString(string $string): Value { if ($string === '') { throw new Error(); } return new Value($string); } ``` ``` Psalm output (using commit 569e97d): ERROR: InvalidReturnStatement - 21:17 - The inferred type 'Value>' does not match the declared return type 'Value' for test ERROR: InvalidReturnStatement - 24:12 - The inferred type 'Value' does not match the declared return type 'Value' for test ERROR: InvalidReturnType - 17:12 - The declared return type 'Value' for test is incorrect, got 'Value>|Value' ERROR: InvalidReturnStatement - 43:17 - The inferred type 'Value>' does not match the declared return type 'Value|Value>' for testInvariantAlt ERROR: InvalidReturnStatement - 46:12 - The inferred type 'Value' does not match the declared return type 'Value|Value>' for testInvariantAlt ERROR: InvalidReturnType - 39:12 - The declared return type 'Value|Value>' for testInvariantAlt is incorrect, got 'Value>|Value' ERROR: InvalidReturnStatement - 54:16 - The inferred type 'Value>' does not match the declared return type 'Value' for testScalar ERROR: InvalidReturnType - 50:12 - The declared return type 'Value' for testScalar is incorrect, got 'Value<'Too big'>|Value>' ERROR: InvalidReturnStatement - 66:12 - The inferred type 'Value' does not match the declared return type 'Value' for testString ERROR: InvalidReturnType - 60:14 - The declared return type 'Value' for testString is incorrect, got 'Value' ```
AndrolGenhald commented 2 years ago

This is basically due to the lack of automatic upcasting of templates (see also #7675).

You can either add an extra function or force set the iterator type to work around the issue for now: https://psalm.dev/r/4090f9a7ad

Return types are one of the few places where automatic upcasting should be relatively easy to implement, so hopefully it's not too difficult if I get around to it sometime in the next month or two.

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

I found these snippets:

https://psalm.dev/r/4090f9a7ad ```php value; } } /** * @return Value */ function test(int $num): Value { if ($num > 0 && $num <= 100) { return new Value(getTestIterator($num)); } return new Value(getTestIterator(null)); } function getTestIterator(?int $num): Iterator { return $num === null ? new EmptyIterator() : new ArrayIterator(range(0, $num)); } /** * @return Value */ function test2(int $num): Value { if ($num > 0 && $num <= 100) { $iterator = new ArrayIterator(range(0, $num)); } else { $iterator = new EmptyIterator(); } /** @var Iterator $iterator */ return new Value($iterator); } ``` ``` Psalm output (using commit 569e97d): No issues! ```
gmazzap commented 2 years ago

Thank you @AndrolGenhald

Right now I'm using @var docboloc where I consume the value. Like:

/** @var Value<ArrayIterator> $it */
$it = test($num);

so I don't have to declare a function for this.

But the issue is bigger than that, especially when you add assertions into the mix.

If I have a template type Foo|Bar and I somehow assert the template type is Bar Psalm is going to complain if the value is not exactly Bar, but one the Bar sub-types.

In short, I can get around it, but if templates would get upcasting that will be a huge improvement, imo.

XedinUnknown commented 2 years ago

In my case, I made it think by using @var that I am actually returning a generic interface - just to make sure this was not the problem. Looks like it isn't: below please find my error, formatted for readability.

The declared return type
    Inpsyde\PayoneerSdk\Api\Command\Error\InteractionErrorInterface
        <
            E:Inpsyde\PayoneerSdk\Api\Command\Error\InteractionErrorFactory
            as Inpsyde\PayoneerSdk\Api\Command\Exception\InteractionExceptionInterface
        >
for
    Inpsyde\PayoneerSdk\Api\Command\Error\InteractionErrorFactory::createInteractionError
is incorrect, got
    Inpsyde\PayoneerSdk\Api\Command\Error\InteractionErrorInterface
        <
            Inpsyde\PayoneerSdk\Api\Command\Exception\InteractionExceptionInterface
        >

The expected and inferred types appear to be exactly the same. What gives? Seems like a trivial case, but not working.

I confirm that the InteractionErrorFactory (concrete implementation) declares it's own generic type and uses it to implement the interface with a generic type, like this, in order to use it for a ctor argument:

// InteractionErrorFactory
/**
 * @template E of InteractionExceptionInterface
 * @implements InteractionErrorFactoryInterface<E>
 */
// InteractionErrorFactory

/**
 * @template E of InteractionExceptionInterface
 */

I'm no expert in Psalm internals, but doesn't like an issue with template letter case to me.

AndrolGenhald commented 2 years ago

@XedinUnknown This is a separate issue, and I think it's actually a bug in your code. It's a bit hard to tell without the full source, but I'm guessing you have something like this. The problem is that this allows you to have the type FooFactory<Bar>, but it will still return Foo.

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

I found these snippets:

https://psalm.dev/r/9005cfd36f ```php */ class FooFactory implements FooFactoryInterface { public function create(): object { return new Foo(); } } ``` ``` Psalm output (using commit cab6f33): ERROR: InvalidReturnStatement - 23:16 - The inferred type 'Foo' does not match the declared return type 'T:FooFactory as Foo' for FooFactory::create ERROR: InvalidReturnType - 21:31 - The declared return type 'T:FooFactory as Foo' for FooFactory::create is incorrect, got 'Foo' ```
XedinUnknown commented 2 years ago

@AndrolGenhald, that looks like my issue! But what's wrong with the snippet you wrote? 🤔

AndrolGenhald commented 2 years ago

https://psalm.dev/r/d39f0af14f

Even if the code can only construct FooFactory<Foo>, FooFactory<Bar> is still a valid type. In this case you probably want to either remove the template and have FooFactory implement FooFactoryInterface<Foo> or create the template class somehow by using class-string<T> or closure(): T or something like that.

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

I found these snippets:

https://psalm.dev/r/d39f0af14f ```php */ class FooFactory implements FooFactoryInterface { public function create(): object { return new Foo(); } } /** @var FooFactory */ $barFactory = new FooFactory(); $bar = $barFactory->create(); /** @psalm-trace $bar */ // it's actually Foo! ``` ``` Psalm output (using commit cab6f33): INFO: Trace - 30:47 - $bar: Bar INFO: UnusedVariable - 29:1 - $bar is never referenced or the value is not used ERROR: InvalidReturnStatement - 23:16 - The inferred type 'Foo' does not match the declared return type 'T:FooFactory as Foo' for FooFactory::create ERROR: InvalidReturnType - 21:31 - The declared return type 'T:FooFactory as Foo' for FooFactory::create is incorrect, got 'Foo' ```
XedinUnknown commented 2 years ago

@AndrolGenhald, sorry, I didn't quite understand what you wrote.

AndrolGenhald commented 2 years ago
  • Yes, FooFactory<Bar> is a valid type. But you are not using it anywhere in your snippet. What of it?

If you use a template Psalm still checks to make sure your code is valid for all possible types of that template, regardless of whether they're used in your code, regardless of whether they can be used. If you have a template that can only ever be Foo it serves no purpose, remove it and use Foo instead.

  • Your latest snippet reports issues. I'm not sure how it fixed the problem. Psalm reports Bar there because this is what you told Psalm the factory is of. But my issue does not happen around instantiation or consumption of the factory: at this point it is not consumed anywhere yet.

It was meant to illustrate the issue, not fix it. Psalm will make sure your code is correct for all possible template types regardless of whether they are instantiated. If you were writing a library you wouldn't want a bug to slip by because you never instantiated a specific template.

  • Where would I use the templated closure or class-string?

https://psalm.dev/r/2cfc76eb6b https://psalm.dev/r/225c764d41

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

I found these snippets:

https://psalm.dev/r/ec0c4c6413 ```php */ class FooFactory implements FooFactoryInterface { public function create(): object { return new Foo(); } } ``` ``` Psalm output (using commit cab6f33): No issues! ```
https://psalm.dev/r/2cfc76eb6b ```php */ class FooFactory implements FooFactoryInterface { /** @var Closure(): T */ private $closure; /** @param Closure(): T $closure */ public function __construct($closure) { $this->closure = $closure; } public function create(): object { return ($this->closure)(); } } ``` ``` Psalm output (using commit cab6f33): No issues! ```
https://psalm.dev/r/225c764d41 ```php */ class FooFactory implements FooFactoryInterface { /** @var class-string */ private string $class; /** @param class-string $class */ public function __construct(string $class) { $this->class = $class; } public function create(): object { return new $this->class(); } } ``` ``` Psalm output (using commit cab6f33): No issues! ```
XedinUnknown commented 2 years ago

Thanks for the explanation, @AndrolGenhald! I hadn't quite understood, but @Biont figured it out.

In the method where I return the "erroneous" type, I add this var typehint with PHPDoc:

/** @var InteractionErrorInterface<E> */

I see why this solves the problem: this confirms that the returned value is correct for all possible permutations. Before that, the returned value was too narrow: it was saying that it has to return X<B>. and I was returning A of which B is a subtype.

But I don't understand why the problem is there in the first place: X<A> obeys X<B> where B of A. Seems like a bug to me. Especially since the problem still persisted after I told Psalm that the returned type is exactly what it says it is expecting.

gmazzap commented 2 years ago

But I don't understand why the problem is there in the first place: X<A> obeys X<B> where B of A. Seems like a bug to me.

@XedinUnknown this is what this issue is about. So far, in Psalm, templated types are "invariant". If you declare X<A> it expects exactly X<A> and not X<instance of A>.

And yes, this is labeled as a bug already by maintainers.

XedinUnknown commented 2 years ago

Great, thanks @gmazzap! I came to the right place then, and it's not a problem with my code. Good to know!

AndrolGenhald commented 2 years ago

But I don't understand why the problem is there in the first place: X<A> obeys X<B> where B of A. Seems like a bug to me. Especially since the problem still persisted after I told Psalm that the returned type is exactly what it says it is expecting.

That sounds backwards, you can't return X<Parent> where X<Child> is expected. If you mean you want to return X<Child> where X<Parent> is expected then yes, that is what this issue is about. X<Child> is not a subtype of X<Parent> unless the template is declared as covariant, so Psalm doesn't allow it.

Returning X<Child> instead of X<Parent> is safe but it requires Psalm to support automatic upcasting of the template, which hasn't been implemented yet.

XedinUnknown commented 2 years ago

@AndrolGenhald, if B of A, that means B includes A. So, yes, the problem seems to be the invariance, rather than the type hierarchy. But yes, I get that this is not yet supported, although I don't know enough to understand why, so you're probably right.

Great, I look forward to seeing a solution!

AndrolGenhald commented 2 years ago

Actually this is a bit more complicated than I was thinking, even for return types: https://3v4l.org/lbVCZ

An automatic upcast can only be allowed when there isn't some sort of type constraint. I thought maybe ensuring it's a local variable would work, but you can have stuff like this which is also illegal. Unfortunately this is difficult to check and probably won't happen any time soon.

weirdan commented 1 year ago

The correct way to annotate the code in initial snippet is to use @template-covariant: https://psalm.dev/r/52f99d05a1

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

I found these snippets:

https://psalm.dev/r/52f99d05a1 ```php value; } } /** * @return Value> */ function test(int $num): Value { if ($num > 0 && $num <= 100) { return new Value(new ArrayIterator(range(0, $num))); } return new Value(new EmptyIterator()); } ``` ``` Psalm output (using commit 8f39de9): No issues! ```
XedinUnknown commented 1 year ago

Will try @template-covariant next time. Does it support a superset of @template? If so, why would I need a separate tag for that, especially given that it seems to just add more support for generic types which I guess we already expect to work that way with @template. I haven't found much docs for it, aside from it appearing in an example for @psalm-yield.

orklah commented 1 year ago

Here's the article on that: https://psalm.dev/docs/annotating_code/templated_annotations/#template-covariance

It doesn't just add support, it adds some restriction on what you can do with your now covariant templated class so you need to be aware of what's going on

XedinUnknown commented 1 year ago

Worked for me here.

Now I tried it in another project, and it gives really confiusing errors like this:

ERROR: InvalidReturnType - modules/dummyjson/src/Transform/ProductHydrator.php:44:16 - The declared return type 'Out:DigitalSilk\DummyJson\Transform\ProductHydrator as mixed' for DigitalSilk\DummyJson\Transform\ProductHydrator::transform is incorrect, got 'DigitalSilk\DummyJson\Data\Product'

Here, I even tried to declare the generic type as mixed, and yet it doesn't understand that Product is a subtype of mixed (let alone of object, ProductInterface, or even Product). It's as if template covariance is altogether not working. This is Psalm 5.x-dev.

The only way I could get it to work is to declare the type of the returned value explicitly with @var. Here's an example that illustrates the problem. If you add /** @var Out */ on line 258, the issue goes away. Somehow, it is unable to infer that Product is a subtype of ProductInterface (or of mixed even), unless you explicitly tell it so.