Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119262
MIME-Version: 1.0
References: <ec2c86fc-53df-4ef2-ae9b-5e5dc76fbb3c@app.fastmail.com>
 <CAOWwgpnwpNWVMas98oPk_qkRJYZFK6CGuvCJa6v1sbKVxcOKYg@mail.gmail.com> <54e92c59-721f-458d-8858-058ac68e3a3e@app.fastmail.com>
In-Reply-To: <54e92c59-721f-458d-8858-058ac68e3a3e@app.fastmail.com>
Date: Wed, 11 Jan 2023 16:16:17 +0100
Message-ID: <CAOWwgpkhhryGHCFXf=-w6BW-N13B+hTU-Ea6Msoi4gO42Yx1Fw@mail.gmail.com>
To: Larry Garfield <larry@garfieldtech.com>
Cc: php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000001df8d205f1fe7cb1"
Subject: Re: [PHP-DEV] [RFC][Vote] Asymmetric Visibility
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--0000000000001df8d205f1fe7cb1
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Le mar. 10 janv. 2023 =C3=A0 00:00, Larry Garfield <larry@garfieldtech.com>=
 a
=C3=A9crit :

> On Mon, Jan 9, 2023, at 10:00 AM, Nicolas Grekas wrote:
> >> I am hereby opening the vote on the Asymmetric Visibility RFC:
> >>
> >> https://wiki.php.net/rfc/asymmetric-visibility
> >>
> >> Voting will be open for 2 weeks.
> >>
> >> While we would certainly prefer if everyone voted in favor, for those
> who
> >> vote against the authors kindly request that you indicate why, using t=
he
> >> second poll question or comment section afterward.  Thank you.
> >>
> >
> > Hi Larry, Ilija,
> >
> > Thanks for opening the vote. I played a bit with the code and I didn't
> spot
> > any unexpected behavior, nice work.
> >
> > I'm undecided for now, on two points:
> > 1. the syntax: looking at https://wiki.php.net/rfc/property-hooks, I
> wonder
> > if using public string $foo {private set;} wouldn't be more appropriate=
.
> > Otherwise, it might be strange to have public private(set) string $foo
> {set
> > ($value) { /* the setter */};} if hooks are the way forward. Sure we
> could
> > compact that but that would introduce two very different syntax whether=
 a
> > setter is defined or not. WDYT?
>
> As discussed, putting the visibility on the right has a number of problem=
s.
>
> 1. If we assume that hooks in something similar to the current unreleased
> RFC happen (which I hope they do), then putting the visibility in the hoo=
k
> area creates additional problems.
>
> For one, hooks will require disabling getting a reference to a property.
> That is not negotiable, as getting a reference to a property would let yo=
u
> bypass the hooks.  Aviz doesn't require that, though.  If the only thing
> you're doing is private-set, then using the hooks syntax for aviz means
> either introducing yet another access marker, like "private raw" to
> indicate that you don't want to disable references (I don't want to have =
to
> explain that to people), or just sucking it up and saying that aviz
> disables references.
>
> But disabling references also makes `$obj->arrayProp[] =3D 4` break
> entirely.  So that would make arrays completely incompatible with
> asymmetric visibility, when there's no actual reason that has to be the
> case.
>
> So in short, we end up with three options:
>
> A) The current proposed syntax.
> B) Having both "{ private set }" and "{private raw}", and having to
> explain WTH that even means.
> C) Arrays are incompatible with asymmetric visibility entirely, period.
>
> Of those, A is, I would argue, the clearly better option.
>
> 2. Some nerdy engine details, courtesy of Ilija.  Basically, given the wa=
y
> we're looking at doing hooks, mixing the hook and a-viz syntax would make
> it *more* complicated, not less.
>
> We're hoping to completely avoid generated accessors ({ get; set; } with
> no additional behavior) completely. Essentially, all they do is disable
> indirect modification (i.e. modifying arrays with []) and references
> (assigning a reference or extracting a reference). The reason they have t=
o
> do that is that both of those things allow mutating the value of the
> property in a way where we cannot call the accessors, either because the
> engine isn't aware of it or because there's no simple way to "feed" the
> modified value into the setter. There are two primary reasons generated
> modifiers existed in the previous draft: 1. To avoid a BC break when addi=
ng
> a hook in the future and 2. to allow sub-classes to override the property
> while staying compatible with the parent.
>
> As described here (
> https://gist.github.com/iluuu1994/4ca8cc52186f6039f4b90a0e27e59180#use-ca=
se-5-arrays-properties)
> we've come to the conclusion that this issue of incompatibility is almost
> exclusively restricted to array properties. However, there are other
> reasons why publicly exposing an array property with write access isn't a
> good idea, as outlined in the gist. In other words, this compatibility
> layer provided by { get; set; } was provided for something that shouldn't
> be a publicly writable property in the first place. We're now hoping to
> completely avoid the { get; set; } syntax which also makes public
> private(set) a better fit, instead of offering a fake { get; private set;=
 }
> syntax that is either limiting (i.e. not working properly for arrays) or
> semantically inconsistent (not preventing indirect modification and
> references).
>
> 3. More stylistically, it means visibility may end up split in two
> different locations.  That's harder for people to read.
>
> Especially if we add hooks later, it means they could be several lines
> apart!  Consider:
>
> public string $foo {
>   beforeSet {
>     return strtolower($value);
>   }
>   afterSet {
>     $this->modified['foo'] =3D true;
>   }
>   get =3D> $this->_foo;
>   protected set =3D> $this->_foo =3D $value;
> }
>
> This is a worst-case example, but worth considering as it would be valid,
> legal syntax and almost certainly appear in the wild.  The "set visibilit=
y"
> is 8 lines away from the "get visibility."  Too, the hooks are more
> confusing because only one of them supports an extra keyword; the other
> three have none.
>
> Even in a more reasonable ordering:
>
> public string $foo {
>   get =3D> $this->_foo;
>   protected set =3D> $this->_foo =3D $value;
>   beforeSet {
>     return strtolower($value);
>   }
>   afterSet {
>     $this->modified['foo'] =3D true;
>   }
> }
>
> Visibility is still spread across separate non-contiguous lines.
>
> Compare the same example with the syntax in the RFC now:
>
> public private(set) string $foo {
>   beforeSet {
>     return strtolower($value);
>   }
>   afterSet {
>     $this->modified['foo'] =3D true;
>   }
>   get =3D> $this->_foo;
>   set =3D> $this->_foo =3D $value;
> }
>
> It's now completely obvious that there's aviz in play.  I don't even have
> to look at the hooks to know that.
>

Thanks for 1., 2. and 3., the projection on hooks is useful and grouping
the modifiers at the top makes sense now.


Aviz and hooks simply do two different things in different parts of the
> engine.
>
> The poll we ran was split on this question, but did show a clear
> preference for what is in the RFC now.
>
>
> I think one of the points some people are hung up on is thinking of the
> hooks like methods, and thus they should have method-esque syntax.  That'=
s
> not an unreasonable first attempt at a mental model, but in practice I
> don't think it's accurate.  That's one reason the follow-up RFC renames
> accessors to "hooks", because they really are "hooks" into the existing
> property access workflow.  They're not methods unto themselves.  That mea=
ns
> "well methods have a visibility modifier so shouldn't these methods have
> one?" doesn't make sense, as the hooks are not methods.
>
> > 2. the feature itself: I feel like this new syntax doesn't open new
> > capabilities over using methods, so this might "just" add complexity in
> the
> > end, adding alternative ways to do something already doable (I'm making
> the
> > argument for hooks also BTW). Would you be able to clarify why we need
> > this? (The only new capability that feels appealing to me is the "init"
> > modifier, but this is not what this RFC is about.)
>
> It's true that *technically* anything that you can do with aviz can be
> done with methods.  However, the same is true of readonly.  Technically w=
e
> could also enforce types on properties through methods too, without needi=
ng
> property types.  And constructor promotion didn't enable any new
> functionality, just less typing/reading.  But all of these things improve
> the ergonomics of the language.
>
> In particular, aviz mainly lets us get rid of a whole host of pointless
> getter methods.  Consider a typical Doctrine model.
>
> class Point
> {
>   private string $id;
>   private int $x;
>   private int $y;
>   private int $z;
>
>   public function getId(): string
>   {
>     return $this->id;
>   }
>
>   public function getX(): int
>   {
>     return $this->x;
>   }
>
>   public function getY(): int
>   {
>     return $this->y;
>   }
>
>   public function getZ(): int
>   {
>     return $this->z;
>   }
>
>   public function setX(int $x): void
>   {
>     $this->x =3D $x;
>   }
>
>   public function setY(int $y): void
>   {
>     $this->y =3D $y;
>   }
>   public function setZ(int $z): void
>   {
>     $this->z =3D $z;
>   }
> }
>
> Aviz lets us remove 4 of the 7 methods in that class:
>
> class Point
> {
>   public private(set) string $id;
>   public private(set) int $x;
>   public private(set) int $y;
>   public private(set) int $z;
>
>   public function setX(int $x): void
>   {
>     $this->x =3D $x;
>   }
>
>   public function setY(int $y): void
>   {
>     $this->y =3D $y;
>   }
>   public function setZ(int $z): void
>   {
>     $this->z =3D $z;
>   }
> }
>

That's all I needed as a reminder, that was missing from the RFC :)
I'm all for reduced boilerplate.

What I like also is that all this (aviz+hooks) creates a forward path that
allows augmenting a property-based API. Right now, if you expose
public/protected properties, you're kinda stuck with them. It's true that
we can always write magic accessors to augment them, but it's hard to get
right. This constitutes a new capability, to counter my own argument :)



Assuming we still want some kind of control over the set actions, say
> enforcing positive values.  That's not necessary for the get side, only t=
he
> set side.
>
> As for the next obvious question of "well what if I do want to do
> something special on get, too?" In that rare instance, that's what the
> hooks RFC is for.
>
> Another use case is in my Serde library (https://github.com/Crell/Serde).
> I won't go too far into details here (as me in chat somewhere if you want
> the dirty details), but in short I have an object (an attribute class,
> actually) that does a lot of multi-stage stateful setup via reflection an=
d
> other mechanisms, and then is "frozen" with a bunch of public readonly
> properties.
>
> Being readonly is a very nice API and very convenient, but I have to do
> some tricky dancing in the setup code to make sure I only write to the
> properties once and in the right order.  There's some default-handling
> functionality that I want to add but don't know how to do safely while
> using a readonly property.  Right now, that's the only option if I want t=
o
> have public-read-only properties.  With a-viz, I think I could simplify
> some of the code by switching to public private(set), eliminate some of
> extra handling, and easily add another feature or two, all without an API
> change.
>
> Aviz also lets us side-step readonly's insistence on private-set.  Right
> now, there is no option for "public read, protected write",  In most case=
s,
> `public protected(set)` would be completely sufficient in practice, but i=
s
> not currently possible.
>
> > About my point 2., did you consider a syntax to map a getter+setter to
> > properties? That might fill the need for asym visibility + hooks in one
> go?
> > I'm seeing quite often libraries that do this sort of mapping based on
> > conventions. This would bake the practice into the language:
> >
> > public string $foo {get: getFoo, set: setFoo} or something like that,
> with
> > method visibility being added on top of the property visibility.
>
> The currently in-progress proposal for hooks supports almost that as a
> trivial option, thanks to the shorthand support.
>
> public string $foo {
>   get =3D> $this->someGetter();
>   set =3D> $this->someSetter($value);
>   beforeSet =3D> $this->someCallback($value);
> }
>
> But you can also inline it if it's simple.  It gives developers both
> options.
>
> We did consider using methods directly for hooks, more akin to how JS and
> Python do it.  We eventually rejected that entirely as trying to keep the
> visibility and the type in sync between the property and the methods woul=
d
> be... really really hard, both for the implementation and for developers.
> With the current setup, there is no opportunity for confusion in the engi=
ne
> and extremely little for the developer.
>
> > Sorry to chime in that late, I'm really undecided and discussing those
> > topics would help.
> >
> > Thanks,
> > Nicolas
>
> I'm honestly a bit surprised to see this comment from you, as you coined
> the term "asymmetric visibility" in the first place, and IIRC objected to
> `readonly` on the grounds it would make aviz harder.  (Turns out, you wer=
e
> right. :-) )  Still, I hope I was able to clarify things.
>

Just doing my homework as a voter, rereading the RFC with a fresh mind,
accounting for readonly being a thing also :)

Nicolas

--0000000000001df8d205f1fe7cb1--