Make intersection types nullable #7259

nicolas-grekas · 2021-07-19T14:18:08Z

Putting my OSS time where my mouth is :)

RFC at https://wiki.php.net/rfc/nullable_intersection_types

PHP 8.1 intersection types should be nullable IMHO. They're going to be much less interesting otherwise.

See https://externals.io/message/115479 for background and discussion.

~~This reuses the existing ? prefix to make intersection types compatible with null as argument, property, and return value.~~ Updated to use X&Y|null as I feel like that's going to gather a broader consensus.

This allows writing:

class Foo
    public X&Y|null $bar;


    function setBar(X&Y|null $bar = null): X&Y|null
    {
		return $this->bar = $bar;
	}
}

~~The ? syntax is choosen because it is the least surprising one and because the code already handles it (eg when computing error messages, see tests.)~~

On the implementation side, the nullable type is handled separately from union types. That's why the patch is trivial.

Please send me more test cases if you have weird situation in mind, I'd happily add them here.

trowski · 2021-07-19T14:41:12Z

The syntax should be either ?(X&Y) or (X&Y)|null (and IMO preferably only the latter) for readability and best forward-compatibility.

nicolas-grekas · 2021-07-19T14:47:22Z

The syntax should be either ?(X&Y)

?X&Y cannot be confused with (?X)&Y if that's what you mean, because (?X)&Y trivially means the same as X&Y. The brackets are thus not needed.

or (X&Y)|null (and IMO preferably only the latter) for readability and best forward-compatibility.

This syntax does not exist, 8.1 should not introduce it I believe. What exists is nullable types with a ? prefix. This builds on it. This is both easy to read (and people are used to it) and forward-compatible, as it doesn't collide with any of the discussed possible syntax for more complex type expressions.

We do need nullable intersection types now, not in the future.

trowski · 2021-07-19T14:58:57Z

It's not that ?X&Y is technically a problem, it's that ? was not allowed with union types due to readability concerns, so allowing it now with intersection types is not something we can quickly do before 8.1. I think the only syntax that could be allowed without an RFC would be (X&Y)|null (and I'm not sure about that, but it is the obvious and certainly forward-compatible syntax).

There are also LSP concerns with allowing null on an intersection type. I don't know what needs to be handled there as I have not given it much thought, but any implementation needs to consider and handle those cases.

nicolas-grekas · 2021-07-19T15:04:24Z

It's not that ?X&Y is technically a problem, it's that ? was not allowed with union types due to readability concerns, so allowing it now with intersection types is not something we can quickly do before 8.1. I think the only syntax that could be allowed without an RFC would be (X&Y)|null.

I don't know why you think so, because to me, ?X&Y is the closest to what we already have, while (X&Y)|null builds on a proposal that has no technical root right now. Nobody knows if complex types are going to make it ever in the future. As such, ?X&Y is the only possible and realistic choice in the immediate future.

There are also LSP concerns with allowing null on an intersection type.

This is handled AFAIK. Please check the test cases and let me know if you have more cases in mind.

Zend/tests/type_declarations/intersection_types/implicit_nullable_intersection_type.phpt

sgolemon · 2021-07-19T16:23:28Z

Zend/zend_language_parser.y

@@ -798,6 +798,7 @@ type_expr:
 	|	'?' type			{ $$ = $2; $$->attr |= ZEND_TYPE_NULLABLE; }
 	|	union_type			{ $$ = $1; }
 	|	intersection_type	{ $$ = $1; }
+	|	'?' intersection_type	{ $$ = $2; $$->attr |= ZEND_TYPE_NULLABLE; }


Regarding the nullability syntax. I find ?X&Y, while technically unambiguous due to null&Y not making sense, just isn't great for readability.

Further, we decided against ?X|Y in the union types RFC, and I think that decision should hold (even though the reasons are somewhat different.

I've got an implementation for (X&Y)|null, which is my preferred syntax at: https://gist.github.com/sgolemon/660b105e6894a6341792a479cda8f76b

However, we are much too late for 8.1 at this point and syntax is too important to rush, so this is going to have to wait for 8.2 no matter what we do.

Regarding the nullability syntax. I find ?X&Y, while technically unambiguous due to null&Y not making sense, just isn't great for readability.

Further, we decided against ?X|Y in the union types RFC, and I think that decision should hold (even though the reasons are somewhat different.

Agreed.

I've got an implementation for (X&Y)|null

I'm not opposed to allowing parenthesis, but requiring them seems unnecessary - the precedence of &/| and &&/|| is known in other contexts in php (operator precedence).

If (A&B)|null was allowed, there'd be the question of whether (A)|null should be allowed, (A)|(null), etc. It seems like it should (IF allowed), though the syntax seems less readable

However, we are much too late for 8.1 at this point and syntax is too important to rush, so this is going to have to wait for 8.2 no matter what we do.

Agreed.

I don't think you can use ?X|Y being disallowed to conclude that everyone would also be against the special case of ?X&Y. At the time in the Github discussion I argued against ?(X|Y), because it added parenthesis to not be ambiguous which would otherwise not be needed, and X|Y|null was available as an alternative. In that case having a more complicated syntax only to somehow include ? would have been counterproductive, in my opinion.

Yet I very much see the value of ?X&Y - it enables optional parameters/properties, and it does not contain parenthesis. If combined union type and intersection type syntax ever makes it into PHP, it would be easy to not allow ? there, just as ? is not allowed for union types. So the syntax seems forward-compatible and geared towards a common use case in PHP.

If (A&B)|null was allowed, there'd be the question of whether (A)|null should be allowed, (A)|(null), etc. It seems like it should (IF allowed), though the syntax seems less readable

That's a fair complaint. For me it comes down to finding a balance between providing the functionality that's being asked for quite strongly, and not over-delivering syntax that hasn't been entirely thought through. That's why my version of this patch only allows null on the right hand side, and requires parens around the intersection. It's the MOST restrictive syntax, and thereby the least likely to cause issues after we've spent more time thinking about it.

Ocramius · 2021-07-19T16:49:32Z

while (X&Y)|null builds on a proposal that has no technical root right now.

Which is why the RFC for intersection types is called "pure intersection types", and solves only that problem (intersection).

Here, we are introducing partial union + intersection semantics around null, which:

requires a new RFC
should really require explicit parentheses, as you are assuming precedence in type definition, which isn't a given.

In addition to that, @trowski makes it clear that readability is also messy. For instance, I would expect:

?A&B => means (?A)&B => reduced to A&B
A&?B => means A&(?B) => reduced to A&B too

In both cases, the ? would be elided, leading to confusion. This is according to my naiive assumption - aware this patch uses a different precedence for ?. Instead, having a clear (A&B)|null is much simpler and leaves no doubt.

It's not a big deal to, for now:

hold yer horses
@param or @var docblock until you have well defined union semantics (for which you are welcome to start an RFC). We've done it for years, and it can wait +1 year, when/if the DSL is voted upon (can be copied from phpdocumentor/phpstan/psalm).

trowski · 2021-07-19T17:44:20Z

This is handled AFAIK. Please check the test cases and let me know if you have more cases in mind.

Ok, when I glanced at the diff I expected to see more C code changes to handle LSP, but it seems the existing code handles it nicely.

However, we are much too late for 8.1 at this point and syntax is too important to rush, so this is going to have to wait for 8.2 no matter what we do.

Agreed. Any syntax choice here is going to have repercussions, so we can't be hasty.

sgolemon · 2021-07-19T18:26:03Z

It's not a big deal to, for now:

hold yer horses

@param or @var docblock until you have well defined union semantics (for which you are welcome to start an RFC). We've done it for years, and it can wait +1 year, when/if the DSL is voted upon (can be copied from phpdocumentor/phpstan/psalm).

☝️☝️☝️ This ☝️☝️☝️

alcaeus · 2021-07-20T04:07:30Z

It's not a big deal to, for now:

hold yer horses

I'll mention that using scalar types in PHP 7 was less than optimal since they weren't nullable. For most of the code I maintain (both open source and commercial) it didn't make sense to bother with typing in php 7.0, instead we waited for nullable types in 7.1.

I fear this case will be similar, especially since making the parameter nullable later will cause a bc break, while the change itself does not cause bc breaks.

If we're all aware of this and decide that limited use cases and thus limited feedback is what we're going for in favour of consistent nullability across types, there's no problem. Otherwise I would bring up the point of moving intersection types to 8.2 with nullability. This would require significant discussion, which is why I'm only doing an informal check of opinion instead of asking on the mailing list.

nicolas-grekas · 2021-07-20T08:03:24Z

Thank you so much @sgolemon for that patch you provided in #7259 (comment)

While I personally think ?X&Y is the most appropriate, I feel like using |null would gather a broader consensus. The attached patch now contains a second commit that changes the syntax to X&Y|null (PR description also updated.) Because operator precedence is known, the brackets are not needed and I'd prefer skipping them. Type expressions are already long enough.

we are much too late for 8.1 at this point and syntax is too important to rush, so this is going to have to wait for 8.2 no matter what we do.

I would kindly ask you to reconsider this. As @alcaeus just mentioned, PHP 7.0 was blatantly missing nullable types. Let's learn from history and recognize that having intersection types not nullable right away makes them way less interesting/useful. This feels like a bug from a feature completeness pov. ExtractExactly what the feature freeze period is for. Alternatively, we could move intersection types to 8.2, but that'd be a shame given that we're so close to having them complete.

(( Apologies, I didn't mean to edit your comment, I meant to quote it and I evidently don't know how to github -- @sgolemon ))

krakjoe · 2021-07-20T12:00:50Z

There doesn't really look like there is consensus on this, and tagging happens this afternoon.

Since no consensus can be built in half a day, it looks like intersections will have to be deployed as they have been voted on while this is resolved.

faizanakram99 · 2021-07-20T15:45:50Z

I'll mention that using scalar types in PHP 7 was less than optimal since they weren't nullable. For most of the code I maintain (both open source and commercial) it didn't make sense to bother with typing in php 7.0, instead we waited for nullable types in 7.1.

I fear this case will be similar, especially since making the parameter nullable later will cause a bc break, while the change itself does not cause bc breaks.

Adding default null value ( = null) added nullability implicitly in 7.0 and is still quite common.

https://3v4l.org/SNctX#v7.0.0

BC breaks were avoided back then by using default null value (and later prefixing types with ? in 7.1).

guilliamxavier · 2021-07-20T16:57:18Z

Adding default null value ( = null) added nullability implicitly in 7.0 and is still quite common.

https://3v4l.org/SNctX#v7.0.0

Note that this"workaround" was only possible for parameters (and with the possibly undesired side effect of making them optional), not returns (e.g. https://3v4l.org/3CrX5#v7.0.0 ), so several return type declarations couldn't be added before 7.1

sgolemon · 2021-07-20T18:33:02Z

Because operator precedence is known, the brackets are not needed and I'd prefer skipping them. Type expressions are already long enough.

While I don't like eliding the parens, I'm not going to veto it on those grounds, but would you consider making parens optional? It wouldn't make the change to the parser massively larger...

we are much too late for 8.1 at this point and syntax is too important to rush, so this is going to have to wait for 8.2 no matter what we do.

I would kindly ask you to reconsider this.

It's not my call at the end of the day. The 8.1 RMs (@ramsey, @patrickallaert, @krakjoe) have the most sway at this point in the release. I would certainly reiterate that going slow only costs us a little time. Rushing has potentially much worse consequences.

krakjoe · 2021-07-20T19:39:52Z

The least disruptive course of action seems to be for this detail to go through the normal RFC process, if voting on that closes before RC, we can just merge it.

guilliamxavier · 2021-07-21T09:20:15Z

While I don't like eliding the parens, I'm not going to veto it on those grounds, but would you consider making parens optional? It wouldn't make the change to the parser massively larger...

While at it: the current patch seems to allow X&Y|null but not null|X&Y (just saying)

Edit: OK, that's a deliberate choice

nicolas-grekas · 2021-08-27T20:22:17Z

The RFC is declined.

I'm closing to let someone else free to take over at any time in the future.

guilliamxavier reviewed Jul 19, 2021

View reviewed changes

Zend/tests/type_declarations/intersection_types/implicit_nullable_intersection_type.phpt Outdated Show resolved Hide resolved

nicolas-grekas force-pushed the nullableintersection branch from 0d9e382 to 014865b Compare July 19, 2021 15:16

Make intersection types nullable

e83f8db

nicolas-grekas force-pushed the nullableintersection branch from 014865b to e83f8db Compare July 19, 2021 15:28

sgolemon requested changes Jul 19, 2021

View reviewed changes

Use X&Y|null instead of ?X&Y

5bc9946

nicolas-grekas force-pushed the nullableintersection branch from 4d9b538 to 5bc9946 Compare July 20, 2021 08:20

krakjoe added the RFC label Jul 21, 2021

nikic added this to the PHP 8.1 milestone Jul 21, 2021

nicolas-grekas closed this Aug 27, 2021

nicolas-grekas deleted the nullableintersection branch August 27, 2021 20:22

nicolas-grekas restored the nullableintersection branch June 19, 2022 18:48

nicolas-grekas deleted the nullableintersection branch June 19, 2022 18:48

Make intersection types nullable #7259

Make intersection types nullable #7259

Uh oh!

Conversation

nicolas-grekas commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trowski commented Jul 19, 2021

Uh oh!

nicolas-grekas commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trowski commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jul 19, 2021

Uh oh!

Uh oh!

sgolemon Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

TysonAndre Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

iquito Jul 19, 2021 • 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!

sgolemon Jul 22, 2021 • 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!

Ocramius commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trowski commented Jul 19, 2021

Uh oh!

sgolemon commented Jul 19, 2021

Uh oh!

alcaeus commented Jul 20, 2021

Uh oh!

nicolas-grekas commented Jul 20, 2021 • edited by sgolemon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krakjoe commented Jul 20, 2021

Uh oh!

faizanakram99 commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guilliamxavier commented Jul 20, 2021

Uh oh!

sgolemon commented Jul 20, 2021

Uh oh!

krakjoe commented Jul 20, 2021

Uh oh!

guilliamxavier commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Aug 27, 2021

Uh oh!

Uh oh!

nicolas-grekas commented Jul 19, 2021 •

edited

Loading

nicolas-grekas commented Jul 19, 2021 •

edited

Loading

trowski commented Jul 19, 2021 •

edited

Loading

iquito Jul 19, 2021 •

edited

Loading

sgolemon Jul 22, 2021 •

edited

Loading

Ocramius commented Jul 19, 2021 •

edited

Loading

nicolas-grekas commented Jul 20, 2021 •

edited by sgolemon

Loading

faizanakram99 commented Jul 20, 2021 •

edited

Loading

guilliamxavier commented Jul 21, 2021 •

edited

Loading