RFC [Discussion]: Marking overridden methods (#[\Override])

Dan Ackroyd

3 years ago

On Thu, 11 May 2023 at 17:37, Tim Düsterhus <tim@bastelstu.be> wrote:

>
> Hi
>
> I'm now opening discussion for the RFC "Marking overridden methods
> (#[\Override])":
>
> I'm now opening discussion for the RFC "Marking overridden methods (#[\Override])":

This RFC is probably a good idea, even if the number of people who
will benefit from the behaviour in the engine might be quite low. My
guess is the number of people who will bother to write those
attributes, and who also won't bother to setup CI, is quite small.

But, this RFC another step forward from where we are, to having 'too
many' attributes/annotations, so I have a question.

How do we stop prevent ourselves from copying the problems that Java
has made for itself?

Or putting it another way, is it possible for us to set any guidelines
on what annotations are 'good' or 'bad' ?

I hope I am at least consistent with my concerns about annotations.

Danack wrote in https://externals.io/message/114116#114196

>
> I think I disagree with this very strongly, and plan* to vote against
> any RFC that embeds another language in annotations.**

> https://blog.softwaremill.com/the-case-against-annotations-4b2fb170ed67

cheers
Dan
Ack

Tim Düsterhus

3 years ago

Hi

On 5/16/23 18:17, Dan Ackroyd wrote:

>> I'm now opening discussion for the RFC "Marking overridden methods (#[\Override])":
>
> This RFC is probably a good idea, even if the number of people who
> will benefit from the behaviour in the engine might be quite low. My
> guess is the number of people who will bother to write those
> attributes, and who also won't bother to setup CI, is quite small.

I would expect IDEs to provide "Quick Fix" actions to add the attribute
where possible if the RFC passes. From what I'm seeing as
not-a-Java-programmer, the @Override annotation in Java appears to be
very commonly used. I certainly came across it when forced to use Java
at university.

With regard to usefulness for less-well tested and old codebases, I can
only talk from my personal experience with such a codebase.

Having the proposed attribute there would certainly have prevented some
regressions. One specific example that comes to my mind is a class that
wasn't final but also wasn't meant to be extended. During refactoring I
inlined one simple method to improve readability. However that broke a
child class that relied on that method being called to adjust the
behavior. If the child class would have had the #[\Override] attribute
on the method in question, the regression would have been immediately
apparent, even without having automated tests covering the functionality.

> But, this RFC another step forward from where we are, to having 'too
> many' attributes/annotations, so I have a question.
>
> How do we stop prevent ourselves from copying the problems that Java
> has made for itself?
>
> Or putting it another way, is it possible for us to set any guidelines
> on what annotations are 'good' or 'bad' ?

I believe I touched on that in the "Why an attribute and not a keyword?"
section, because I assumed a question like this would come up.

The attribute as-proposed would not have any runtime effect on the code
and neither would it have an effect on the public API of the code that
uses the attribute. It is purely an assistance to the reader of the code
and could be stripped without changing anything. If having the attribute
on a method would result in an error being emitted after the initial
code-and-debug cycle then the attribute is merely pointing out a
breaking change in a *different class*. This breaking change would also
exist without the attribute. It would just be less obvious.

As such I believe that on a scale from 'good' to 'bad' the attribute is
very far in the direction of the 'good' end. It is certainly more 'good'
than both AllowDynamicProperties (affects the public API) and
SensitiveParameter (affects runtime behavior). The ReturnTypeWillChange
attribute is just a hack that will go away in the future based on my
understanding, so I won't put it on the scale.

> I hope I am at least consistent with my concerns about annotations.
>
> Danack wrote in https://externals.io/message/114116#114196
>>
>> I think I disagree with this very strongly, and plan* to vote against
>> any RFC that embeds another language in annotations.**
>
>> https://blog.softwaremill.com/the-case-against-annotations-4b2fb170ed67

Thank you for the link to that article, that was an insightful read as
someone who, as said above, was forced to Java, but didn't deep-dive it
due to dislike.

I'd like to point out that the article specifically calls out the
@Override annotation as one of the "good" annotations in Java:

> That’s not to say that annotations are not useful at all. There are some great use-cases. For example, the @Override and @FunctionalInterface annotations in Java or @tailrec in Scala. These are checked at compile-time and provide actionable value to the programmer. They don’t modify how the program behaves at run-time, and they don’t required any kind of container at run-time to fully utilise the annotated class or method.

This assessment by the article's author appears to match my assessment
from above.

I also believe that #[\Override] falls into a similar category as a
#[\Memoize] you mentioned in your "one asterisk" footnote of the linked
email.

Best regards
Tim Düsterhus

Marco Pivetta

3 years ago

Hey Tim,

On Thu, 11 May 2023, 18:37 Tim Düsterhus, <tim@bastelstu.be> wrote:

> Hi
>
> I'm now opening discussion for the RFC "Marking overridden methods
> (#[\Override])":
>
> ----
>
> RFC: Marking overridden methods (#[\Override])
> https://wiki.php.net/rfc/marking_overriden_methods
>
> Proof of concept implementation is in:
>
> https://github.com/php/php-src/pull/9836

The idea is neat, but the trend is roughly:

* Many moving to `final` classes (finally!)
* Moving to less methods in an `interface` (some RFCs around functional
interfaces, even)

I am not sure this RFC is really relevant: I can see some use in generated
code, since this would act as an engine-level checksum for some minimal
changes, but otherwise it's a bit bloated?

Would it perhaps make sense to have this in userland first, in phpstan or
psalm plugins, to see if there is interest?

David Gebler

3 years ago

On Thu, May 18, 2023 at 9:12 AM Marco Pivetta <ocramius@gmail.com> wrote:

>
> Would it perhaps make sense to have this in userland first, in phpstan or
> psalm plugins, to see if there is interest?
>

100% this in my view; this is exactly the kind of check which you would
expect to be done at the static analysis stage and I don't see a benefit to
new behavior in the engine here. Allowing us as PHP users to add whatever
kind of metadata and tooling to interpret it we find useful is exactly what
user-defined attributes are for. So any attributes added in the engine
which declare some behavioural change should be limited to those able to
provide a tangible benefit which can't be achieved in userland.

-Dave

Dan Ackroyd

3 years ago

On Sat, 20 May 2023 at 18:58, David Gebler <davidgebler@gmail.com> wrote:

>
> this is exactly the kind of check which you would
> expect to be done at the static analysis stage

Even for those who use static analysis, most (afaik) don't have it
running constantly in local development and this RFC would prevent
people wondering why their code is behaving surprisingly before it is
static analysed.

Also, not everyone uses static analysis tools.

cheers
Dan
Ack

David Gebler

3 years ago

On Mon, May 22, 2023 at 1:01 PM Dan Ackroyd <Danack@basereality.com> wrote:

> Even for those who use static analysis, most (afaik) don't have it
> running constantly in local development and this RFC would prevent
> people wondering why their code is behaving surprisingly before it is
> static analysed.
>

It takes care of one possible surprise they might encounter as a result of
not running static analysis of their code prior to runtime. Just one, out
of dozens of possible types of inference which are done by static analysis
checks and could hypothetically be done by PHP at runtime. But I'd question
whether there's an appetite out there in general to start adding all sorts
of new runtime checks, which I would argue means any new runtime check
warrants the utmost consideration of cost-benefit.

> Also, not everyone uses static analysis tools.
>

That's because PHP is a dynamic language which doesn't have an explicit
compilation step and by extension where static analysis of any sort is
optional. That's my question around the benefit with this RFC; either you
use static analysis tools as part of your PHP workflow, because you care
about that stuff, or you don't. If you do, you don't need any new error or
warning emitting behaviour added to the engine itself, you just need
PHPStan / Psalm / IDE / whatever to flag it. If you don't habitually use
these tools already, you're probably not going to be annotating your code
with #[Override] anyway.

My wager is the kind of people and teams who would be most interested in
using and would benefit most from an #[Override] attribute are the kinds of
people who are using static analysis tools and probably have configured
PHPStorm / other IDE of choice to run those checks when a file changes, or
via commit hook, or build pipeline, or whatever else. What I see here,
then, is a perfect candidate for a userland attribute.

Anyway, usual disclaimer applies; I'm not a voter on RFCs, I'm one man with
an opinion, my opinion only matters insofar as gathering a range of
community feedback matters to the author and interested members of
internals.

-Dave

Tim Düsterhus

3 years ago

Hi

On 5/22/23 23:32, David Gebler wrote:

> whether there's an appetite out there in general to start adding all sorts
> of new runtime checks, which I would argue means any new runtime check
> warrants the utmost consideration of cost-benefit.

Just to make sure there is no misunderstanding I'd like to clarify that
the proposed check of this RFC is not a runtime error. It's a compile
time error, just like the check for incompatible method signatures
during inheritance. In fact it shares a non-trivial amount of the
existing logic of that check, because it's so similar.

My understanding is that once the class is successfully sitting in
Opcache there will not be any further overhead.

------

With regard to the rest of the email, I (naturally?) disagree that this
should not be part of the language itself. I currently have less time
available than expected, I'll put the email back on unread and hope to
be able to give a more expanded and nuanced reply later. I just wanted
to get the first part with regard to runtime vs. compile time out for now.

Best regards
Tim Düsterhus

David Gebler

3 years ago

On Tue, May 23, 2023 at 9:27 PM Tim Düsterhus <tim@bastelstu.be> wrote:

> Just to make sure there is no misunderstanding I'd like to clarify that
> the proposed check of this RFC is not a runtime error. It's a compile
> time error, just like the check for incompatible method signatures
> during inheritance. In fact it shares a non-trivial amount of the
> existing logic of that check, because it's so similar.
>
> My understanding is that once the class is successfully sitting in
> Opcache there will not be any further overhead.
>

I figured as much, I say runtime just to differentiate from compiled
languages with a similar feature. Although OPcache is ubiquitous in
production environments now, it's not obligated and the cache only lasts as
long as the SAPI process. It's not overhead I think is the issue, anyway -
at least not off adding "one more thing" at this stage, it's more the
precedent of is there a sufficient benefit to adding this and potentially
more features like it at the language-level.

Merely delineating intent isn't something I feel warrants a change in the
language, particularly when you'd need SAs and IDEs to implement it anyway
for users to not be caught out with a fatal error at runtime (/compile
time).
Don't get me wrong; *delineating intent is a good thing*. I entirely get
where you're coming from, however I'd refer back to my earlier comment: any
attributes added in the engine should provide a tangible benefit which
*can't be achieved in userland*.

> I'll put the email back on unread and hope to
> be able to give a more expanded and nuanced reply later.
>

Do this if you feel it's beneficial to the wider audience of internals to
better understand the motivation and justification for your proposal, but
ultimately it's not me you need to persuade so certainly don't worry about
getting into further nuances on my account.

I don't have any more feedback or thoughts for you until/unless the RFC
changes, so I'll finish by saying for me, I'd either back Marco's
suggestion of try it as a plugin for PHPStan first, or expand your proposal
more along the lines of what Sara's suggested.

Thanks.

-Dave

Tim Düsterhus

3 years ago

Hi

On 5/24/23 01:32, David Gebler wrote:

> where you're coming from, however I'd refer back to my earlier comment: any
> attributes added in the engine should provide a tangible benefit which
> *can't be achieved in userland*.
>

I think this is a flawed premise: Any sort of analysis that PHP itself
performs can also be performed in userland. IDEs and static analysis
tools already do that. In fact the probably most-requested feature for
PHP, Generics, is already implemented in existing userland static analyzers.

Ilija already explained the benefit of having the proposed #[\Override]
attribute in the language and that benefit applies to all kinds of code
analysis that could technically be (and already is) performed in userland:

It's part of the language and thus will *always* be available. If it's
not always available, then folks who switch jobs or contribute to
various open source projects that made a different choice with regard to
analyzers will possibly:

- Be unable to rely on a specific check that they found useful in the
past, because the new SA tool doesn't implement it.
- Be unable to rely on a specific check working as they learned it,
because the new SA tool implements it differently.

Basically these developers are not working with PHP, but with PHP+Psalm,
PHP+PHPStan or PHP+Phan or whatever new tool might come around or that I
forgot about and they will need to (re)learn all the
functionality/differences if a future endeavour makes a different choice.

By having it as part of the language by definition it would work
identically for everyone and it can also be documented as part of the
language. Developers will naturally come across it while learning PHP
and it might be included in tutorials and so even if they never heard
about the userland analyzers they might use it in their own code and
reap the benefits.

As the proposal is a compile time check, the mistake would also be
immediately apparent just by loading the class (e.g. as part of *any*
kind of test), it is not necessary to actually execute the method in
question. So it should be sufficiently visible and usable even in
codebases that are in a less-than-stellar state.

In short: It should not be required to combine PHP with another tool to
make it a usable language.

>> I'll put the email back on unread and hope to
>> be able to give a more expanded and nuanced reply later.
>>
>
> Do this if you feel it's beneficial to the wider audience of internals to
> better understand the motivation and justification for your proposal, but
> ultimately it's not me you need to persuade so certainly don't worry about
> getting into further nuances on my account.

Yes, I believe it is beneficial for the wider audience, because folks
might share your opinion, but not publicly speak up on the list.

> suggestion of try it as a plugin for PHPStan first

I prefer to spend my time where I believe I can make the biggest impact.
That is PHP itself and not the analyzer that I personally prefer.

Best regards
Tim Düsterhus

Rowan Collins

3 years ago

On 29/05/2023 19:29, Tim Düsterhus wrote:

> I think this is a flawed premise: Any sort of analysis that PHP itself
> performs can also be performed in userland.

This isn't actually true. There is a lot of dynamic functionality in PHP
where correctness can't be proven ahead of time, and run-time checks are
the only way. That's why Hack evolved away from being a superset of PHP
to being a related but incompatible language, and why languages like
Dart continue to include run-time type checks despite mandatory
pre-compilation analysis.

> It's part of the language and thus will *always* be available. If it's
> not always available, then folks who switch jobs or contribute to
> various open source projects that made a different choice with regard
> to analyzers will possibly:
>
> - Be unable to rely on a specific check that they found useful in the
> past, because the new SA tool doesn't implement it.
> - Be unable to rely on a specific check working as they learned it,
> because the new SA tool implements it differently.

This is a reasonable argument, but it basically comes down to PHP
Internals becoming the controller of a standardised set of static
analysis attributes, because a lot of the functionality will still only
exist in each of those tools.

I don't know how those tools currently collaborate on designing the
syntax and semantics for extra attributes or docblock annotations, but
it's not clear that this would be the right place to do it.

> As the proposal is a compile time check, the mistake would also be
> immediately apparent just by loading the class (e.g. as part of *any*
> kind of test), it is not necessary to actually execute the method in
> question. So it should be sufficiently visible and usable even in
> codebases that are in a less-than-stellar state.

The reason I'm so skeptical of this feature is that the category of
mistake the engine would be able to catch seems like it would be rather
rare. And I really do think it makes a difference that the author of the
*inheriting* class is the only one that can trigger the check.

For example: if the maintainer of Guzzle decides this check is useful,
they can add the attribute to appropriate methods in that code base.
Guzzle itself already has both PHPStan and Psalm configured with
automated CI checks, and will likely be able to configure them to warn
if an Override attribute is missing, which the engine can't do. So the
only benefit to them is standardising the attribute for use with SA tools.

Users of Guzzle won't see any impact from the Guzzle code base adding
those attributes; they would have to add them to their own code
separately. If they are not using any kind of static analysis tools,
they will not be reminded to add them. They will then only see a benefit
from remembering to add them in the rare case that a parent class
removes a method they were previously overriding.

It all seems rather a small niche for the engine to care about, unless
it's the beginning of a larger project to standardise and encourage such
attributes.

Regards,

--
Rowan Tommins
[IMSoP]

Tim Düsterhus

3 years ago

Hi

On 5/30/23 14:17, Rowan Tommins wrote:

> On 29/05/2023 19:29, Tim Düsterhus wrote:
>> I think this is a flawed premise: Any sort of analysis that PHP itself
>> performs can also be performed in userland.
>
>
> This isn't actually true. There is a lot of dynamic functionality in PHP
> where correctness can't be proven ahead of time, and run-time checks are
> the only way. That's why Hack evolved away from being a superset of PHP
> to being a related but incompatible language, and why languages like
> Dart continue to include run-time type checks despite mandatory
> pre-compilation analysis.
>

Fair enough. Main point still stands, though. LSP checks or other type
checks wouldn't technically be necessary in PHP itself. As with "duck
typing" the runtime check at the last possible moment (e.g. the actual
method call) would catch the mistake, however it would catch it much
much later, once the broken data made it halfway through the application.

These type checks are effectively (re)implemented in userland and are
even more powerful, due to the support for generics / typed arrays.
Nonetheless having them in the language provides a clear benefit,
because they are always available and thus compatible, no matter which
choices the other developer of a library made with regard to static
analysis.

>> It's part of the language and thus will *always* be available. If it's
>> not always available, then folks who switch jobs or contribute to
>> various open source projects that made a different choice with regard
>> to analyzers will possibly:
>>
>> - Be unable to rely on a specific check that they found useful in the
>> past, because the new SA tool doesn't implement it.
>> - Be unable to rely on a specific check working as they learned it,
>> because the new SA tool implements it differently.
>
>
> This is a reasonable argument, but it basically comes down to PHP
> Internals becoming the controller of a standardised set of static
> analysis attributes, because a lot of the functionality will still only
> exist in each of those tools.

In this specific case of #[\Override] the main functionality is fully
available with "just PHP itself".

The reverse check that the attribute is used wherever possible is pushed
to the userland analyzers / IDEs for now. It would be possible to
include it in PHP itself, if we can agree on a reasonable solution to
"configure it" (see my email in response to Claude).

I'm not sure if I would agree on a standardized set of attributes if PHP
itself would not be able to make use of it, because the value of
documenting it in the PHP manual would not exist. Developers would need
to download one of many third party tools to make use of it.

>> As the proposal is a compile time check, the mistake would also be
>> immediately apparent just by loading the class (e.g. as part of *any*
>> kind of test), it is not necessary to actually execute the method in
>> question. So it should be sufficiently visible and usable even in
>> codebases that are in a less-than-stellar state.
>
>
> The reason I'm so skeptical of this feature is that the category of
> mistake the engine would be able to catch seems like it would be rather
> rare. And I really do think it makes a difference that the author of the
> *inheriting* class is the only one that can trigger the check.

It's anecdata, but the attribute would have saved me debugging effort
several times in the past year - that's the whole reason I'm proposing
it. It also saved me time in other language that have the feature
already built-in.

> Users of Guzzle won't see any impact from the Guzzle code base adding
> those attributes; they would have to add them to their own code
> separately. If they are not using any kind of static analysis tools,
> they will not be reminded to add them. They will then only see a benefit
> from remembering to add them in the rare case that a parent class
> removes a method they were previously overriding.

As mentioned before with my own Java experience, I believe that having
this kind of feature in the language itself will naturally help its
adoption. It will start to appear in codebases, developers will learn
about its existence by looking at the code. It will likely also be
included in PHP tutorials targeted at developers that learn PHP.

Speaking from my own experience with languages that have this kind of
feature, it is not something that I "need to remember to add". It
becomes a natural part of creating a new method, just like adding
parameter types and return types became a natural part of it.

> It all seems rather a small niche for the engine to care about, unless
> it's the beginning of a larger project to standardise and encourage such
> attributes.

Best regards
Tim Düsterhus

Dan Ackroyd

3 years ago

On Mon, 22 May 2023 at 22:32, David Gebler <davidgebler@gmail.com> wrote:

>
> either you  use static analysis tools as part of your PHP workflow, because
> you care about that stuff, or you don't.

I these words imply an unpleasant connotation; that people who don't
use static analysis tools are bad people who don't care about their
code.

Just because someone doesn't use the same tools that 'good'
programmers who care about their code use, doesn't mean that they
'deserve' to be punished for doing not having having setup those
tools*.

> I would argue means any new runtime check
> warrants the utmost consideration of cost-benefit.

The Override annotation is a small thing that we can add to PHP, that
has a very low cost, and would benefit programmers who haven't had
enough time/experience to setup static analysis tools. It appears to
be a 'positive sum' proposal.

Or as Ilija Tovilo wrote:

>
> The benefits seem worth the maintenance cost, even if small for the
> average user.

cheers
Dan
Ack

* At least in this case, because having to deal with code that uses
inheritance is already punishment enough.

Rowan Collins

3 years ago

On 27 May 2023 11:40:44 BST, Dan Ackroyd <Danack@basereality.com> wrote:

>On Mon, 22 May 2023 at 22:32, David Gebler <davidgebler@gmail.com> wrote:
>>
>> either you  use static analysis tools as part of your PHP workflow, because
>> you care about that stuff, or you don't.
>
>I these words imply an unpleasant connotation; that people who don't
>use static analysis tools are bad people who don't care about their
>code.

That's not what was being said at all, as is clear a couple of sentences later:

> If you don't habitually use these tools already, you're probably not going to be annotating your code with #[Override] anyway.

The claim being made was not about good or bad programmers, or whether they *deserve* the benefit of the change; it was about whether it's likely that they'll *receive* the benefit of the change.

To rephrase, the proposal is for the engine to only warn about code where the attribute is present, not where it's absent, so for a user to benefit they have to edit their code to include it. If they edit their code and then run a static analyser, they don't need the attribute to be built into the engine; if they don't add it, the feature has zero benefit to them.

So the argument is that the key estimate for whether to include it in the engine is how many users will add the attribute, but not run a static analysis tool. If that number is very low, adding it to the engine has a very low value.

Regards,

--
Rowan Tommins
[IMSoP]

Tim Düsterhus

3 years ago

Hi

On 5/27/23 23:10, Rowan Tommins wrote:

> So the argument is that the key estimate for whether to include it in the engine is how many users will add the attribute, but not run a static analysis tool. If that number is very low, adding it to the engine has a very low value.

As mentioned in my "sibling email": I would not underestimate the
"advertising" power of having something included in the language and
thus by extension being included in the PHP documentation and naturally
seeping into various tutorials.

The fact that I came across Java's @Override annotation as part of my
university studies that really did not focus on "Java" per se, but
rather "programming in general" likely indicates that having something
universally available helps its adoption by be general public.

Best regards
Tim Düsterhus

Dan Ackroyd

3 years ago

On Thu, 18 May 2023 at 09:12, Marco Pivetta <ocramius@gmail.com> wrote:

>
> I am not sure this RFC is really relevant... Would it perhaps
> make sense to have this in userland first, in phpstan or psalm
> plugins, to see if there is interest?

The RFC lists other languages where an equivalent is available, and we
can see that it appears quite popular:

https://stackoverflow.com/questions/94361/when-do-you-use-javas-override-annotation-and-why
https://github.com/microsoft/TypeScript/issues/2000

In those languages, it is a recommended "defensive programming"
(https://ocramius.github.io/extremely-defensive-php/#/23) technique,
that protects against the types of mistakes that are listed in the
RFC.

So one should be able to see there is interest....unless of course
someone is so bad at basic human empathy, that they can't accept other
people's lived experiences as valid, and will only accept something as
'good programming practice' if they have invented or experienced it
themselves.

But if that's the case, there's no use trying to persuade them.

All you can do is point out that habit of not liking things that they
haven't yet used/invented themselves, and hope that other people don't
spend too much time arguing with someone who has a pattern of being
unreasonable.

cheers
Dan
Ack

Rowan Collins

3 years ago

On Mon, 22 May 2023 at 12:49, Dan Ackroyd <Danack@basereality.com> wrote:

>
> So one should be able to see there is interest....unless of course
> someone is so bad at basic human empathy, that they can't accept other
> people's lived experiences as valid, and will only accept something as
> 'good programming practice' if they have invented or experienced it
> themselves.
>

I'm not sure how you intended this to be read, but it comes across as quite
aggressive, and doesn't really move the discussion forward.

I think Dave's last sentence was much more productive:

> So any attributes added in the engine which declare some behavioural

change should be limited to those able to provide a tangible benefit which
can't be achieved in userland.

That is, we can separate two questions:

1) Are there users who will benefit from marking which methods were
intended as over-rides, and which weren't?
2) Is there a significant benefit to including it in the engine, rather
than in userland?

The answer to (1) is probably "yes", based on experience in other languages.

The answer to (2) is less clear - the other languages referenced all have
mandatory pre-compilation stages which can and do perform static analysis.

Regards,

--
Rowan Tommins
[IMSoP]

Marco Pivetta

3 years ago

Amazing wording Dan: great way to drive people away. YIKES.

Marco Pivetta

https://mastodon.social/@ocramius

https://ocramius.github.io/

On Mon, 22 May 2023 at 13:49, Dan Ackroyd <Danack@basereality.com> wrote:

Ilija Tovilo

3 years ago

Hi Tim

On Thu, May 11, 2023 at 6:37 PM Tim Düsterhus <tim@bastelstu.be> wrote:

> I'm now opening discussion for the RFC "Marking overridden methods
> (#[\Override])":
>
> RFC: Marking overridden methods (#[\Override])
> https://wiki.php.net/rfc/marking_overriden_methods

We've already talked in private, but let me state my position here as well.

The implementation is quite simple (~60 lines of non-whitespace,
non-generated C code), and does not introduce any new syntax that
parsers/static analyzers are *forced* to handle.

The RFC shows that there is a benefit for code using the attribute,
namely showing intent to the reader, reducing the risk of typos and
being more resistant to errors when refactoring / upgrading library
versions. Having the feature supported by the engine, while not
strictly necessary, allows users who don't use static analyzers to
profit from it, and define consistent semantics for static analyzers
to follow.

The benefits seem worth the maintenance cost, even if small for the
average user.

Ilija

Sara Golemon

3 years ago

On Thu, May 11, 2023 at 11:37 AM Tim Düsterhus <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

Larry Garfield

3 years ago

On Tue, May 23, 2023, at 10:47 AM, Sara Golemon wrote:

> On Thu, May 11, 2023 at 11:37 AM Tim Düsterhus <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".

I've been luke-warm on this RFC so far.  I can sorta see the use case, but I can also see the argument for "meh, the SA tools can already do this, and those who would bother using it are probably already using SA tools."

Sara's suggestion of using it for other operations, such as "yes, I'm violating LSP, trust me, I know what I'm doing" has me very interested.  That has a number of potential uses that SA tools would not be sufficient for.

The first is testing, which is the example Sara gave.

The second is when you have a wide type in an interface method parameter, but in a given child class you know with certainty that the type is going to be some subset of that.  It's not typical, but I have a few cases where I've needed that.  Narrower docblock types seem to work to keep PHPStan happy, but having a more directly-supported in the language option would be nice.

Making an attribute that can *control* inheritance rather than just *flag* inheritance seems like it would be much more compelling.

--Larry Garfield

Tim Düsterhus

3 years ago

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

Claude Pache

3 years ago

> Le 11 mai 2023 à 18:37, Tim Düsterhus <tim@bastelstu.be> a écrit :
>
> Hi
>
> I'm now opening discussion for the RFC "Marking overridden methods (#[\Override])":
>
> ----
>
> RFC: Marking overridden methods (#[\Override])
> https://wiki.php.net/rfc/marking_overriden_methods
>

Hi Tim,

One weakness of the proposal, is that there is no notice when a method without #[\Override] annotation accidentally overrides a parent method. This is necessary for the sake of BC, of course. Therefore, (inspired by the --noImplicitOverride flag of TypeScript), I suggest adding a complementary #[\NoImplicitOverride] annotation on the derived class, that makes #[\Override] mandatory on overriding methods. The following example would then trigger a compilation error since Laravel 5.4:

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Support\Facades\Http;

#[\NoImplicitOverride] // <====== HERE
class RssFeed extends Model {
/* Laravel 5.4 added the refresh() method to Eloquent, but we already
* have a custom method with the same name and signature that does
* something entirely different.
*/
public function refresh()
{
$this->message = Http::get($this->url);
$this->save();
}
}

?>

―Claude

Tim Düsterhus

3 years ago

Hi

On 5/29/23 21:34, Claude Pache wrote:

> One weakness of the proposal, is that there is no notice when a method without #[\Override] annotation accidentally overrides a parent method. This is necessary for the sake of BC, of course. Therefore, (inspired by the --noImplicitOverride flag of TypeScript), I suggest adding a complementary #[\NoImplicitOverride] annotation on the derived class, that makes #[\Override] mandatory on overriding methods. The following example would then trigger a compilation error since Laravel 5.4:
>

Thank you for that suggestion. As a solution to that problem only a
per-method attribute came to my mind and that clearly does not scale /
adds too much visual noise for a very common case.

However I'm not sure if a class-level attribute is the right choice
either. Contrary to #[\Override] itself, which clearly adds metadata
(namely the "intent") to a single method, the #[\NoImplicitOverride] is
more of a config flag for the engine behavior, instead of "metadata". It
doesn't really belong on the class, the class just happens to be a
convenient location.

The `declare()` construct however is intended to hold this kind of
behavior directives. So `declare(no_implicit_override=1)`, similarly to
`declare(strict_types=1)` appears to be the natural solution here. It's
not particularly pretty, but would be reasonably consistent with the
rest of the language. It would be less forward-compatible (unknown
declares emit a Warning), though. I'm also not sure about the impact on
the implementation. The support for "no implicit override" would require
at least another bit-flag for only a smaller incremental improvement
over #[\Override] itself.

Best regards
Tim Düsterhus

Niels Dossche

3 years ago

Hi Tim

On 5/11/23 18:37, Tim Düsterhus wrote:

> Hi
>
> I'm now opening discussion for the RFC "Marking overridden methods (#[\Override])":
>
> ----
>
> RFC: Marking overridden methods (#[\Override])
> https://wiki.php.net/rfc/marking_overriden_methods
>
> Proof of concept implementation is in:
>
> https://github.com/php/php-src/pull/9836
>
> ----
>
> Thanks to Ilija Tovilo for reviewing the implementation, providing additional test cases and proof-reading the RFC text. Also thanks to my coworkers at WoltLab for proof-reading the RFC text as well. Any remaining mistakes [1] are my fault :-)
>
> Best regards
> Tim Düsterhus
>
> [1] For example the typo in the URL that I noticed too late.
>

I support this RFC. Thank you for working on this.
The implementation also looks quite small and not intrusive in the engine.

I see some people sharing feelings that this perhaps belongs more into static analysis tools instead.
In my opinion, static analysis tools are used to catch mistakes the language can't protect you against (*).
In the case of this attribute, the language could very well protect the user so I think it makes sense to add it to the language.
Furthermore, implementing it in the language makes it universal.

(*) at least, without sacrifices

Kind regards
Niels

Tim Düsterhus

3 years ago

Hi

On 5/11/23 18:37, Tim Düsterhus wrote:

> I'm now opening discussion for the RFC "Marking overridden methods
> (#[\Override])":
>
> ----
>
> RFC: Marking overridden methods (#[\Override])
> https://wiki.php.net/rfc/marking_overriden_methods
>
> Proof of concept implementation is in:
>
> https://github.com/php/php-src/pull/9836

Discussion has died down, unfortunately without (much) commentary with
regard to Claude's and Sara's suggestions. However I believe that the
proposal is in a state where is brings a real benefit and where it is
also extensible in the future, e.g. to include Claude's "No implicit
override" suggestion. I have included it in the "Future Scope" section.
Sara's first proposal to make it a LSP check bypass I have added to the
"rejected features" section, as argued in my previous response to it.

I've also added references for Swift and Kotlin, as these languages
where mentioned in PHP Core Roundup #13 by The PHP Foundation.

I plan to start the vote later this week, possibly on Wednesday.

Best regards
Tim Düsterhus