Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:116622
MIME-Version: 1.0
References: <CAMrTa2FB-EVGq29oLCU8fYkMYi+KsQiEAFv1xxMFO7vnm8e6=Q@mail.gmail.com>
 <CA+kxMuQqGpkuqwFLrW29G38aOUNbvXM6Dq=P8NXt=1zai_=zxA@mail.gmail.com>
In-Reply-To: <CA+kxMuQqGpkuqwFLrW29G38aOUNbvXM6Dq=P8NXt=1zai_=zxA@mail.gmail.com>
Date: Sat, 11 Dec 2021 11:13:41 -0800
Message-ID: <CAMrTa2H3Su5GBsxqie3hs9ugDVzMgu0gPUxFFDbFhQcj0QCT7A@mail.gmail.com>
To: Dan Ackroyd <Danack@basereality.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000edc20c05d2e3a3e8"
Subject: Re: [PHP-DEV] [RFC] User Defined Operator Overloads (v0.6)
From: jordan.ledoux@gmail.com (Jordan LeDoux)

--000000000000edc20c05d2e3a3e8
Content-Type: text/plain; charset="UTF-8"

On Sat, Dec 11, 2021 at 7:15 AM Dan Ackroyd <Danack@basereality.com> wrote:

>
> There are quite a few downstream costs for making a new type of
> methods in classes. All projects that analyze code (rector,
> codesniffer, PHPStorm, PhpStan/Psalm, PHPUnit's code coverage
> annotations etc) would have to add a non-trivial amount of code to not
> bork when reading the new syntax. Requiring more code to be added and
> maintained in PHP's builtin Reflection extension is also a cost.
> That's quite a bit of work for a feature that has relatively rare
> use-cases.
>

While I'm not suggesting that this isn't an important consideration for
voters with this RFC, (I think it should be weighed for sure), I *think*
that all of the things you mentioned will need similar updates to work
correctly with this RFC even if it was done with plain old magic methods
instead. Again, I'm not saying it's not important, but isn't this true of
many RFCs that add new syntax, such as enums or even things like array
unpacking?

As for Reflection, I hadn't gotten to that yet, but I actually pushed the
commit for it with full test coverage of the changes last night. It wasn't
actually that big of a change, and the latest commit on the PR can be
checked out and played if you want to test it. With support for the enum
casing as well. Operators get an additional function flag ZEND_ACC_OPERATOR
that made it fairly simple to implement the Reflection changes with minimal
additional code. The new methods I mentioned are actually smaller that the
implementations for the existing ones. As far as maintainability goes,
removing this aspect doesn't make this RFC more maintainable in core in my
opinion. It becomes harder to maintain it I think, as it requires more
special casing in other places that is more obtuse and obscure.


> I don't get this. The magic methods in previous drafts of the RFC
> don't have a problem with & as the methods are named with 'two
> underscores' + name e.g. __bitwiseAnd. That does't appear to cause a
> problem with an ampersand?
>

This was referring to continuing to use symbol names, but without a new
keyword.

The name of the function (e.g. __add) always refers to the symbol used
> where it is used, not what it is doing.
>
> If the code is `$a + $b` then that is an addition operator, when the
> code is read. If I was reading the code, and I saw that either  $a or
> $b were objects, I would know to go looking for an __add magic method.
>

True, and I don't think it would be *ambiguous* this way. However, method
names for other methods tend to describe what the function does.


>
> " '// This function unions, it does not add'"
>
> Then that is probably an example of an inappropriate use of operator
> overloads, and so shouldn't be used as a justification for a syntax
> choice.
>

Larry's example with dot-product and cross-product is a better example, but
one that is less accessible to those who don't know vector math/linear
algebra. I used this example because this is exactly how PHP treats + for
arrays, so I would think most Collections would implement + as the union
operator in order to remain consistent with internals.


> I think this is just wrong, and makes the RFC unacceptable to me.
>
> Although most of the code I write is code that just performs
> operations as I see fit, some of the time the operations need to be
> driven by user data. Even something simple like a
> calculator-as-a-service would need to call the operations dynamically
> from user provided data.
>

I'm confused why this wouldn't still be possible? First, you can still get
the closure for the operator implementation from Reflection if you really,
really need it. But second, with user data couldn't you use a setter to
change the object state prior to the op, use a method specifically for
calling as a method, or just combine them with the operator?

`$obj->setValue($userData);`

`$result = $obj + $userData;`


>
> I also have an aesthetic preference when writing tests to be explicit
> as possible, rather than concise as possible e.g.
>
> $foo->__add(5, OperandPosition::LeftSide);
> $foo->__add(5, OperandPosition::RightSide);
>
> instead of:
>
> $foo + 5;
> 5 + $foo
>
> As I find that easier to reason about.
>
>
This I can understand. Again, I don't think you're wrong here, I think this
is a matter of opinion and taste. I can understand your position, but I
think the long-term maintainability and support the keyword offers is worth
the short term pain.

Larry:

> I largely agree here.  I don't know if it's because of the `operator`
choice or not, but being able to call an operator dynamically is important
in many use cases.  It doesn't have to be a pristine syntax, but some way
to do that dynamically (without having a big match statement everywhere you
need to) would be very welcome.

Overall, the point in making them non-callable was to force PHP developers
to stop thinking about these as methods. They are modifications to engine
behavior that are given directly to PHP devs. Using them as methods would
*usually* indicate incorrect usage. If that's what you need, then probably
you need a method, not an operator overload. If you need *both*, then what
I would suggest is implementing the logic as a normal method, and then
calling that method inside of the operator overload.

> Another question: Can an interface or abstract class require an operator
to be implemented?  That's not currently discussed at all.  (I would expect
the answer to be Yes.)

Yes. Both abstract classes and interfaces can require implementations of
the operator keyword as they would with a method.

Jordan

--000000000000edc20c05d2e3a3e8--