Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120453
Message-ID: <b2c0a7ab-71bd-a799-495e-9a6e1e84c25f@gmail.com>
Date: Tue, 30 May 2023 13:17:03 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
To: internals@lists.php.net
References: <e58dde83-654d-b0ad-de77-d0450a56ef75@bastelstu.be>
 <CADyq6sJ3aaipfagKtoqksOm2JFp-UN5H0hkrOndgKad2W9x3fg@mail.gmail.com>
 <CA+p1FabKB1K2P9ZidyNgva+mfe1xzTtracyKkQGFKeWYcW1+_Q@mail.gmail.com>
 <CA+kxMuRse5=y_+XKk1LyQ3MpkLan75DzbE=sxtmeiaZf7W0yLQ@mail.gmail.com>
 <CA+p1FaZmagr9Js_a2wp2SOrbhb-uZAhcknXKRLQHonw45gBBOg@mail.gmail.com>
 <c2ea6b97-5ecd-ca12-c748-f0b5e37402c4@bastelstu.be>
 <CA+p1FaYpnRGwG35=a3X8eBGoONvJkyLiqFgcGr+BV0_XMM61pg@mail.gmail.com>
 <c8dcba49-2f76-1812-a9af-7930662e3b5b@bastelstu.be>
Content-Language: en-GB
In-Reply-To: <c8dcba49-2f76-1812-a9af-7930662e3b5b@bastelstu.be>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] RFC [Discussion]: Marking overridden methods
 (#[\Override])
From: rowan.collins@gmail.com (Rowan Tommins)

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]