Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119251
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-1185-g841157300a-fm-20221208.002-g84115730
Mime-Version: 1.0
Message-ID: <54e92c59-721f-458d-8858-058ac68e3a3e@app.fastmail.com>
In-Reply-To: 
 <CAOWwgpnwpNWVMas98oPk_qkRJYZFK6CGuvCJa6v1sbKVxcOKYg@mail.gmail.com>
References: <ec2c86fc-53df-4ef2-ae9b-5e5dc76fbb3c@app.fastmail.com>
 <CAOWwgpnwpNWVMas98oPk_qkRJYZFK6CGuvCJa6v1sbKVxcOKYg@mail.gmail.com>
Date: Mon, 09 Jan 2023 17:00:09 -0600
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] [RFC][Vote] Asymmetric Visibility
From: larry@garfieldtech.com ("Larry Garfield")

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 the
>> 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 problems.

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 hook 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 you 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[] = 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 way 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 to 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 adding 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-case-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'] = true;
  }
  get => $this->_foo;
  protected set => $this->_foo = $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 visibility" 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 => $this->_foo;
  protected set => $this->_foo = $value;
  beforeSet { 
    return strtolower($value);
  }
  afterSet {
    $this->modified['foo'] = 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'] = true;
  }
  get => $this->_foo;
  set => $this->_foo = $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.

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 means "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 we could also enforce types on properties through methods too, without needing 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 = $x;
  }

  public function setY(int $y): void
  {
    $this->y = $y;
  }
  public function setZ(int $z): void
  {
    $this->z = $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 = $x;
  }

  public function setY(int $y): void
  {
    $this->y = $y;
  }
  public function setZ(int $z): void
  {
    $this->z = $z;
  }
}

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 the 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 and 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 to 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 cases, `public protected(set)` would be completely sufficient in practice, but is 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 => $this->someGetter();
  set => $this->someSetter($value);
  beforeSet => $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 would be... really really hard, both for the implementation and for developers.  With the current setup, there is no opportunity for confusion in the engine 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 were right. :-) )  Still, I hope I was able to clarify things.

--Larry Garfield