[8.4] Widening property hook's type for promoted property does not work

[Wirone]

Description

The following code:

<?php

declare(strict_types=1);

class Foo
{
    public function __construct(
        public int $baz { set (int|string|stdClass $value) {
            if (is_int($value)) {
                $this->baz = $value;
            } else if ($value instanceof stdClass
                && property_exists($value, 'baz')
                && is_int($value->baz)
            ) {
                $this->baz = $value->baz;
            } else if (is_numeric($value)) {
                $this->baz = (int) $value;
            } else {
                throw new InvalidArgumentException('Invalid value');
            }
        } },
    ) {}
}

$obj = new stdClass();
$obj->baz = 123;

var_dump(new Foo($obj));

Resulted in this output:

Fatal error: Uncaught TypeError: Foo::__construct(): Argument #1 ($baz) must be of type int, stdClass given, called in /in/2mZY5 on line 29 and defined in /in/2mZY5:7
Stack trace:
#0 /in/2mZY5(29): Foo->__construct(Object(stdClass))
#1 {main}
  thrown in /in/2mZY5 on line 7

But I expect this to work instead, just like when property is not promoted.

Originally encountered when preparing Twitter thread.

PHP Version

8.4 (current master)

Operating System

No response

[bwoebi]

@iluuu1994 It's sort of a buggish inconsistency. Not sure I'd classify that as feature...

[iluuu1994]

@bwoebi It's not a bug. I'm not even sure the proposed feature makes sense at all. E.g. consider https://3v4l.org/SK5ZY/rfc#vgit.master, it's not possible to statically know about the type of $value in set.

[Wirone]

@iluuu1994 it should be possible either using both approaches (promoted and regular property), or none of them IMHO. The RFC clearly states that widening type is possible in property hook's setter, I don't see a reason for not allowing it in promoted property, since it's only a syntax sugar (more or less).

[iluuu1994]

I don't see a reason for not allowing it in promoted property, since it's only a syntax sugar (more or less).

class Foo {
    public function __construct(
        public int $baz { set (int|string|stdClass $value) {} },
    ) {}
}

desugars to

class Foo {
    public int $baz { set (int|string|stdClass $value) {} };

    public function __construct(int $baz) {
        $this->baz = $baz;
    }
}

It's possible to special-case this and make the constructor parameter $baz use the type from set(), but that's not possible when set is inherited, because the type is not known statically.

class Foo {
    public string $prop {
        set(string|int $value) => $value;
    }
}

class Bar extends Foo {
    public function __construct(
        public string $prop,
    ) {}
}

Constructor-property promotion aims to reduce clutter for the common case, I don't believe it needs to cover every case. IMO, there's nothing unexpected here. /cc @Crell

[Wirone]

Personally I wouldn't expect set hook being inherited in child class, where promoted property is re-declared explicitly without it 🤔. Just like I wouldn't expect that widening type in setter-hook is supported only for non-promoted properties. These behaviours are surprising for me, but looking at the de-sugared code above it actually makes sense for the promoted property scenario to use property's type, not the hook setter's one. I'll leave decision to you, as you have much better knowledge around this.

However, it has a lot of gotchas, and I hope it will be well-documented when it's released 🙂.

[iluuu1994]

Personally I wouldn't expect set hook being inherited in child class, where promoted property is re-declared explicitly without it

If hooks were not inherited, it would be impossible for the parent to add behavior to a property consistently, unless every overridden property always added the { get => parent::$prop::get(); set { parent::$prop::set($value); } } boilerplate. Instead of accidentally dropping the parent implementation, children that wish not to inherit the hook need to explicitly override it and customize the behavior as desired.

[Crell]

Hm. This is an unfortunate edge case we didn't think of. With Ilija's explanation, though, it's entirely logical how we got here. So I'd say it is unexpected, but the answer is to just document the reason why this particular edge case doesn't work, not try to work around it. I'll try to remember to cover that whenever I get to writing the hooks docs.

[NickSdot]

Hi there. I have a lot of respect for your work, and am very happy that we will get property hooks. Thank you!

Please allow me to add my userland perspective here. Like the OP, I would have expected this to work. IMHO, it is completely unexpected and inconsistent that it does not. From a userland and logical point of view, it only makes sense for the above example to work.

Why? Because the setter ensures that the final value type to set matches the value type expected by the constructor.

This is an unfortunate edge case we didn't think of. With Ilija's explanation, though, it's entirely logical how we got here.

Yes, it is unfortunate. It's a big feature. We all understand that not everything can be thought of in advance.

However, I am concerned about "just let's document it". I don't want to be the bad guy here. But do we really want to bake more inconsistencies into the language? Shouldn't a new language feature be free of it? Is simply accepting and documenting it really the way to go here?

Yesterday I wrote the following on Twitter:

Then hooks rather shouldn’t be allowed in constructors IMO. If implementation is too hard, it might be a sign that it shouldn’t be done.

Baking in inconsistencies will either cause that we live with them forever or one day end up as a breaking change (see uname).

Unfortunate.

Perhaps for now disallowing the use of property hooks in constructors is the better choice?
And if a consistent solution can be found, it can be allowed later?

As a user, I'd appreciate it if this could be considered. Thank you advance. 🙏

[Crell]

As Ilija noted, it's not clear how it could be expanded to also handle type changes in constructor promotion. There's inheritance weirdness that makes it extra complicated. (Inheritance makes everything harder.) As we're now in RC3, ripping out a feature (hooks on promoted properties) is also not really on the table. It may have been in the betas, but it's too late to be pulling things out, even if we agreed it made sense to do (which doesn't seem to be the case).

If we can (now or in the future) figure out a way to have the expanded type translate to the constructor argument, that would be fine. But it seems like that's not as easy as it sounds.

[iluuu1994]

@NickSdot Thanks for your comment. Like Larry said, it's basically too late to make such changes, especially without discussing them with the community. As mentioned, disallowing hooks in CPP doesn't actually solve the problem for inheritance (https://3v4l.org/SK5ZY/rfc#vgit.master). I also believe that if we found a way to fix this in the future, we could do so without a BC break. Widening parameters is backwards compatible, and constructors are not subject to LSP rules.