Type projections by jiripudil · Pull Request #138 · phpstan/phpdoc-parser

jiripudil · 2022-06-20T08:41:22Z

This PR adds support for parsing type projections (phpstan/phpstan#3290) which I'd like to implement in PHPStan. Type projections are a way of declaring the variance of a type parameter at the site of use rather than at the site of its declaration (via @template-covariant). This allows you to have the type parameter in an incompatible position in the generic class, and only prevents you from using it where the projection is used.

Type projection can be created in the type argument position of a generic type by using either:

a variance keyword ('covariant' or 'contravariant') followed by the projected type, which denotes an out- or in-projection, respectively,
an asterisk, which denotes a star projection, or

/**
 * @param Box<covariant Animal> $covariant
 * @param Box<contravariant Cat> $contravariant
 * @param Box<*> $star
 */

The choice of the variance keywords favours consistency: while Kotlin uses in and out which might arguably be more intent-revealing, PHPStan already uses @template-covariant in a similar position, so I say let's stick with it.

One thing I'm not sure about is whether Foo<covariant> should throw a parse error (as currently implemented), or whether it should create an IdentifierTypeNode('covariant'). The former is potentially a BC break if somebody has a class named covariant in their code, and however unlikely it seems, you can always break somebody's build 🤔

staabm · 2022-06-20T08:46:02Z

doc/grammars/type.abnf

+	= TokenAngleBracketOpen GenericTypeArgument *(TokenComma GenericTypeArgument) TokenAngleBracketClose
+
+GenericTypeArgument
+    = [TokenContravariant / TokenCovariant] Type


nit: tabs vs. spaces

But which one is correct? It's pretty inconsistent across this whole file 😃

ondrejmirtes · 2022-06-20T08:47:24Z

/cc @VincentLanglet @hrach who were interested in this in the past.

Also /cc @rvanvelzen @JanTvrdik for an opinion on the syntax :)

JanTvrdik · 2022-06-20T08:49:33Z

I would much more prefer the in / out syntax used by Kotlin and TypeScript. It's both shorter and easier to understand.

ondrejmirtes · 2022-06-20T08:49:49Z

The TL; DR is that @param Collection<covariant Animal> will allow you to pass Collection<Dog> there even in case of @template T (and not @template-covariant). But you won't be able to call methods that have T in the parameter position.

More info about current behaviour here: https://phpstan.org/blog/whats-up-with-template-covariant

Also /cc @arnaud-lb for an opinion :)

ondrejmirtes · 2022-06-20T08:50:13Z

I don't understand the in/out syntax, I don't know which one is which. Any mnemonic help for that?

hrach · 2022-06-20T08:52:20Z

My mind-dump (aka explanation) here https://hrach.dev/posts/variance/ - star projection is syntax sugar for covariant use-site type projection.

JanTvrdik · 2022-06-20T08:56:30Z

Any mnemonic help for that?

in is for parameters (inputs, writing "into" collection) and out is for return types (outputs, reading "out of" collection).

hrach · 2022-06-20T09:00:48Z

src/Ast/Type/TypeProjectionNode.php

+	public $type;
+
+	/** @var 'covariant'|'contravariant' */
+	public $variance;


Shouldn't be here also invariant variance and be the default?

And maybe use class string constants instead of literal strings so it can be nicely checked in user code :)

Correct me if I'm wrong but I thought invariance does not create a type projection? Currently, the parser simply parses the naked type if there is no covariant/contravariant (or in/out) and it doesn't get any special treatment further on.

I think if we included invariant here, this class would have to become TypeArgumentNode which is either invariant (=> naked type), or contravariant or covariant (=> type projection). But then what about star projection? It is only an idiomatic shortcut for out mixed / in never and could be resolved to either one, but I can see the value in distinguishing it explicitly.

What about doing a similar thing I suggested in phpstan-src and make the variance simply an additional property of GenericTypeNode instead?

@jiripudil sorry, you're right, I missed the 359's early return. Anyway, wouldn't you rather consider something like GenericTypeParameterNode($type, $variance) (and ~ GenericStarParameterNode) -- i.e. the parser describes what it parses, not what is the semantic.

(but that's BC break :/ if done through composition)

What about doing a similar thing I suggested in phpstan-src and make the variance simply an additional property of GenericTypeNode instead?

I haven't yet had time to investigate this direction properly. I guess it could work. I'm still unsure about how to approach the star projection, though. It is equivalent to covariant mixed and I believe that's how people usually think of it, but IMO it deserves an explicit representation, even if only to preserve the appearance in code in the error output.

VincentLanglet · 2022-06-20T09:13:59Z

/cc @VincentLanglet @hrach who were interested in this in the past.

The TL; DR is that @param Collection<covariant Animal> will allow you to pass Collection<Dog> there even in case of @template T (and not @template-covariant). But you won't be able to call methods that have T in the parameter position.

More info about current behaviour here: https://phpstan.org/blog/whats-up-with-template-covariant

Currently:

https://phpstan.org/r/d496e8d3-2aac-4a56-b88b-af37a9332bd1 reports an error since the template is not declared
https://phpstan.org/r/8ce15174-c2c5-46d5-9bec-36d1b44707d8 reports an error because of the covariance

I'm still kinda interested in this feature ; but so far I found an hacky solution:

https://phpstan.org/r/a859d377-52ba-46a1-93f0-914469d35607
It's maybe stronger than the solution proposed by this PR since
https://phpstan.org/r/30b20739-5d6b-4deb-913a-b25b10592645 is reported as an error
but https://phpstan.org/r/6e829e83-21bb-4bfe-a310-aba9862ed3b5 is fully working (and I'm not sure this PR will support this)
And it works with psalm https://psalm.dev/r/c566a5d99a

I like the syntax

@param Box<*>

Since it basically says "I accept any Box" but I just add <*> to avoid a Phpstan level 6 error.

jiripudil · 2022-06-20T09:46:01Z

I am in favor of in/out too, but above all, I'd prefer the language to be consistent, so if we go that way, I think we should also support @template-out (and eventually @template-in). WDYT?

VincentLanglet · 2022-06-20T10:20:07Z

Might be worth to ping psalm maintainer @orklah to check if this feature can also be handled by psalm.

hrach · 2022-06-20T10:34:49Z

I think we should also support @template-out (and eventually @template-in). WDYT?

👍

vhenzl · 2022-06-20T10:41:21Z

I don't understand the in/out syntax, I don't know which one is which. Any mnemonic help for that?

TypeScript has a good explanation: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#optional-variance-annotations-for-type-parameters

in is for setters with contravariant parameters, out for getters with a covariant return type.

JanTvrdik · 2022-06-20T11:45:29Z

src/Ast/Type/TypeProjectionNode.php

+
+use PHPStan\PhpDocParser\Ast\NodeAttributes;
+
+class TypeProjectionNode implements TypeNode


Ideally TypeProjectionNode should not implement TypeNode, because it is not a type and cannot be used in places where type is expected. It's only allowed in place of generic argument.

It is roughly equivalent to CallableTypeParameterNode which implements just Node instead of TypeNode.

I made a suggestion here: #138 (comment)

ondrejmirtes · 2022-06-20T14:47:10Z

What's most important to me is consistency. It's easy to explain "To solve this problem, you either need to write covariant at declaration or at call-site". It's much harder to grasp "you need to write covariant at declaration or in at call-site".

Sure, one day we can move on and use in/out instead everywhere but we'd have to do that in sync with Psalm maintainers and PhpStorm developers at minimum.

VincentLanglet · 2022-06-20T14:53:29Z

Does the following example will be valid:

	/**
	 * @param Collection<covariant mixed> $collection
	 */
	public function compute(Collection $collection): int
	{
		$item = $collection->get();
		if (null !== $item) {
		    $collection->add($item);
		}
		
		return $collection->count();
	}

?
or it will only solve

	/**
	 * @param Collection<covariant mixed> $collection
	 */
	public function compute(Collection $collection): int
	{
		return $collection->count();
	}

?
for the example https://phpstan.org/r/6e829e83-21bb-4bfe-a310-aba9862ed3b5

ondrejmirtes · 2022-06-20T14:57:34Z

You can't call $collection->add($item) on Collection<covariant mixed>.

VincentLanglet · 2022-06-20T15:02:55Z

You can't call $collection->add($item) on Collection<covariant mixed>.

ok, so it will be less permissive than my hacky solution

	/**
	 * @template T
	 * @param Collection<T> $collection
	 */
	public function compute(Collection $collection): int
	{
		$item = $collection->get();
		if (null !== $item) {
		    $collection->add($item);
		}
		
		return $collection->count();
	}

but at least it will not be an hacky solution and solve lot of usecase.

Updating the blog article about generics will be useful IMHO for people like me which are not a lot familiar with covariance/contravariance. I currently don't know what will be the use case for Collection<contravariant mixed>

ondrejmirtes · 2022-06-20T15:04:53Z

I currently don't know what will be the use case for Collection<contravariant mixed>

I don't either, there's no @template-contravariant yet...

arnaud-lb · 2022-06-20T15:37:37Z

I have a preference for out/in as it's easier to remember than covariant/contravariant and also shorter to type/read. But since we already use covariant/contravariant, that it's the way to go in my opinion.

One thing I'm not sure about is whether Foo<covariant> should throw a parse error (as currently implemented), or whether it should create an IdentifierTypeNode('covariant'). The former is potentially a BC break if somebody has a class named covariant in their code, and however unlikely it seems, you can always break somebody's build thinking

Are there any downsides for for the latter ?

jiripudil · 2022-06-20T16:06:10Z

ok, so it will be less permissive than my hacky solution

I don't think making the function itself generic is hacky, actually. See https://hrach.dev/posts/variance/ linked above, there's a section named 'Breaking the Contract' at the bottom with code very similar to yours which solves the problem exactly that way.

I don't either, there's no @template-contravariant yet...

Honestly, neither do I, all the examples you can find on the Internet look very artificial to me. Maybe we could limit type projections to covariance, for starters? Some of the logic might need to be implemented anyway because star projections somewhat act in both ways, but we could only support covariant projections on the parser level and then introduce contravariant projections together with @template-contravariant when someone with an actual use case requests them.

Are there any downsides for for the latter ?

Well, I think for 99.99 % of users, Foo<covariant> is by mistake, and parse error means failing early. But it fails anyway, and with a pretty informative error message too. So no, I don't see any downsides.

ondrejmirtes · 2022-11-27T13:45:39Z

@jiripudil I just watched your talk on generics on YouTube, and I’m really looking forward to this! I just checked the code again, I think there shouldn’t be class TypeProjectionNode implements TypeNode. We need to move this onto GenericTypeNode, and in some backward-compatible manner, like an extra property called typeVariances.

Thank you!

staabm · 2022-11-27T14:38:45Z

Is this said talk public? :-)

mabar · 2022-11-27T14:39:52Z

@staabm It is, but in Czech https://www.youtube.com/watch?v=-tOCEtrQPww

jiripudil · 2022-11-27T16:31:25Z

I just watched your talk on generics on YouTube, and I’m really looking forward to this! I just checked the code again, I think there shouldn’t be class TypeProjectionNode implements TypeNode. We need to move this onto GenericTypeNode, and in some backward-compatible manner, like an extra property called typeVariances.

Thanks! Yes, I've had that in the works for some time :) I've rebased this onto 1.9.x and changed the implementation to populate GenericTypeNode::$variances.

I think I've also managed to figure out that damned star projection – it's effectively a bivariant placeholder, so I'm treating it as such. I'm really happy about this because in phpstan-src, we'll be able to represent it the same way as here – add support for this new case of TemplateTypeVariance, and simply keep a list of use-site variances in GenericObjectType, as you've suggested :)

…n BC

ondrejmirtes

Otherwise I'm ready to merge this and tag a new minor release 😊

ondrejmirtes · 2022-11-27T17:26:05Z

src/Ast/Type/GenericTypeNode.php

+		$genericTypes = [];
+
+		foreach ($this->genericTypes as $index => $type) {
+			$variance = $this->variances[$index];


This offset might not exist.

ondrejmirtes · 2022-11-27T17:28:31Z

src/Parser/TypeParser.php

+		} elseif ($tokens->tryConsumeTokenValue('covariant')) {
+			$variance = Ast\Type\GenericTypeNode::VARIANCE_COVARIANT;
+		} else {
+			$variance = Ast\Type\GenericTypeNode::VARIANCE_INVARIANT;


Some unsupported covariance name like bullshitriant isn't gonna cause a parse error and will silently fall back to invariant, right? Maybe we should address that or at least test it.

It falls back to invariant and proceeds to parse the value as a type. Thus,

Foo<bullshitriant> creates a generic type with IdentifierTypeNode('bullshitriant') in it (supposedly failing later due to the unknown class). That seems correct to me, I don't think there's a reliable way to distinguish between a typo and a type on the parser level.

Foo<bullshitriant Type> throws because 'bullshitriant' is parsed as an identifier and the parser expects a comma or a closing angle bracket to follow. I'll add this case to the test.

ondrejmirtes · 2022-11-27T19:13:05Z

Thank you very much!

BTW I can imagine at least 3-4 PRs in phpstan-src about this:

@template-contravariant for completeness sake
Basic support for this new syntax in the typesystemby reading the new node property, putting it into GenericObjectType etc.
Doing all the stuff about limiting what method can be called based on call-site variance, this is gonna be some new logic in GenericObjectType
Checking that the usage of type projection is sane and safe, checking declaration site variance against call-site variance etc. Checking that call-site variance isn't used in a bad place like @implements etc.

staabm reviewed Jun 20, 2022

View reviewed changes

hrach reviewed Jun 20, 2022

View reviewed changes

JanTvrdik reviewed Jun 20, 2022

View reviewed changes

ondrejmirtes mentioned this pull request Aug 17, 2022

Lower bounds support phpstan/phpstan#5179

Open

jiripudil added 3 commits November 22, 2022 15:43

parse type projections

6e37881

parse type projections into GenericTypeNode::$variances

7eb0dd5

distinguish star projection as a bivariant type argument

ace9e07

ondrejmirtes force-pushed the type-projections branch from 8b988e6 to 32417d9 Compare November 27, 2022 13:42

jiripudil force-pushed the type-projections branch from 32417d9 to ace9e07 Compare November 27, 2022 16:21

jiripudil changed the base branch from 1.8.x to 1.9.x November 27, 2022 16:21

make GenericTypeNode::$variances default to an empty array to maintai…

dc25aa4

…n BC

ondrejmirtes requested changes Nov 27, 2022

View reviewed changes

jiripudil added 2 commits November 27, 2022 18:50

fix potential undefined array offset

a3f6f04

add test for parse error due to a typo in the variance keyword

1495f4c

jiripudil requested a review from ondrejmirtes November 27, 2022 18:11

ondrejmirtes merged commit df1a794 into phpstan:1.9.x Nov 27, 2022

jiripudil deleted the type-projections branch November 27, 2022 19:12

jiripudil mentioned this pull request Jun 19, 2023

Type projections, part 1: call-site variance in GenericObjectType phpstan/phpstan-src#2471

Merged

mvorisek mentioned this pull request Jun 22, 2023

function (Model $x) {} passed to Closure(object): void is not reported in level <=6 phpstan/phpstan#7342

Open


		use PHPStan\PhpDocParser\Ast\NodeAttributes;

		class TypeProjectionNode implements TypeNode

Conversation

jiripudil commented Jun 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Jun 20, 2022

Uh oh!

JanTvrdik commented Jun 20, 2022

Uh oh!

ondrejmirtes commented Jun 20, 2022

Uh oh!

ondrejmirtes commented Jun 20, 2022

Uh oh!

hrach commented Jun 20, 2022

Uh oh!

JanTvrdik commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrach Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VincentLanglet commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiripudil commented Jun 20, 2022

Uh oh!

VincentLanglet commented Jun 20, 2022

Uh oh!

hrach commented Jun 20, 2022

Uh oh!

vhenzl commented Jun 20, 2022

Uh oh!

JanTvrdik Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Jun 20, 2022

Uh oh!

VincentLanglet commented Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Jun 20, 2022

Uh oh!

VincentLanglet commented Jun 20, 2022

Uh oh!

ondrejmirtes commented Jun 20, 2022

Uh oh!

arnaud-lb commented Jun 20, 2022

Uh oh!

jiripudil commented Jun 20, 2022

Uh oh!

ondrejmirtes commented Nov 27, 2022

Uh oh!

staabm commented Nov 27, 2022

Uh oh!

mabar commented Nov 27, 2022

Uh oh!

jiripudil commented Nov 27, 2022

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

JanTvrdik commented Jun 20, 2022 •

edited

Loading

hrach Jun 20, 2022 •

edited

Loading

VincentLanglet commented Jun 20, 2022 •

edited

Loading

JanTvrdik Jun 20, 2022 •

edited

Loading

VincentLanglet commented Jun 20, 2022 •

edited

Loading

jiripudil Nov 27, 2022 •

edited

Loading