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

Allow specifying a custom default template type/begin implementing @satisfies #10577

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

danog
Copy link
Collaborator

@danog danog commented Jan 19, 2024

This is the first step in my plan to fix #9838.

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.

Copy link

I found these snippets:

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

/**
 * @template TValue as mixed = never
 */
class FutureSplQueue {
    /**
     * @template TNewValue as satisfied-by<TValue>
     *
     * @param TNewValue $value
     *
     * @psalm-this-out self<(TValue|TNewValue): satisfied-by<TValue>>
     */
    public function enqueue($value): void {}
}

/** @satisfies FutureSplQueue<int> */
$a = new FutureSplQueue;

// Equivalent to

/** @var FutureSplQueue<never: int> */
$a = new FutureSplQueue;

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

$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

/** @satisfies int */
$a = 0;

// Equivalent to

/** @var (0: int) */
$a = 0;

// In a way, somewhat similar to the following in a possible future where PHP gets typed variables...

$a: int = 0;

/** @psalm-trace $a */; // 0: int

if (random_int(0, 1)) {
    $a = 1;
}

/** @psalm-trace $a */; // (0|1): int

// Some kind of InvalidAssignment...
$a = "str";
Psalm output (using commit a2140cc):

ERROR: ParseError - 13:3 - Syntax error, unexpected ':' on line 13

ERROR: InvalidDocblock - 9:1 - (0: int) is not a valid type (Saw : outside of object-like array in /var/www/vhosts/psalm.dev/httpdocs/src/somefile.php:8)

ERROR: UndefinedConstant - 13:5 - Const int is not defined

INFO: Trace - 15:23 - $a: 0

INFO: Trace - 21:23 - $a: 0|1

INFO: UnusedVariable - 4:1 - $a is never referenced or the value is not used

INFO: UnusedVariable - 9:1 - $a is never referenced or the value is not used

INFO: UnusedVariable - 18:5 - $a is never referenced or the value is not used

INFO: UnusedVariable - 24:1 - $a is never referenced or the value is not used

@danog danog requested review from weirdan, muglug and orklah January 19, 2024 12:28
@danog danog changed the title Allow specifying a custom default template type Allow specifying a custom default template type/begin implementing @satisfies Jan 19, 2024
@janopae
Copy link
Contributor

janopae commented Jan 19, 2024

I think this is a great effort that would handle the edge cases presented in #9838, along with generally proviting greater accuracy when dealing with values inside containers.

I think it is important that @satisfies only works on creation, and can't be added later-on. We then still have the open question whether it should work for factory methods, and under which conditions a function would qualify as "factory function" – see #9838 (comment).

@klimick
Copy link
Contributor

klimick commented Jan 24, 2024

I think it is important that @Satisfies only works on creation

Why it should be limited to the new expression?
Typescript allows this

@klimick
Copy link
Contributor

klimick commented Jan 24, 2024

I can't undestand how it solves this issue: https://psalm.dev/r/7da42be7d7

/** @satisfies MyContainer<int> */
$t = new MyContainer;

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

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

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

/**
 * @param MyContainer<int> $container
 */
function test(MyContainer $container): void {}
test($t);

/** @psalm-trace $t */; // What type will be here?

Copy link

I found these snippets:

https://psalm.dev/r/7da42be7d7
<?php

/**
 * @template T
 */
final readonly class MyCollection
{
}

/**
 * @param MyCollection<int> $_
 */
function test(MyCollection $_): void {}

/** @var MyCollection<1|2|3> */
$t = new MyCollection();

/** @psalm-trace $t */;
test($t);
/** @psalm-trace $t */;
Psalm output (using commit 3c90054):

INFO: Trace - 18:23 - $t: MyCollection<1|2|3>

INFO: Trace - 20:23 - $t: MyCollection<int>

@janopae
Copy link
Contributor

janopae commented Jan 25, 2024

Why it should be limited to the new expression?
Typescript allows this

As I wrote in my comment, I don't think it should be limited to the new expression, but also allow for factory functions (like the one in your TypeScript example).

But you shouldn't be able to do this.

When using satisfies, the instance must not have been saved somewhere else before, where the statisfies widening causes problems.

Edit: Instead of just asking questions and pointing out requirements, I thought I'd spend some time creating a proposal to fulfill these requirements:

Proposal

The satisfies operator should only work either with the new statement, or with functions that are 1. mutation-free and 2. factory functions.

Why mutation-free?

To ensure a function doesn't save a reference to the object that satisfies gets applied to into an identifier that doesn't know about the satisfies requirement, knowing that the function is mutation-free should be enough. This might also disable some legitamate use cases (factory functions that use mutation for something else than saving a reference of the returned object), but it is something that Psalm already provides, and should be rather easy to implement.

Factory functions?

There is another case however that we have to consider: What if the value returned by the function is the same as one that has been provided as a parameter? In this case, it is not really a factory function, but a function that "converts" the value somehow. The satisfied requirement might already been broken before the function has been called in that case. You could write code like this:

/**
* @var array<int,string>
*/
$a = ['test];

/**
* @var array<int,string>
*/
$referenceToA = $a;

function identity(array $a) {
    return $a;
}

/**
* @satisfies array<array-key, string> $a
*/
$a = identity($a);
$a['lol'] = 'lel';

// I can calculate the sum of all array keys of $referenceToA, since they're all ints, right?

array_sum(array_keys($referenceToA));

// right? o.O

I don't think Psalm already has the capabilities to ensure a return value does not contain a reference to any of the parameters. Maybe we need some sort of factoryFunction flag for functions?

@kkmuffme
Copy link
Contributor

kkmuffme commented Feb 1, 2024

Great idea!

  1. From what I read above it's not clear whether this works for regular function/method calls too? (since that is where it's most widely used in TS and it would be quite confusing if it wouldn't in psalm)

  2. Since @Satisfies basically an assertion, do you reuse the existing assertion code to automatically get those errors reported? e.g. to e.g. report redundant @Satisfies comments:

/** @satisfies int<0, 100> */
$a = rand(0, 100);
  1. in TS this would report an error since the type is int|false - do you report an error here or is this allowed?
/** @satisfies int */
$a = file_put_contents(...);

@danog
Copy link
Collaborator Author

danog commented Feb 2, 2024

@kkmuffme

  1. Sure
  2. Sure, that can be done.
  3. This is an error like in 2, @satisfies is not @var.

@weirdan
Copy link
Collaborator

weirdan commented Feb 4, 2024

@template T as mixed = whatever

The choice of the syntax is backward incompatible as previously = whatever would be treated as a part of the comment. To be on the safe side we must choose a syntax that is currently rejected by Psalm, e.g. @template T as mixed : whatever.

@edsrzf
Copy link
Contributor

edsrzf commented Apr 9, 2024

If I understand correctly, there are two or three things happening in this change:

  • A way to specify a default template parameter -- this is the backward incompatible change.
  • A safe way to specify template parameters.
  • A way to make a variable's type "sticky", so it can't change, more like other languages. (This is the part I'm not sure I'm correct about. I'm taking this from the second snippet link in the PR description.)

Given that only the first thing is backward incompatible, maybe we could focus efforts on just this part, in order to make progress toward Psalm 6.0? This by itself still seems valuable, and the rest can be done later without breaking compatibility.

A couple other thoughts on the overall idea:

This seems a little different from TypeScript's satisfies. In TypeScript, satisfies is purely an assertion and never changes the type of a variable. But here it sounds like we're wanting to change the variable's type?

I wonder if it would be worth further separating the second and third concepts above? It's not obvious to me that safely specifying template parameters should also cause the variable's type to be unchangeable.

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

Successfully merging this pull request may close these issues.

6 participants