vimeo / psalm

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

Override Psalm's template type inference #9838

Open janopae opened 1 year ago

janopae commented 1 year ago

Sometimes, you need to declare a type manually, as the inferred type is too specific:

/**
* @template Tkey of array-key
* @template T
*/
class ArrayCollection {
    /**
    * @param array<Tkey, T> $array
    */
    public function __construct(private array $array){}
}

$collection = new ArrayCollection(['a', 'b']);

/** @psalm-trace $collection: ArrayCollection<int<0, 1>, 'a'|'b'> */

This type would probably be pretty useless in most cases, because which function would take an ArrayCollection<int<0, 1>, 'a'|'b'> as a parameter? The problem is that you can't always pass a subtype into a supertype declaration (so you couldn't pass this variable into a function that takes an ArrayCollection<int, string>, for example).

You could do it like this:


/** @var ArrayCollection<array-key, string> $collection */
$collection = new ArrayCollection(['a', 'b']);

However, inline @var annoations are equivalents to @psalm-ignore and destroy the soundness of the type system, because they are completely unchecked. So in the future, the signature of the constructor (or better imagine a factory method instead) might change and return something else that is incompatible with the declared type, and nobody would notice.

I'm not aware of a way to provide a less specific type in such a case while still having psalm keeping track of it.

I'd propose either

  1. changing the way inline @var annotations work to being type-checked (huge breaking change)
  2. @cast annotations (as discussed in #7675) that lets you cast any type into a supertype at any time (introduces the problem that template params could be changed later-on – which could lead to Dog collections filled with Cats)
  3. add a new annotation like @template-params, that syntactically works like a type cast (so you could write doSomething(/** @template-params <string, int> */ new ArrayCollection()); and don't need a variable name) and overrides Psalms automatic template type inference ("template-params" because AFAIK templates are the only kind of type where you can't always pass a subtype into a supertype declaration and therefore need this)

A super-dooper advanced solution

The super-deluxe solution would be an equivalent to TypeScript's satisfies operator, which would allow us to use the more general type, but keep the inferred specific type in the back of psalm's mind in case it is needed (until the variable is modified under the assumption of the looser type). However, I don't think that this use case is needed that often. A simpler solution, which just allows declaring a looser type without losing type checking would be sufficient in most cases.

orklah commented 1 year ago

Yeah, there's probably space for improvement there. I also like what PHPStan did with @var where it restricts to subtypes (so it can only be used when the source of the type was not precise enough) and prevent completely changing the inferred type

Note that if your template contains a literal, Psalm won't check for covariance and will allow you to pass your template even when it's more specific than what is asked (so I believe the examples you made in your posts would actually work for the most part)

janopae commented 1 year ago

Thanks for your reply.

Note that if your template contains a literal, Psalm won't check for covariance and will allow you to pass your template even when it's more specific than what is asked (so I believe the examples you made in your posts would actually work for the most part)

I wish that was the case, but it seems like Psalm does check for covariance, at least in my case: https://psalm.dev/r/066942a733

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

I found these snippets:

https://psalm.dev/r/066942a733 ```php $array */ public function __construct(private array $array){} } /** * @param ArrayCollection $a */ function doSomething(ArrayCollection $a): void {} $collection = new ArrayCollection(['a', 'b']); doSomething($collection); ``` ``` Psalm output (using commit 75baaf7): INFO: UnusedParam - 17:38 - Param a is never referenced in this method ERROR: InvalidArgument - 22:13 - Argument 1 of doSomething expects ArrayCollection, but ArrayCollection, 'a'|'b'> provided ```
janopae commented 1 year ago

Btw., after thinking about this, I have a strong preference for a simple solution that only lets you override Psalm's inference for template types.

This is because @var annotations require a variable name. So you can't use it inline.

In my case, there is a repeating code pattern that goes something like this:

return new WrapperCollection(
    new ArrayCollection(
        ['something', 'else']
    );
);

I think I have to refactor all these places to this now:

/** @var ArrayCollection<int, string> $wrappedCollection */
$wrappedCollection = new ArrayCollection(
        ['something', 'else']
);

return new WrapperCollection($wrappedCollection);

but I really don't like it.

orklah commented 1 year ago

I wish that was the case, but it seems like Psalm does check for covariance, at least in my case: https://psalm.dev/r/066942a733

Seems to be working but not for keys: https://psalm.dev/r/8bb9e3bd81 That's probably a bug

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

I found these snippets:

https://psalm.dev/r/066942a733 ```php $array */ public function __construct(private array $array){} } /** * @param ArrayCollection $a */ function doSomething(ArrayCollection $a): void {} $collection = new ArrayCollection(['a', 'b']); doSomething($collection); ``` ``` Psalm output (using commit a82e7fc): INFO: UnusedParam - 17:38 - Param a is never referenced in this method ERROR: InvalidArgument - 22:13 - Argument 1 of doSomething expects ArrayCollection, but ArrayCollection, 'a'|'b'> provided ```
https://psalm.dev/r/8bb9e3bd81 ```php $array */ public function __construct(private array $array){} } /** * @param ArrayCollection $a */ function doSomething(ArrayCollection $a): void {} /** @var ArrayCollection */ $collection = new ArrayCollection(['a', 'b']); doSomething($collection); ``` ``` Psalm output (using commit a82e7fc): INFO: UnusedParam - 17:38 - Param a is never referenced in this method ```
klimick commented 1 year ago

That's probably a bug

Int ranges and tuples does not handle here: https://github.com/vimeo/psalm/blob/a82e7fc893b98a7990f46787a8b737c2a7ab1c2b/src/Psalm/Internal/Type/Comparator/GenericTypeComparator.php#L140-L144

https://psalm.dev/r/2b67d6c172 https://psalm.dev/r/ebe8581387 (I tried to solve it here https://github.com/vimeo/psalm/pull/9746, but stop due to lack information and time to disscussion.)

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

I found these snippets:

https://psalm.dev/r/2b67d6c172 ```php $array */ public function __construct(private array $array){} } /** * @param ArrayCollection $_ */ function doSomething(ArrayCollection $_): void {} $collection = new ArrayCollection([ rand(0, 15), ]); doSomething($collection); ``` ``` Psalm output (using commit a82e7fc): ERROR: InvalidArgument - 23:13 - Argument 1 of doSomething expects ArrayCollection, but ArrayCollection> provided ```
https://psalm.dev/r/ebe8581387 ```php $array */ public function __construct(private array $array){} } /** * @param ArrayCollection> $_ */ function doSomething(ArrayCollection $_): void {} $collection = new ArrayCollection([ [new \DateTimeImmutable] ]); doSomething($collection); ``` ``` Psalm output (using commit a82e7fc): ERROR: InvalidArgument - 23:13 - Argument 1 of doSomething expects ArrayCollection>, but ArrayCollection provided ```
klimick commented 1 year ago

Also, I was thinking about templates widening:

/**
 * @template-widen TKey
 * @template-widen TValue
 */
final class ArrayCollection
{
    /**
     * @param array<TKey, TValue> $values
     */
    public function __construct(
        public readonly array $values,
    ) {
    }
}

// $coll is ArrayCollection<int, string>, not ArrayCollection<int<0, 2>, 'fst'|'snd'|'thr'>
$coll = new ArrayCollection(['fst', 'snd', 'thr']);

How often have you needed literals inside an ArrayCollection? I think never) I tried to do @template-widen but got stuck on with following problem:

It is not clear how to behave with non-empty-string or non-empty-list/array. For example, why new ArrayCollection(['fst', 'snd', 'thr']) will be ArrayCollection<int, string> not ArrayCollection<int, non-empty-string>?

It was still possible to infer a non-empty-string, but it was tricky:

/**
 * @param list<non-empty-string> $strings
 * @return list<non-empty-string>
 */
function nonEmptyStrings(array $strings): array
{
    return $strings;
}

// will be ArrayCollection<non-empty-string>
$withNeStrings = new ArrayCollection(nonEmptyStrings(['fst', 'snd', 'thr']));

What if I want ArrayCollection<non-empty-list<non-empty-string>> or ArrayCollection<non-empty-list<string>> ? How infer it?

This is why I couldn't complete this feature. But I think it feature might be useful for you. What shorter and more expressiveness?

return new WrapperCollection(
    new ArrayCollection(['something', 'else']);
);

or

return new WrapperCollection(
    /** @psalm-templates <int, string> */
    new ArrayCollection(['something', 'else']);
);

Personally, I don't really like writing dockblocks. Especially when they are written in the middle of the code.

klimick commented 1 year ago

While I was writing all this, I realized that @template-widen and @psalm-templates can works in tandem:

// Will be inferred as ArrayCollection<int, non-empty-string>
$withNonEmptyStrings = /** @psalm-templates <int, non-empty-string> */
    new ArrayCollection(['fst', 'snd', 'thr']);

// Will be inferred as ArrayCollection<int, string>
$withJustStrings = new ArrayCollection(['fst', 'snd', 'thr']);

// Will be inferred as ArrayCollection<int, non-empty-string>
// but with error 'ArrayCollection<int, int> is not assignable to ArrayCollection<int, non-empty-string>'
$wrong = /** @psalm-templates <int, non-empty-string> */
    new ArrayCollection([1, 2, 3]);

Do you understand what I mean? Or have I confused you?

janopae commented 1 year ago

You meant what I proposed as @template-params when you wrote @psalm-templates, right?

Would @template-widen do anything else than what @orklah explained to be the desired default behaviour for template type inference (namely taking the super type in case of a scalar)? How wide would the templates be widened?

I'm a bit sceptical about the template-widen annotation. To me, it raises the following questions:

Is it really the task of the templated class to decide how template parameter types will be inferred, or shouldn't that be up to the caller? If templates types got inferred differently for different classes, wouldn't that be a possible source of confusion?

Would @template-widen on the templated class side even be necessary if we had @psalm-templates on the caller side?

Maybe it would be better to agree on one good, practical default way for Psalm to infer templates automatically on the creation of a value (this cold be a automatism that takes the parent type in case of a literal), and another way to manually specify any deviations from this default behaviour as explicitly as possible, while making sure that changing template types over the lifetime of an object is not possible.

klimick commented 1 year ago

You meant what I proposed as @template-params when you wrote @psalm-templates, right?

Yes. I've misspelled sorry.

Is it really the task of the templated class to decide how template parameter types will be inferred, or shouldn't that be up to the caller? If templates types got inferred differently for different classes, wouldn't that be a possible source of confusion?

In this case, I didn't come up with anything new:

All of these examples show the way how to infer literal or non-literal type parameter. It is a part of the type contract. Psalm infer literal types by default. So @template-widen is the way how to get less specific type parameter by default. WIthout any dockblocks. I can't remember any situation where literal type param of ArrayCollection`Option`\'Either'\'AnyAwesomeTemplatedType' can be useful. But non-literal types... It's used anywhere.

IF you faced with rare situation where literal type will be useful for class with @template-widen you can use @template-params:

$a = /** @template-params<int, 1|2|3> */new ArrayCollection([1, 2, 3]);

Would @template-widen on the templated class side even be necessary if we had @psalm-templates on the caller side?

It is unrelated features. But it can be used together (I did leave example here https://github.com/vimeo/psalm/issues/9838#issuecomment-1573361732) You may totally reject any idea of implementing @template-widen. I'm just sharing options that would allow us to write fewer dockblocks)

How wide would the templates be widened?

Hard to say now. This needs to be discussed separately. If quickly: literal int -> int literal bool -> bool literal float -> float literal string -> string (not non-empty-string) list{A, B, C} -> list<A|B|C> (not non-empty-list<A|B|C>) array{a: A, b: B, c: C} -> array<string, A|B|C> (not non-empty-array<non-empty-string, A|B|C>)

Would @template-widen do anything else than what @orklah explained to be the desired default behaviour for template type inference (namely taking the super type in case of a scalar)?

My least favorite feature that breaks the invariant) Imagine code a little bit harder and try to find where invariant was broken: https://psalm.dev/r/b6ef64e4d6

Psalm allows to specialize: literal int -> int literal bool -> bool literal float -> float literal string -> string (or non-empty-string)

int ranges does not supported and because you faced with your problem. You can add supports here But will it be the right decision? I don't know.

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

I found these snippets:

https://psalm.dev/r/b6ef64e4d6 ```php $_ */ function specialize(Foo $_): void { } $var = new Foo(42); /** @psalm-trace $var */; specialize($var); /** @psalm-trace $var */; ``` ``` Psalm output (using commit a82e7fc): INFO: Trace - 25:25 - $var: Foo<42> INFO: Trace - 27:25 - $var: Foo ```
janopae commented 1 year ago

In this case, I didn't come up with anything new

Oh, I didn't realise that this was already a thing in TypeScript, PHPStan and Scalar.

At least in PHPStan, you could argue that it was for the lack of a way to explicitly specify template parameters, but you couldn't argue this way for TypeScript and Scalar, so I guess there are real use cases for this that I didn't see.

If PHPStan already supports this annotation, then I think that Psalm should implement it too, for conistency's sake alone.

And yes, I agree that it would play nicely together with explicitly specifiyng template types.

My least favorite feature that breaks the invariant) Imagine code a little bit harder and try to find where invariant was broken: https://psalm.dev/r/b6ef64e4d6

Yes, I see that problem, too. I already thought about opening an issue to remove this feature as soon as there is a way to specify template types explicitly. However, the feature could also be removed if @template-widen was supported.

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

I found these snippets:

https://psalm.dev/r/b6ef64e4d6 ```php $_ */ function specialize(Foo $_): void { } $var = new Foo(42); /** @psalm-trace $var */; specialize($var); /** @psalm-trace $var */; ``` ``` Psalm output (using commit a82e7fc): INFO: Trace - 25:25 - $var: Foo<42> INFO: Trace - 27:25 - $var: Foo ```
klimick commented 1 year ago

Scalar

Scala :D

Will @template-params only affect the new expression?

janopae commented 1 year ago

Will @template-params only affect the new expression?

That was my idea, yes. However, in the mean time, I found #8066, which proposes the same idea, and lists some other expressions to be affected:

klimick commented 1 year ago

Not sure that I understand what Reflection means.

janopae commented 1 year ago

Not sure that I understand what Reflection means.

I think they mean that when you create an object using ReflectionClass::newInstance, you'd probably want to provide template parameters, too.

I just thought about this, and maybe this applies to all kinds of factory methods, or even more general, all pure functions where template types get inferred?

danog commented 8 months ago

Btw, I've opened https://github.com/vimeo/psalm/pull/10577, as the first step in my plan to fix this issue.

First of all, the default template types all SPL datastructures will be changed to never; then, a new @satisfies assertion will be introduced to avoid the (ab)use of @var.

The semantics of @satisfies will be entirely similar to typescript's satisfies operator, and will allow, among other things, typechecked call-site specification of template parameters.

Here's a few examples of how I generally plan @satifies to work:

I'm totally open to suggestions and feedback on this structure!

===

Explanation of the rationale for custom template defaults (btw, a custom default of never is already hardcoded for SplObjectStorage in the current codebase of Psalm):

Default template values

By default, if a class template parameter is not initialized in the constructor or by a @var annotation, Psalm will initialize it to mixed.

However, a custom default initial type can also by provided using the following syntax:

<?php

/**
 * Template T can have any type (`mixed`), but if it's not specified, it will default to `never`.
 * 
 * @template T as mixed = never
 */
class MyContainer {
  /** @var list<T> */
  private array $values = [];

  /** @param T $value */
  public function addValue($value): void {
    $this->values []= $value;
  }

  /** @return list<T> */
  public function getValue(): array {
    return $this->values;
  }
}

$t1 = new MyContainer;

/** @psalm-trace $t1 */; // MyContainer<never>

// InvalidArgument: Argument 1 of MyContainer::addValue expects never, but 123 provided
$t1->addValue(123); 

/** @satisfies MyContainer<int> Always specify template parameters! */
$t2 = new MyContainer;

/** @psalm-trace $t2 */; // MyContainer<never: int>

// OK!
$t2->addValue(123); 

/** @psalm-trace $t2 */; // MyContainer<123: int>

This can be often useful, like in the above example, to always force specification of a template type when constructing generic objects by specifying never as default type: never is the bottom type, and thus all usages of methods relying on an uninitialized template type will use never and will emit Psalm issues, essentially warning the user to explicitly specify a template parameter when constructing the class.

psalm-github-bot[bot] commented 8 months ago

I found these snippets:

https://psalm.dev/r/fcc7e48e65 ```php * * @param TNewValue $value * * @psalm-this-out self<(TValue|TNewValue): satisfied-by> */ public function enqueue($value): void {} } /** @satisfies FutureSplQueue */ $a = new FutureSplQueue; // Equivalent to /** @var FutureSplQueue */ $a = new FutureSplQueue; /** @psalm-trace $a */; // FutureSplQueue $a->enqueue(1); /** @psalm-trace $a */; // FutureSplQueue<123: int> $a->enqueue(2); /** @psalm-trace $a */; // FutureSplQueue<123|321: int> // InvalidArguments: Argument 1 of FutureSplQueue::enqueue expects int, but "str" provided $a->enqueue("str"); ``` ``` Psalm encountered an internal error: /vendor/vimeo/psalm/src/Psalm/Internal/Type/ParseTreeCreator.php: Unexpected LHS of property ```
https://psalm.dev/r/59402fff1a ```php
janopae commented 8 months ago

Great idea!

I still have to think a bit about the details, and I'll leave feedback in your PR.

$a->enqueue(1);

/** @psalm-trace $a */; // FutureSplQueue<123: int>

$a->enqueue(2);

/** @psalm-trace $a */; // FutureSplQueue<123|321: int>

I guess you meant either

$a->enqueue(123);

/** @psalm-trace $a */; // FutureSplQueue<123: int>

$a->enqueue(321);

/** @psalm-trace $a */; // FutureSplQueue<123|321: int>

or

$a->enqueue(1);

/** @psalm-trace $a */; // FutureSplQueue<1: int>

$a->enqueue(2);

/** @psalm-trace $a */; // FutureSplQueue<2: int>
danog commented 8 months ago

@janopae Yep that's a typo, I meant https://psalm.dev/r/327c186a44

psalm-github-bot[bot] commented 8 months ago

I found these snippets:

https://psalm.dev/r/327c186a44 ```php * * @param TNewValue $value * * @psalm-this-out self<(TValue|TNewValue): satisfied-by> */ public function enqueue($value): void {} } /** @satisfies FutureSplQueue */ $a = new FutureSplQueue; // Equivalent to /** @var FutureSplQueue */ $a = new FutureSplQueue; /** @psalm-trace $a */; // FutureSplQueue $a->enqueue(1); /** @psalm-trace $a */; // FutureSplQueue<1: int> $a->enqueue(2); /** @psalm-trace $a */; // FutureSplQueue<1|2: int> // InvalidArguments: Argument 1 of FutureSplQueue::enqueue expects int, but "str" provided $a->enqueue("str"); ``` ``` Psalm encountered an internal error: /vendor/vimeo/psalm/src/Psalm/Internal/Type/ParseTreeCreator.php: Unexpected LHS of property ```