Skip to content

Don't make argument nullable based on AST null initializer #4720

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

Closed
wants to merge 1 commit into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Sep 18, 2019

This removes a hack that was added in PHP 5.6 (iirc) prior to the introduction of nullable types. The issue are cases like:

function foo(int $a = SOME_CONSTANT) {}

where SOME_CONSTANT evaluates to null at runtime. In this case, we will allow null as a value for the parameter, implicitly promoting the type to ?int. There are multiple problems with this magic:

  • The type may change at runtime, for example:
<?php
namespace Foo;
function test(int $foo = SOME_CONST) { return $foo; }

define('SOME_CONST', null);
var_dump(test(null)); // works
define('Foo\SOME_CONST', 42);
var_dump(test(null)); // TypeError
  • Reflection does not understand this. It will always report the type as non-nullable.
  • This breaks our LSP/variance checks. For example, this code is permitted, but shouldn't be, because it changes a ?int parameter to an int parameter (effectively).
<?php
class A {
    public function test(int $foo = SOME_CONST1) {}
}

class B extends A {
    public function test(int $foo = SOME_CONST2) {}
}

const SOME_CONST1 = null;
const SOME_CONST2 = 42;

I think we should remove this hack in master. Instead, people can use an explicit nullable type:

function foo(?int $a = SOME_CONSTANT) {}

It should be noted that this does not affect the case where a compile-time null value is used. So

function foo(int $a = null) {}

continues working the same. Keeping this is not a problem, because we determine at compile-time that the real type is ?int, so all the above issues don't apply.

@dstogov Does this change look okay to you?

@dstogov
Copy link
Member

dstogov commented Sep 18, 2019

I agree, it was a hack and it's good to remove it.
Dispite, this makes a BC break, it's quite small.

@php-pulls php-pulls closed this in cdd4e59 Sep 19, 2019
@carusogabriel carusogabriel added this to the PHP 8.0 milestone May 29, 2020
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.

3 participants