Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120391
MIME-Version: 1.0
References: <e58dde83-654d-b0ad-de77-d0450a56ef75@bastelstu.be>
In-Reply-To: <e58dde83-654d-b0ad-de77-d0450a56ef75@bastelstu.be>
Date: Tue, 23 May 2023 10:47:44 -0500
Message-ID: <CAESVnVrs0hV4C_NqZvkJ+V_M7UOQDPSpqkaBvuxhx6=7L0ViVg@mail.gmail.com>
To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000008ead7805fc5e4f05"
Subject: Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods (#[\Override])
From: pollita@php.net (Sara Golemon)

--0000000000008ead7805fc5e4f05
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 11, 2023 at 11:37=E2=80=AFAM Tim D=C3=BCsterhus <tim@bastelstu.=
be> wrote:
>
> I'm now opening discussion for the RFC "Marking overridden methods
> (#[\Override])":
>

I 100% get the intent behind this RFC, and as someone who's used this in
multiple other languages the benefit to defensive coding is obvious.

Thoughts:

I think targeting 8.3 is aggressive as we're less than a month from FF
(accounting for discussion and voting period).


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.  The second argument about FC is more defensible, though I
personally think it's not merited in this particular case.  I'd personally
rather have a keyword here.


If you do go with an Attribute, then I'd go ahead and go further by
allowing to specify the name of the class being overridden and possibly
flags about intentional LSP violations.  For examples:


// 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".



// 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).


Just my initial thoughts, overall +1 on your proposal.

-Sara

--0000000000008ead7805fc5e4f05--