Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods (#[\Override])
On Tue, May 23, 2023 at 9:27 PM Tim Düsterhus 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
Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods (#[\Override])
On Tue, May 23, 2023, at 10:47 AM, Sara Golemon wrote:
> On Thu, May 11, 2023 at 11:37 AM Tim Düsterhus 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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods (#[\Override])
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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [VOTE] Use exceptions by default in SQLite3 extension
Voting has now ended with 21 votes for the "yes", and zero votes for the "no". Thanks everyone! -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods (#[\Override])
On Thu, May 11, 2023 at 11:37 AM Tim Düsterhus 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
