Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122673
Precedence: bulk
MIME-Version: 1.0
References: <fca97d6c-d4ec-4000-958a-72c32beb8421@app.fastmail.com>
 <a715dacb-9bac-4bea-a2bb-c623737fd81e@app.fastmail.com> <c057b162-e2fe-4ade-b27f-e09815743748@rwec.co.uk>
 <7eada0fd-39c5-4a89-8c74-80c671801a2d@app.fastmail.com> <1698692e-8eb1-4bfc-a743-375696cd8f1c@rwec.co.uk>
 <154481e0-5f62-4026-994a-28a644d71527@app.fastmail.com> <C517590C-77D1-462C-92E9-D2851345BE67@rwec.co.uk>
 <e18863f1-9cc9-400b-b6b4-d1be62fa4d0f@app.fastmail.com> <46609F15-DD40-4BD2-A78A-16021C3447C8@rwec.co.uk>
 <CAPyj-LBEzCNzWkh8qwfBrC-ccMKFANX=zwNq9O0qSnNdFNjtzw@mail.gmail.com>
 <f95527c0-a0d5-48da-a071-ce1c4ffe6814@rwec.co.uk> <CAPyj-LC2=pdo79AbfBJbyb-e8JbZyswsaBeuFq=Zv80JpHs3fw@mail.gmail.com>
 <d1ca7dbe-71a3-40fc-a88e-e64d11606ea7@rwec.co.uk>
In-Reply-To: <d1ca7dbe-71a3-40fc-a88e-e64d11606ea7@rwec.co.uk>
Date: Mon, 18 Mar 2024 01:04:16 +0100
Message-ID: <CAPyj-LD7jBpF=3+61hjhTc_TGgUMejVWvWHRDrVxZebGdCBEKw@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC[ Property accessor hooks, take 2
To: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: tovilo.ilija@gmail.com (Ilija Tovilo)

Hi Rowan

On Sun, Mar 17, 2024 at 3:41=E2=80=AFPM Rowan Tommins [IMSoP]
<imsop.php@rwec.co.uk> wrote:
>
> The remaining difference I can see in the current RFC which seems to be
> unnecessary is that combining &get with set is only allowed on virtual
> properties. Although it may be "virtual" in the strict sense, any &get
> hook must actually be referring to some value stored somewhere - that
> might be a backed property, another field on the current class, a
> property of some other object, etc:
>
> public int $foo { &get =3D> $this->foo; set { $this->foo =3D $value; } }
>
> public int $bar { &get =3D> $this->_bar; set { $this->_bar =3D $value; } =
}
>
> public int $baz { &get =3D> $this->delegatedObj->baz; set {
> $this->delegatedObj->baz =3D $value; } }
>
> This sentence from the RFC applies equally to all three of these examples=
:
>
>  > That is because any attempted modification of the value by reference
> would bypass a |set| hook, if one is defined.
>
> I suggest that we either trust the user to understand that that will
> happen, and allow combining &get and set on any property; or we do not
> trust them, and forbid it on any property.

I'm indeed afraid that people will blindly make their array properties
by-reference, without understanding the implications. Allowing
by-reference behavior for virtual read/write properties is a tradeoff,
for cases where it  may be necessary. Exposing private properties
by-reference is already possible outside of hooks
(https://3v4l.org/VNhf7), that's not something we can prevent for
secondary backing properties. However, we can at least make sure that
a reference to the baking value of a hooked property doesn't escape.

I realize this is somewhat inconsistent, but I believe it is
reasonable. If you want to expose the underlying property
by-reference, you need to jump through some additional hoops.

> > Apart from the things already mentioned, it's unclear to me whether,
> > with such `set;` declarations, a `get`-only backed property should
> > even be legal. With the complete absence of a write operation, the
> > assignment within the `set` itself would fail. To make this work, the
> > absence of `set;` would need to mean something like "writable, but
> > only within another hook", which introduces yet another form of
> > asymmetric visibility.
>
> Any write inside the get hook already by-passes the set hook and refers
> to the underlying property, so there would be no need for any default
> set behaviour other than throwing an error.
>
> It's not likely to be a common scenario, but the below works with the
> current implementation https://3v4l.org/t7qhR/rfc#vrfc.property-hooks
>
> class Example {
>      public int $nextNumber {
>          get {
>              $this->nextNumber ??=3D 0;
>              return $this->nextNumber++;
>          }
>          // Mimic the current behaviour of a virtual property:
> https://3v4l.org/cAfAI/rfc#vrfc.property-hooks
>          set =3D> throw new Error('Property Example::$nextNumber is
> read-only');
>      }
> }

Again, it depends on how you think about it. As you have argued, for a
get-only property, the backing value should not be writable without an
explicit `set;` declaration. You can interpret `set;` as an
auto-generated hook, or as a marker that indicates that the backing
value is accessible without a hook. As mentioned in my previous
e-mail, auto-generated hooks is something we'd really like to avoid.
So, if the absence of `set;` means that the backing value is not
writable, the hook itself must be exempt from this rule.

Another thing to consider: The current implementation inherits the
backing value and all hooks from its parent. If the suggestion is to
add an explicit `set;` declaration to make it more obvious that the
property is writable, how does this help overridden properties?

```php
class P {
    public $prop {
        get =3D> strtolower($this->prop);
        set;
    }
}

class C extends P {
    public $prop {
        get =3D> strtoupper(parent::$prop::get());
    }
}
```

Even though `P::$prop` signals that it is writable, there is no such
indication in `C::$prop`. You may suggest to also add `set;` to the
child, but then what if the parent adds a custom implementation for
`set;`?

```php
class P {
    public $prop {
        get =3D> strtolower($this->prop);
        set {
            echo $value, "\n";
            $this->prop =3D $value;
        }
    }
}

class C extends P {
    public $prop {
        get =3D> strtoupper(parent::$prop::get());
        set;
    }
}
```

The meaning for `set;` is no longer clear. Does it mean that there's a
generated hook that accesses the backing field? Does it mean that the
backing field is accessible without a hook? Or does it mean that it
accesses the parent hook? The truth is, with inheritance there's no
way to look at the property declaration and fully understand what's
going on, unless all hooks must be spelled out for the sake of clarity
(e.g. `get =3D> parent::$prop::get()`).

> We are already allowing more than Kotlin by letting hooks call out to a
> method, and have that method refer back to the raw value.
> Hypothetically, we could allow *any* method to access it, using some
> syntax like $this->foo::raw. As a spectrum from least access to most acce=
ss:
>
> 1) $field - accessible only in the lexical scope of the hook
>
> 2) $this->foo - accessible in the dynamic scope of the hook, e.g. a hook
> calling $this->doSomething(__PROPERTY__);
>
> 3) $this->foo::raw - accessible anywhere in the class, e.g. a public
> clearAll() method by-passing hooks
>
> Whichever we provide for backed properties, option 3 is available for
> virtual properties anyway, and common with __get/__set: store a value in
> a private property, and have a public hooked property providing access
> to it.

I seriously doubt accessing the backing value outside of the current
hook is useful. The backing value is an implementation detail. If it
is absolutely needed, `ReflectionProperty::setRawValue()` offers a way
to do it. I understand the desire for a shorter alternative like
`$field`, but it doesn't seem like the majority shares this desire at
this point in time.

> I understand now that option 2 fits most easily with the implementation,
> and with decisions around inheritance and upgrade of existing code; but
> the other options do have their advantages from a user's point of view.

A different syntax like `$this->prop::raw` comes with similar
complexity issues, similar to those previously discussed for
`parent::$prop`/`parent::$prop =3D 'prop'`. All operations that
currently work out-of-the-box for the currently proposed syntax (read,
assign, assign by ref, assign with operation (+=3D, -=3D, etc.), inc/dec,
isset, send by-ref) would need new implementations. We could limit the
new syntax to read/assign/assign by ref, but that means more typing
for all other cases (e.g. `$this->prop::raw =3D $this->prop::raw + 1`
vs. `$this->prop++`). So, is that really better?

> My concern is more about the external impact of what is effectively a
> change to the type system of the language: will IDEs give correct
> feedback to users about which assignments are legal? will tools like
> PhpStan and Psalm require complex changes to analyse code using such
> properties? will we be prevented from adding some optimisation to
> OpCache because these properties break some otherwise safe assumption?

I don't consider Opcache a problem. An assignment on a non-final
property with mismatched types is no longer guaranteed to fail.
However, Opcache doesn't usually optimize error cases, e.g. replacing
them with direct exceptions. I can't speak for IDEs or static
analyzers, but I'm not sure what makes this case special. We can ask
some of their maintainers for feedback.

> Maybe I'm being over-cautious, but those are the kinds of questions I
> would expect to come up if this feature had its own RFC.

Of course, no worries. I'd rather hear it now than when the voting has
started. ;)

Ilija