Correct and test none/null return types distinction

[hikari-no-yume]

Currently:

1. function foo(): array {} produces the "must be an instance of int, none returned" error
2. function foo(): array { return; } produces the "must be an instance of int, null returned" error
3. function foo(): array { return null; } produces the "must be an instance of int, null returned" error

I think the 2nd case is wrong. return; doesn't return an explicit value, neither does omitting a return statement altogether, so it should say "none" and not "null" for that case.

This patch fixes the 2nd case to produce "none", and adds tests.

[morrisonlevi]

I'm wondering if we can do better messages for return type errors in general. Something like:

Catchable fatal error: function lacks_return() must return an array but returned nothing in %s on line %d

Instead of:

Catchable fatal error: Return value of lacks_return() must be of the type array, none returned in %s on line %s

What do you think?
/cc @dstogov

[hikari-no-yume]

Producing "but" seems weird to me for some reason. I wrote a unit testing library that used "but" in its assertion failure error messages, and it just sounded wrong. I can't put my finger on why, though.

[morrisonlevi]

We could use "and" or "yet" there as well, if you like those better.

[laruence]

maybe use the similar to the warning message like parse args?

""%s%s%s() expects parameter %d to be %s, %s given""

%s expects return type of array , null returned in %s on %d?

[dstogov]

Hi Levi,

Change it as you wish (just update the tests accordingly).
I'm not a native speaker, and can't suggest anything.
Just commit it.

Thanks. Dmitry.

On Fri, Feb 13, 2015 at 5:06 AM, Levi Morrison notifications@github.com
wrote:

I'm wondering if we can do better messages for return type errors in
general. Something like:

Catchable fatal error: function lacks_return() must return an array but
returned nothing in %s on line %d

What do you think?
/cc @dstogov https://github.com/dstogov

—
Reply to this email directly or view it on GitHub
#1080 (comment).

[hikari-no-yume]

Instead of

Return value of lacks_return() must $type1, $type2 returned

perhaps it could be

lacks_return() must $type1, $type2 returned

so:

• $type1 = "return a value of the type array", $type2 = "array"
• $type1 = "not return a value", $type2 = "null"
• $type1 = "return an instance of FooBar", $type2 = "no value"

?

[hikari-no-yume]

As an interim solution, though, "no value" is fine, so I've updated the patch to use that instead of "none".

[hikari-no-yume]

Current travis failure is unrelated to changes in patch.

[bwoebi]

It rather looks to me like first case is wrong. It's well-defined that no defined return value means null is returned. So please reflect this in the error message too.

[hikari-no-yume]

@bwoebi Surely, then, the lack of a return statement should do the same?

[hikari-no-yume]

Also, bear in mind that what you say is only true since PHP 7.

[bwoebi]

@TazeTSchnitzel It always returned null. Lack of return statement should give null in the error message too, yes. As said, first error message is wrong to me.

[hikari-no-yume]

It always returned null, but there's a distinction between explicitly returning null, and implicitly receiving null if nothing is returned.

[bwoebi]

Well, at the end of the op_array is always an implicit return null;. So, we always explicitly receive null, we just maybe don't explicitly return null, but if's not explicitly returned, it's returned implicitly… So we always return null at the end.

And by that logic (which also always was my view on that), it's always null, not "none". There's no such thing like no return value in PHP. Every function has its return value, and if it's null.

[hikari-no-yume]

That's not true. In PHP 5, functions could return either nothing, or null.

[bwoebi]

Example of something returning nothing in PHP 5?

[staabm]

@TazeTSchnitzel see http://3v4l.org/H0U0O

[hikari-no-yume]

@staabm Yes, I'm aware that if you try to get at the return value you'll see NULL. That's not what I'm referring to.

[bwoebi]

@TazeTSchnitzel then sorry, I don't get your point.

[hikari-no-yume]

It's an internal difference only, I think (return_value == NULL IIRC). But it's a difference nonetheless.

Even if they effectively do the same thing, there's a difference in intentions. While return; and an empty block do ultimately result in a NULL return value, they mean different things to someone reading them. You omit a return, or use return;, when the return value is unused (i.e. void function). You include return value when the return value is used (e.g. ?int).

[hikari-no-yume]

Another way to think of it is that magic is bad: where did that NULL come from? It's not useful to say NULL was returned, because it's a NULL PHP has created. The function hasn't explicitly returned a value, so don't act as if it has. It's returned no value, and PHP has filled in a NULL for it.

[bwoebi]

A function implicitly returns a NULL and caller explicitly receives NULL. That's my view on it. Period. It's fine if you disagree, but don't merge this.

[hikari-no-yume]

If it's an implicit NULL return, then you shouldn't produce the same message as an explicit NULL return.

Also, the patch and RFC already make this distinction. This just clears up a minor erratum (return; not treated the same as implicit return).

[bwoebi]

The RFC doesn't. (If I didn't miss anything in my quick glance now)

The patch does, but shouldn't. Should be fixed in the patch.

[hikari-no-yume]

Why? It'd be inconsistent with parameters...

[bwoebi]

With which parameters?

[hikari-no-yume]

@bwoebi There's a distinction between missing and NULL parameters

[bwoebi]

@TazeTSchnitzel That's completely unrelated and not an inconsistency.

[hikari-no-yume]

Not really...

[hikari-no-yume]

It's ultimately a question of user-friendly error messages. Talking about NULL return values when you haven't returned any value whatsoever is not user-friendly. Yes, PHP implicitly returns NULL for you. That's not the same. If someone doesn't return a value, give them a different error. :/