Nullable types#1893

morrisonlevi · 2016-04-28T21:43:25Z

This is an implementation for the Nullable Types RFC.

hikari-no-yume · 2016-04-28T22:13:20Z

Correct me if I'm wrong, but I don't think the tests currently check for the interaction between ? and = null in inheritance?

morrisonlevi · 2016-04-28T22:15:56Z

Indeed, I haven't added tests for this part. Will look into it later.

hikari-no-yume · 2016-04-28T22:19:01Z

The RFC has some potential test-cases, I guess: https://wiki.php.net/rfc/nullable_types#default_values

dstogov · 2016-04-28T22:57:39Z

The patch misses inherited method compatibility rules described in RFC.
You'll probably need just uncomment corresponding block in zend_inheritance.c.
RFC should mention a corresponding BC break.

morrisonlevi · 2016-04-28T23:06:04Z

I have spot checked method compatibility and didn't see any issue with the patch. Can you provide examples for both situations, @dstogov?

dstogov · 2016-04-28T23:11:30Z

See the second example from https://wiki.php.net/rfc/nullable_types#parameters

From: Levi Morrison notifications@github.com
Sent: Friday, April 29, 2016 2:07:05 AM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Nullable types (#1893)

I have spot checked method compatibility and didn't see any issue with the patch. Can you provide examples for both situations, @dstogovhttps://github.com/dstogov?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-215588431

morrisonlevi · 2016-04-29T00:22:30Z

I see an issue with the formatting in the error string but the behavior looks to be correct. Where is the backwards compatibility break?

dstogov · 2016-04-29T08:14:15Z

You propose to disable overriding nullable with non-nullable, so the following code (from your email) won't work any more (it works in 7.0).

From: Levi Morrison notifications@github.com
Sent: Friday, April 29, 2016 3:23:37 AM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Nullable types (#1893)

I see an issue with the formatting in the error string but the behavior looks to be correct. Where is the backwards compatibility break?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-215600292

dstogov · 2016-04-29T08:26:10Z

Oh, I see - Bob just silently reintroduced the BC break bypassing the RFC [😊]

I reverted this. This break is actually a part of your RFC.

From: Dmitry Stogov
Sent: Friday, April 29, 2016 11:14:10 AM
To: php/php-src; php/php-src
Cc: Mention
Subject: Re: [php/php-src] Nullable types (#1893)

You propose to disable overriding nullable with non-nullable, so the following code (from your email) won't work any more (it works in 7.0).

From: Levi Morrison notifications@github.com
Sent: Friday, April 29, 2016 3:23:37 AM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Nullable types (#1893)

I see an issue with the formatting in the error string but the behavior looks to be correct. Where is the backwards compatibility break?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-215600292

hikari-no-yume · 2016-04-29T09:57:28Z

I'm surprised that example works in 7.0 at all, it seems like an LSP violation.

morrisonlevi · 2016-04-29T14:32:52Z

Dmitry,

The following code fails with an error since PHP 5.3+ ^{(https://3v4l.org/kRKUN)}:

interface Fooable {
    function foo(Fooable $f = null);
}
interface StrictFoo extends Fooable {
    function foo(Fooable $f);
}

Thus this is not a BC break:

interface Fooable {
    function foo(?Fooable $f);
}
interface StrictFoo extends Fooable {
    function foo(Fooable $f);
}

The BC break you are thinking of has to do with changing the default parameter in this subclass so it isn't null anymore. That has to do with default parameters, not nullable parameters, and is therefore not part of this RFC.

Does that make sense?

dstogov · 2016-04-29T17:56:57Z

Sorry, I'm far from computers till Wednesday. The example I sent you works on PHP 7.0.5.

-------- Original Message --------
From:Levi Morrison
Sent:Fri, 29 Apr 2016 17:34:01 +0300
To:php/php-src
Cc:Dmitry Stogov ,Mention
Subject:Re: [php/php-src] Nullable types (#1893)

Dmitry,

The following code fails with an error since PHP 5.3+ (https://3v4l.org/kRKUN):

interface Fooable {
function foo(Fooable $f = null);
}
interface StrictFoo extends Fooable {
function foo(Fooable $f);
}

Thus this is not a BC break:

interface Fooable {
function foo(?Fooable $f);
}
interface StrictFoo extends Fooable {
function foo(Fooable $f);
}

The BC break you are thinking of has to do with changing the default parameter in this subclass so it isn't null anymore. That has to do with default parameters, not nullable parameters, and is therefore not part of this RFC.

Does that make sense?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-215737327

This works off of Dmitry's commit for nullable return types

This also affects bug #72119

Majkl578 · 2016-05-11T00:38:08Z

Hi,
I think error messages should be adjusted as well, now it does not say anything about whether the argument is nullable or not. For example:

(function (?int $x) {})();
// Fatal error: Uncaught TypeError: Argument 1 passed to {closure}() must be of the type integer, none given

(function () : ?bool {})();
// Fatal error: Uncaught TypeError: Return value of {closure}() must be of the type boolean,

dstogov · 2016-05-11T05:17:22Z

Good point.

From: Michael Moravec notifications@github.com
Sent: Wednesday, May 11, 2016 3:39:16 AM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Nullable types (#1893)

Hi,
I think error messages should be adjusted as well, now it does not say anything about whether the argument is nullable or not. For example:

(function (?int $x) {})();
// Fatal error: Uncaught TypeError: Argument 1 passed to {closure}() must be of the type integer, none given

(function () : ?bool {})();
// Fatal error: Uncaught TypeError: Return value of {closure}() must be of the type boolean,

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-218332152

hikari-no-yume · 2016-05-11T09:11:42Z

Perhaps it could say "must be either null or of the type integer", or even "must be either of the type null or of the type integer".

dstogov · 2016-05-11T09:19:19Z

I would change the error message from "must be of the type X" to "must be of the type X or null"

From: Andrea Faulds notifications@github.com
Sent: Wednesday, May 11, 2016 12:12:46 PM
To: php/php-src
Cc: Dmitry Stogov; Mention
Subject: Re: [php/php-src] Nullable types (#1893)

Perhaps it could say "must be either null or of the type integer", or even "must be either of the type null or of the type integer".

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/1893#issuecomment-218404288

theodorejb · 2016-05-11T13:42:07Z

Shouldn't there be a test to ensure that the following is not allowed?

function nullableVoid(): ?void {}

Majkl578 · 2016-05-11T14:33:24Z

How about:

... must be either of the type integer or null, ...

Imho null should be the latter in the sentence since it's not a primary type.

@theodorejb: And maybe also nullable null (?null) as well, although it's not a valid type hint now, but who knows if union types get accepted.

morrisonlevi · 2016-05-12T22:39:34Z

?void is currently allowed in the patch. I planned on handling it after the Union Types RFC has been voted on. I don't want to write code to reject ?void if it's just going away.

hikari-no-yume · 2016-05-13T19:36:51Z

Reverting a change made by a previously accepted RFC could be quite controversial. You might want to make a separate RFC for that.

morrisonlevi force-pushed the nullable_types branch from 0759661 to e670226 Compare April 29, 2016 14:41

Implement nullable return types.

49fe737

morrisonlevi force-pushed the nullable_types branch from e670226 to a5a7e90 Compare May 5, 2016 15:34

morrisonlevi added 2 commits May 5, 2016 11:53

Add nullable parameter types

9662259

This works off of Dmitry's commit for nullable return types

Fix bug #71428

56c3d75

This also affects bug #72119

morrisonlevi force-pushed the nullable_types branch from a5a7e90 to 56c3d75 Compare May 5, 2016 18:15

php-pulls merged commit 56c3d75 into php:master May 23, 2016

malukenho mentioned this pull request May 23, 2016

Nullable params type Ocramius/ProxyManager#314

Closed

This was referenced Jun 4, 2016

Implemented "Nullable Types" #1045

Closed

Implement nullable return types. #1851

Closed

Uh oh!

Conversation

morrisonlevi commented Apr 28, 2016

Uh oh!

hikari-no-yume commented Apr 28, 2016

Uh oh!

morrisonlevi commented Apr 28, 2016

Uh oh!

hikari-no-yume commented Apr 28, 2016

Uh oh!

dstogov commented Apr 28, 2016

Uh oh!

morrisonlevi commented Apr 28, 2016

Uh oh!

dstogov commented Apr 28, 2016

Uh oh!

morrisonlevi commented Apr 29, 2016

Uh oh!

dstogov commented Apr 29, 2016

Uh oh!

dstogov commented Apr 29, 2016

Uh oh!

hikari-no-yume commented Apr 29, 2016

Uh oh!

morrisonlevi commented Apr 29, 2016

Uh oh!

dstogov commented Apr 29, 2016

Uh oh!

Majkl578 commented May 11, 2016

Uh oh!

dstogov commented May 11, 2016

Uh oh!

hikari-no-yume commented May 11, 2016

Uh oh!

dstogov commented May 11, 2016

Uh oh!

theodorejb commented May 11, 2016

Uh oh!

Majkl578 commented May 11, 2016

Uh oh!

morrisonlevi commented May 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hikari-no-yume commented May 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

morrisonlevi commented May 12, 2016 •

edited

Loading