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

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

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

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

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

Hi

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

Hi

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