I uploaded some Gerrit changes for the code sniffer: chain starts at change 865650 <https://gerrit.wikimedia.org/r/c/mediawiki/tools/codesniffer/+/865650>. Reviews welcome!
With those changes combined, *fully* typed functions (all parameters + return type) and typed properties are allowed (but not required, of course) to have no doc comment, or to have doc comments without @param / @return. However, partial @param are not allowed, since I think that would be too confusing – if some parameter can’t have a static type yet,¹ then the phpdoc has to list all the params (with their types). ¹: incidentally, PHP 8.2 was released today <https://www.php.net/archive/2022.php#2022-12-08-1> (h/t Reedy), with support for the types true, false, and null. Am Mi., 16. Nov. 2022 um 17:17 Uhr schrieb Lucas Werkmeister < lucas.werkmeis...@wikimedia.de>: > What Timo writes sounds good to me. I don’t think we need to enforce > removal of existing code blocks – that can happen gradually as the code is > touched for other reasons instead. > > Regarding performance, I think we should limit micro-optimizations like > omitting static types to code that we know is hot; for most of our code, I > think the benefit of avoiding bugs that can be caught or prevented by > static types is more important. We already have some hot code that uses a > more performance-oriented code style (e.g. RemexHtml “sometimes use[s] > direct member access instead of going through accessors, and manually > inline[s] some performance-sensitive code”), but we don’t use that code > style everywhere. > > Aside: I would also hope that PHP will eventually learn some optimizations > that are taken for granted in many other languages, such as inlining > setters/getters and other short methods, or (more relevant to this thread) > eliminating runtime type checks when they can be statically proven. But I > definitely don’t have the skills to push that forward, so I won’t complain > about it too much. > > I’ll try to find some time soon-ish to look into MediaWiki CodeSniffer and > see if some improvements can be implemented without too much trouble. > > > Am Mi., 16. Nov. 2022 um 01:00 Uhr schrieb Krinkle <krin...@fastmail.com>: > >> >> >> 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@lists.wikimedia.org >> To unsubscribe send an email to wikitech-l-le...@lists.wikimedia.org >> >> https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/ >> > > > -- > 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. > -- 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@lists.wikimedia.org To unsubscribe send an email to wikitech-l-le...@lists.wikimedia.org https://lists.wikimedia.org/postorius/lists/wikitech-l.lists.wikimedia.org/
