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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
