Hi Rowan On Sun, Mar 17, 2024 at 3:41 PM Rowan Tommins [IMSoP] <imsop....@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 => $this->foo; set { $this->foo = $value; } } > > public int $bar { &get => $this->_bar; set { $this->_bar = $value; } } > > public int $baz { &get => $this->delegatedObj->baz; set { > $this->delegatedObj->baz = $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 ??= 0; > return $this->nextNumber++; > } > // Mimic the current behaviour of a virtual property: > https://3v4l.org/cAfAI/rfc#vrfc.property-hooks > set => 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 => strtolower($this->prop); set; } } class C extends P { public $prop { get => 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 => strtolower($this->prop); set { echo $value, "\n"; $this->prop = $value; } } } class C extends P { public $prop { get => 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 => 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 access: > > 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 = 'prop'`. All operations that currently work out-of-the-box for the currently proposed syntax (read, assign, assign by ref, assign with operation (+=, -=, 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 = $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