Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120436
Message-ID: <a3eeebc9-4336-bdf0-4659-af1b4ba2d317@bastelstu.be>
Date: Mon, 29 May 2023 21:09:07 +0200
MIME-Version: 1.0
Content-Language: en-US
To: Sara Golemon <pollita@php.net>
Cc: PHP internals <internals@lists.php.net>
References: <e58dde83-654d-b0ad-de77-d0450a56ef75@bastelstu.be>
 <CAESVnVrs0hV4C_NqZvkJ+V_M7UOQDPSpqkaBvuxhx6=7L0ViVg@mail.gmail.com>
In-Reply-To: <CAESVnVrs0hV4C_NqZvkJ+V_M7UOQDPSpqkaBvuxhx6=7L0ViVg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods
 (#[\Override])
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)

Hi

On 5/23/23 17:47, Sara Golemon wrote:
> I think targeting 8.3 is aggressive as we're less than a month from FF
> (accounting for discussion and voting period).

I didn't expect the proposal to need much of a discussion, as the 
functionality is known from existing programming languages and the 
proposed semantics are the direct result of how the existing LSP checks 
work.

> 
> The first argument (about not impacting callers) for "why an attribute and
> not a keyword" feels like it's tying itself into a knot to make a specious
> point.

To be clear: That is intended as a real argument that I gave some 
thought. The fact that it does not affect users of the method in 
question differs from the other keywords that are part of the method 
signature and thus I find it useful to have it visually separate.

The visibility decides who can call the method, abstract/final add 
restrictions for classes extending the class in question and 
static/non-static decides how the method is called.

The proposed override marker does nothing like that.

> 
> // Intentional LSP violations
> class A {
>    public function foo(): \SimpleXMLElement { return
> simplexml_load_file("/tmp/file.xml"); }
> }
> class TestA extends A {
>    #[\Override(Override::IGNORE_RETURN_TYPE_VIOLATION)]
>    public function foo(): TestProxyClass { return TestProxy(parent::foo()); }
> }
> 
> LSP checks are super valuable for writing clean and well debuggable code,
> but sometimes they get in the way of mocking or some other non-production
> activity.  This could provide a "get out of jail free card" for those
> circumstances where you just want to tell the engine, "Shut up, I know what
> I'm doing".
> 

This is only tangentially to what my proposal intends to achieve and 
likely needs an entire discussion of its own. I believe it should be a 
separate thing, similarly to the #[\ReturnTypeWillChange] attribute.

> 
> // Specific parent override
> class A {
>    public function foo() { return 1; }
> }
> class B extends A {
>    // Not present in older versions of library, just added by maintainers.
>    public function foo() { return bar(); }
> }
> class C extends B {
>    // Errors because we're now overriding B::foo(), not A::foo().
>    #[\Override(A::class)]
>    public function foo() { return parent::foo() + 1; }
> }
> 
> C was written at a time before B::foo() was implemented and makes
> assumptions about its behavior.  Then B adds their of foo() which breaks
> those assumptions.  C gets to know about this more quickly because the
> upgrade breaks those assumptions.  C should only use this subfeature in
> places where the inheritance hierarchy matters (such as intentional LSP
> violations).
> 

This is something I could get behind. In fact one of the "complaints" 
that I read about Java's @Override is that it does not distinguish 
between a parent class and an interface.

However the semantics are much less clear, here. What about multiple 
interfaces that (intentionally) define the same method or a parent class 
+ an interface that both define the method?

The parameter would likely need to be an array and emit an error if any 
class provides the method that is *not* listed, as well as if a class is 
listed that does not provide the method.

However this likely gets a little funky if the method in your example 
was initially implemented in B and later added to A, because then A is 
not listed, but nothing changed about B which is the direct ancestor.

Interestingly this would also allow to handle the case of "this method 
is not overriding anything" by using `#[\Override([])]`.

I'd probably leave this as possible future scope. A new `?array $classes 
= null` (`class-string[]|null`) parameter could be added in the future 
without breaking anything. In any case I would need assistance with the 
implementation or someone else to implement that, because the added 
complexity is outside of what I'm comfortable with doing myself.

Best regards
Tim Düsterhus