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

12 days ago by Dan Ackroyd — view source

unread

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

10 days ago by tim@bastelstu.be — view source

unread

Hi

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

10 days ago by Marco Pivetta — view source

unread

Hey Tim,

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?

8 days ago by David Gebler — view source

unread

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

6 days ago by Dan Ackroyd — view source

unread

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

5 days ago by David Gebler — view source

unread

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

4 days ago by tim@bastelstu.be — view source

unread

Hi

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

4 days ago by David Gebler — view source

unread

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

1 day ago by Dan Ackroyd — view source

unread

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.

22 hours ago by Rowan Tommins — view source

unread

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]

6 days ago by Dan Ackroyd — view source

unread

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

6 days ago by Rowan Tommins — view source

unread

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:

Are there users who will benefit from marking which methods were
intended as over-rides, and which weren't?
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]

6 days ago by Marco Pivetta — view source

unread

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

Marco Pivetta

https://mastodon.social/@ocramius

https://ocramius.github.io/

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

6 days ago by Ilija Tovilo — view source

unread

Hi Tim

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

5 days ago by Sara Golemon — view source

unread

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

4 days ago by Larry Garfield — view source

unread

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