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