Skip to content
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

null default-value promoted class parameter before required parameter is silently ignored #11488

Closed
syranide opened this issue Jun 20, 2023 · 4 comments

Comments

@syranide
Copy link

syranide commented Jun 20, 2023

Description

The following code:

<?php
class Foobar {
  public function __construct(
      public string|null $a = null,
      public $b,
  ) {}
}

new Foobar(b: 'x');

Resulted in this output:

PHP Fatal error:  Uncaught ArgumentCountError: Foobar::__construct(): Argument #1 ($a) not passed

But I expected this output instead:

PHP Deprecated: Optional parameter $a declared before required parameter $b is implicitly treated as a required parameter

PHP Fatal error:  Uncaught ArgumentCountError: Foobar::__construct(): Argument #1 ($a) not passed

To explain; $a does not end up being optional, and if you perform reflection on the parameter and/or property, neither parameter/property has a default-value. So PHP is silently ignoring the user-specified behavior, which only surfaces once you do the call to the function (or as it is sometimes in our case, it doesn't show up at all because some objects are created via reflection).

Further information

Normal functions and methods support "poor mans nullable types" for now, which makes the above case a weird quirk of the backwards compatibility, as noted by @KapitanOczywisty in this post #11485 (comment).

However, constructors with parameter/property promotion behave in a stricter manner.

class C1 {
  public function __construct(
      public string|null $a = null,
      public $b,
  ) {}
}

class C2 {
  public function __construct(
      public string|null $a = "a",
      public $b,
  ) {}
}

class C3 {
  public function __construct(
      public string $a = null,
      public $b,
  ) {}
}

class C4 {
  public function __construct(
      public string $a = "a",
      public $b,
  ) {}
}

class C5 {
  public function __construct(
      public $a = null,
      public $b,
  ) {}
}
  • C1 emits no error, but silently makes the parameter/property mandatory.
  • C2 emits "Deprecated: Optional parameter $a declared before required parameter $b is implicitly treated as a required parameter".
  • C3 emits "Fatal error: Cannot use null as default value for parameter $a of type string".
  • C4 emits "Deprecated: Optional parameter $a declared before required parameter $b is implicitly treated as a required parameter".
  • C5 emits "Deprecated: Optional parameter $a declared before required parameter $b is implicitly treated as a required parameter".

C3 proves that poor mans nullable types aren't intended to apply to constructors with typed promoted parameters/properties like they do for normal parameters, and C5 proves that it's deprecated for them regardless. And C2 and C4 proves that optional parameters aren't allowed before required parameters and emits a deprecation.

However, C1 emits no warning at all, even though parameter/property doesn't end up having a null default-value, nor is it optional. So C1 behaves identically to not specifying a default-value at all, whereas the only purpose of specifying the null default-value is to make it optional, which it isn't.

So it definitely seems like C1 should emit a deprecation as well.

PHP Version

PHP 8.2.7

Operating System

No response

@KapitanOczywisty
Copy link

KapitanOczywisty commented Jun 20, 2023

Deprecation notice would be added before Fatal error, so expected result should be:

Deprecated: Optional parameter $a declared before required parameter $b is implicitly treated as a required parameter

PHP Fatal error:  Uncaught ArgumentCountError: Foobar::__construct(): Argument #1 ($a) not passed

@syranide
Copy link
Author

syranide commented Jun 20, 2023

Fair point, in-terms of clarity, I'll update it. I was debating not including the function call at all, but it helps illustrate the context better.

@iluuu1994
Copy link
Member

I was about to classify this as a feature, but noticed that this already works correctly for ?string, but not for string|null (https://3v4l.org/uvbF7). The reason is that the null check on the type declaration is AST based, and naively assumes only ?string exists. We should instead delay this check to type compilation.

php-src/Zend/zend_compile.c

Lines 6958 to 6960 in 6c015ae

bool is_implicit_nullable =
type_ast && !(type_ast->attr & ZEND_TYPE_NULLABLE)
&& Z_TYPE(default_node.u.constant) == IS_NULL;

Constructor property promotion fails for a different reason, as stated in the RFC:

https://wiki.php.net/rfc/constructor_promotion

Similarly, because promoted parameters imply a property declaration, nullability must be explicitly declared, and is not inferred from a null default value:

class Test {
    // Error: Using null default on non-nullable property
    public function __construct(public Type $prop = null) {}
 
    // Correct: Make the type explicitly nullable instead
    public function __construct(public ?Type $prop = null) {}
}

The RFC requires an explicit ? to avoid confusion on whether property type includes null or not.

iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Jun 21, 2023
… null type

The check would only work for the ?type syntax, but not  type|null. Switch to a
check during type compilation instead.

Fixes phpGH-11488
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Jun 21, 2023
… null type

The check would only work for the ?type syntax, but not  type|null. Switch to a
check during type compilation instead.

Fixes phpGH-11488
@iluuu1994
Copy link
Member

#11497 I still targeted master for this. The fix was not completely trivial, the bug is non-crucial and 8.3 will still allow fixing the error before removal (if that's even planned).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants