We have already disabled some of these rules for new code in the Translate extension (ref <https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/Translate/+/refs/heads/master/.phpcs.xml#27> )

I can tell

I agree, a lot of the doc comments become redundant when types are specified in type hints. Note that doc comments for properties are actually not required, as far as I can tell – you could replace: /** @var LinkRenderer */ private $linkRenderer; …with: private LinkRenderer $linkRenderer; …today!, and no lint checks are going to stop you. I've been suggesting this for new code since we moved to PHP 7.4, which allows these type hints on properties. -- Bartosz Dziewoński _______________________________________________ Wikitech-l mailing list -- wikitech-l(a)lists.wikimedia.org To unsubscribe send an email to wikitech-l-leave(a)lists.wikimedia.org https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/

Thank you for suggesting this! I agree that type declaration is preferable to type documentation, and that type documentation is often redundant if type declaration is present. However, we can't always use type declarations. For instance, union types are quite useful, since PHP doesn't have method overloading. And union type declarations will only become available in PHP 8. So we'll have a mix of declared and un-declared parameters and fields for a while. I think we should still require type documentation if there is no type declaration - and of course, if a method has any @param tags, it needs to have all of them. Also there is the notable exception of the array type. Saying that something is an array is generally insufficient, we should say at least whether it's a list or an associative array, and document the type of the array elements or and/or well-known keys. And we should be careful that we don't end up discouraging documentation of the meaning of a parameter. The barrier to adding some explanation of the meaning of a parameter is lower if there is already a @param string $name line. If I'd first have to create a doc block, I may just not add the documentation at all. We should still encourage having doc blocks in all but the most trivial cases (simple constructors, getters and setters probably don't need one). -- daniel PS: I'm not sure I like constructor argument property promotion... For very simple value objects that might be nice, but generally, I fear it will make it harder to see all fields declared on an object. Am 28.10.2022 um 16:03 schrieb Lucas Werkmeister: Hi all! In my opinion, MediaWiki’s PHPCS ruleset feels largely rooted in an older version of PHP, where static type declarations (formerly known as “type hints”) did not exist. As we move towards more modern code, I think some rules should be relaxed, and others adjusted. More specifically, I’d like to know if most people agree with the following propositions and conclusion: Proposition 1: *Some code is sufficiently documented by names and types*, and does not require additional documentation. Cases where additional documentation is required do certainly exist, but they can only be identified by human reviewers, not by automated tools. You can see this in our existing code wherever a doc comment specifies only a type (with @var, @param, or @return), but no additional text. For example, in CreditsAction <https://gerrit.wikimedia.org/g/mediawiki/core/+/de752f45af/includes/actions/CreditsAction.php>, nobody needs to be told that the LinkRenderer will be used to render links, or that the UserFactory creates User objects: class CreditsAction extends FormlessAction { /** @var LinkRenderer */ private $linkRenderer; /** @var UserFactory */ private $userFactory; Likewise, it’s not necessary to explain in great detail that the string returned by LinksTable::getTableName() <https://gerrit.wikimedia.org/g/mediawiki/core/+/de752f45af/includes/deferred/LinksUpdate/LinksTable.php#175> is the table name, that the $actor parameter of ActorCache::remove( UserIdentity $actor ) <https://gerrit.wikimedia.org/g/mediawiki/core/+/de752f45af/includes/user/ActorCache.php#103> represents the actor to remove from the cache, or what the meaning of the Message $m and returned MessageValue are in Message\Converter::convertMessage() <https://gerrit.wikimedia.org/g/mediawiki/core/+/de752f45af/includes/Message/Converter.php#48> : /** * Convert a Message to a MessageValue * @param Message $m * @return MessageValue */ public function convertMessage( Message $m ) { (I want to clarify that in this last example I’m only talking about the @param and @return tags that already don’t have any prose text. While the method comment “Convert a Message to a MessageValue” might also be considered redundant, I think this would be more contentious, and I’m not advocating for removing that today.) Proposition 2: *Adding types as static types is generally preferable.* Unlike doc comments, static types are checked at runtime and thus guaranteed to be correct (as long as the code runs at all); the small runtime cost should be partially offset by performance improvements in newer PHP versions, and otherwise considered to be worth it. New code should generally include static types where possible, and existing code may have static types added as part of other work on it. I believe this describes our current development practice as MediaWiki developers. Note that some older MediaWiki classes are considered unsuitable for static types, and should only be used in comments; this is expected to help in a future migration away from these classes (see T240307#6191788 <https://phabricator.wikimedia.org/T240307#6191788>). Proposition 3: *Where types can be losslessly represented as PHP static types, types in doc comments are unnecessary.* When doc comments are considered necessary for actual documentation beyond types, then the type is generally still included (and Phan will check that it matches the static type), but when no further documentation is needed (see proposition 1 above), then the @var, @param, etc. doc comment can be omitted. Note that depending on the PHP version, not all types can be losslessly represented as PHP static types yet (e.g. union types and mixed both need to wait for PHP 8.0, null and false for PHP 8.2); in such cases, doc comments can remain necessary. Conclusion: *We should update our PHPCS ruleset to require fewer doc comments.* Exact rules are probably to be decided, depending on how much work we’re willing to put into the sniff implementations (e.g. is it feasible to require /** @param */ doc comments only if a parameter has no static type?), but generally, I argue that we want code such as the following to be allowed by our standard PHPCS ruleset: class CreditsAction extends FormlessAction { private LinkRenderer $linkRenderer; private UserFactory $userFactory; /** Convert a Message to a MessageValue */ public function convertMessage( Message $m ): MessageValue { When doc comments are still necessary or at least beneficial because the type alone isn’t enough information, it’s up to humans to decide this while writing the code or point it out during code review. What do people think about this? :) PS: In PHP 8, we could abbreviate some of this code even more using constructor property promotion: class CreditsAction extends FormlessAction { public function __construct( Page $page, IContextSource $context, private LinkRenderer $linkRenderer, private UserFactory $userFactory ) { parent::__construct( $page, $context ); } (Again, I’m not saying that all code should look like this – but I think we have plenty of existing code that effectively carries no additional information in its documentation, and which could be converted into this form without losing anything.) Cheers, Lucas -- Lucas Werkmeister (he/er) Software Engineer Wikimedia Deutschland e. V. | Tempelhofer Ufer 23-24 | 10963 Berlin Phone: +49 (0)30-577 11 62-0 https://wikimedia.de Imagine a world in which every single human being can freely share in the sum of all knowledge. Help us to achieve our vision! https://spenden.wikimedia.de Wikimedia Deutschland - Gesellschaft zur Förderung Freien Wissens e. V. Eingetragen im Vereinsregister des Amtsgerichts Berlin-Charlottenburg unter der Nummer 23855 B. Als gemeinnützig anerkannt durch das Finanzamt für Körperschaften I Berlin, Steuernummer 27/029/42207. _______________________________________________ Wikitech-l mailing list -- wikitech-l(a)lists.wikimedia.org To unsubscribe send an email to wikitech-l-leave@lists.wikimedia.orghttps://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/ -- Daniel Kinzler Principal Software Engineer, Platform Engineering Wikimedia Foundation _______________________________________________ Wikitech-l mailing list -- wikitech-l(a)lists.wikimedia.org To unsubscribe send an email to wikitech-l-leave(a)lists.wikimedia.org https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/

Also there is the notable exception of the array type. Saying that something is an array is generally insufficient, we should say at least whether it’s a list or an associative array, and document the type of the array elements or and/or well-known keys.

And we should be careful that we don’t end up discouraging documentation of the meaning of a parameter. The barrier to adding some explanation of the meaning of a parameter is lower if there is already a @param string $name line. If I’d first have to create a doc block, I may just not add the documentation at all. We should still encourage having doc blocks in all but the most trivial cases (simple constructors, getters and setters probably don’t need one).

– daniel PS: I’m not sure I like constructor argument property promotion… For very simple value objects that might be nice, but generally, I fear it will make it harder to see all fields declared on an object. Am 28.10.2022 um 16:03 schrieb Lucas Werkmeister: Hi all! In my opinion, MediaWiki’s PHPCS ruleset feels largely rooted in an older version of PHP, where static type declarations (formerly known as “type hints”) did not exist. As we move towards more modern code, I think some rules should be relaxed, and others adjusted. More specifically, I’d like to know if most people agree with the following propositions and conclusion: Proposition 1: Some code is sufficiently documented by names and types, and does not require additional documentation. Cases where additional documentation is required do certainly exist, but they can only be identified by human reviewers, not by automated tools. You can see this in our existing code wherever a doc comment specifies only a type (with @var, @param, or @return), but no additional text. For example, in CreditsAction, nobody needs to be told that the LinkRenderer will be used to render links, or that the UserFactory creates User objects: class CreditsAction extends FormlessAction { /** @var LinkRenderer */ private $linkRenderer; /** @var UserFactory */ private $userFactory; Likewise, it’s not necessary to explain in great detail that the string returned by LinksTable::getTableName() is the table name, that the $actor parameter of ActorCache::remove( UserIdentity $actor ) represents the actor to remove from the cache, or what the meaning of the Message $m and returned MessageValue are in Message\Converter::convertMessage(): /** * Convert a Message to a MessageValue * @param Message $m * @return MessageValue */ public function convertMessage( Message $m ) { (I want to clarify that in this last example I’m only talking about the @param and @return tags that already don’t have any prose text. While the method comment “Convert a Message to a MessageValue” might also be considered redundant, I think this would be more contentious, and I’m not advocating for removing that today.) Proposition 2: Adding types as static types is generally preferable. Unlike doc comments, static types are checked at runtime and thus guaranteed to be correct (as long as the code runs at all); the small runtime cost should be partially offset by performance improvements in newer PHP versions, and otherwise considered to be worth it. New code should generally include static types where possible, and existing code may have static types added as part of other work on it. I believe this describes our current development practice as MediaWiki developers. Note that some older MediaWiki classes are considered unsuitable for static types, and should only be used in comments; this is expected to help in a future migration away from these classes (see T240307#6191788). Proposition 3: Where types can be losslessly represented as PHP static types, types in doc comments are unnecessary. When doc comments are considered necessary for actual documentation beyond types, then the type is generally still included (and Phan will check that it matches the static type), but when no further documentation is needed (see proposition 1 above), then the @var, @param, etc. doc comment can be omitted. Note that depending on the PHP version, not all types can be losslessly represented as PHP static types yet (e.g. union types and mixed both need to wait for PHP 8.0, null and false for PHP 8.2); in such cases, doc comments can remain necessary. Conclusion: We should update our PHPCS ruleset to require fewer doc comments. Exact rules are probably to be decided, depending on how much work we’re willing to put into the sniff implementations (e.g. is it feasible to require /** @param */ doc comments only if a parameter has no static type?), but generally, I argue that we want code such as the following to be allowed by our standard PHPCS ruleset: class CreditsAction extends FormlessAction { private LinkRenderer $linkRenderer; private UserFactory $userFactory; /** Convert a Message to a MessageValue */ public function convertMessage( Message $m ): MessageValue { When doc comments are still necessary or at least beneficial because the type alone isn’t enough information, it’s up to humans to decide this while writing the code or point it out during code review. What do people think about this? :) PS: In PHP 8, we could abbreviate some of this code even more using constructor property promotion: class CreditsAction extends FormlessAction { public function __construct( Page $page, IContextSource $context, private LinkRenderer $linkRenderer, private UserFactory $userFactory ) { parent::__construct( $page, $context ); } (Again, I’m not saying that all code should look like this – but I think we have plenty of existing code that effectively carries no additional information in its documentation, and which could be converted into this form without losing anything.) Cheers, Lucas – Lucas Werkmeister (he/er) Software Engineer Wikimedia Deutschland e. V. | Tempelhofer Ufer 23-24 | 10963 Berlin Phone: +49 (0)30-577 11 62-0 <https://wikimedia.de> Imagine a world in which every single human being can freely share in the sum of all knowledge. Help us to achieve our vision! <https://spenden.wikimedia.de> Wikimedia Deutschland - Gesellschaft zur Förderung Freien Wissens e. V. Eingetragen im Vereinsregister des Amtsgerichts Berlin-Charlottenburg unter der Nummer 23855 B. Als gemeinnützig anerkannt durch das Finanzamt für Körperschaften I Berlin, Steuernummer 27/029/42207. _______________________________________________ Wikitech-l mailing list – wikitech-l(a)lists.wikimedia.org To unsubscribe send an email to wikitech-l-leave(a)lists.wikimedia.org <https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/>

Clutter, because it's redundant to add a return type declaration when the return type is already in the doc comment. If we stop requiring doc comments as you propose, then fine, add a return type declaration to methods with no doc comment. But if there is a doc comment, an additional return type declaration just pads out the file for no reason.

The performance impact is measurable for hot functions. In gerrit 820244 <https://gerrit.wikimedia.org/r/c/mediawiki/core/+/820244> I removed parameter type declarations from a private method for a benchmark improvement of 2%.

On Tue, 15 Nov 2022, at 11:41, Daniel Kinzler wrote: Am 10.11.2022 um 03:08 schrieb Tim Starling: Clutter, because it's redundant to add a return type declaration when the return type is already in the doc comment. If we stop requiring doc comments as you propose, then fine, add a return type declaration to methods with no doc comment. But if there is a doc comment, an additional return type declaration just pads out the file for no reason. I agree that we shouldn't have redundant doc tags and return type declarations. I would suggest that all methods should have a return type declaration, but should not have a @return doc tag unless there is additional info […] The performance impact is measurable for hot functions. In gerrit 820244 <https://gerrit.wikimedia.org/r/c/mediawiki/core/+/820244> I removed parameter type declarations from a private method for a benchmark improvement of 2%. This raises an interesting issue, one that has bitten me before: How do we know that a given method is "hot"? Maybe we should establish a @hot or @performance tag to indicate that a given method should be optimized for speed. […] I think the enforced and automated codesniffer could remain fairly simple: As today, the sniff encourages all methods to have parameter and return types documented in a way that humans, Phan, and IDEs can understand for static analysis to avoid and catch mistakes. What I propose we change is that instead of enforcing this solely through a mandatory doc comment, enforce it by requiring at least one of them to be present. Either parameters and returns are typed, or a doc block exists. Both may exist, of course. We've established in this email thread that it can be cluttering (and waste of effort) to require repeating of information when doing so adds no value. It is also my understanding that Phan and IDEs already understand either and both so we don't need them to be aware of which "should" exist. Is there value in enforcing removal of existing doc blocks after someone has written it? This seems to me like potentially a significant time sink with no return on that other because we enforced it as a new rule. If we agree there is no urgency in removing existing doc blocks or actively blocking CI when someone choose to write a doc block, then afaik we do not need new annotations like "hot" or "performance" or some other tag to surpress warnings about doc blocks. I do think it is important to preserve author intent when it comes to performance optimisations. However these are by no means limited to this new notion of saving native type overhead. There are all sorts of code optimisations. I believe we typically document these through an inline comment like "Optimization: ..." next to the code in question, in which the need for optimisation and sometimes (if non-obvious) how that optimisation is achieved, are mentioend. That should suffice I think in preserving the use case and e.g. prevent someone from re-introducing typing where it was previously removed for perf reasons. In other words: Codesniffer helps us avoid unknown types (in docblock and/or native type), and inline comments remind us about past performance optimisations. Do we need more? If so, what is the benefit/usecase for more? What do we risk if we don't? -- Timo _______________________________________________ Wikitech-l mailing list -- wikitech-l(a)lists.wikimedia.org To unsubscribe send an email to wikitech-l-leave(a)lists.wikimedia.org https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/