Page MenuHomePhabricator

Do not warn about missing function doc comment when parameter and return types are fully specified by type hints
Closed, ResolvedPublic

Description

This is allowed:

/**
 * @param Bar $bar
 * @param Baz $baz
 * @return Foo
 */
public function getFoo( Bar $bar, Baz $baz ): Foo {}

This throws MediaWiki.Commenting.FunctionComment.MissingDocumentationPublic:

public function getFoo( Bar $bar, Baz $baz ): Foo {}

but the only difference is that the first one is harder to read and wastes more space.

Either we should require human-readable description for methods / parameters, or not require the doc block at all when the types are already fully specified.

See similar T288754: Don't complain about missing property documentation on typed properties

This was also discussed on wikitech-l with universal support, see https://lists.wikimedia.org/hyperkitty/list/wikitech-l@lists.wikimedia.org/thread/NWXPNHRNLEVXHSWX33H473OAWQP6CDOA/.

Event Timeline

Some thoughts:

  • array types should still have documentation for what it is an array of
  • unless all parameters and the return are documented with strong typehints, the sniff should still be run, since we shouldn't have only some parameters documented in the doc block
  • if there is no return typehint, check whether anything other that an empty return is ever used - if not, it isn't needed (though void would be allowed) so that shouldn't trigger the sniff

array types should still have documentation for what it is an array of

We accept @param array so I don't think it makes sense to reject array typehints. (In the longer term, maybe we should reject both and require explicit array typing.)

unless all parameters and the return are documented with strong typehints, the sniff should still be run, since we shouldn't have only some parameters documented in the doc block

Do you mean something specific with *strong* typehint? AFAIK PHP has only one kind of typehint. Otherwise, agreed.

if there is no return typehint, check whether anything other that an empty return is ever used - if not, it isn't needed (though void would be allowed) so that shouldn't trigger the sniff

I would keep it simple and require either a void typehint or a doc block.

array types should still have documentation for what it is an array of

unless all parameters and the return are documented with strong typehints, the sniff should still be run, since we shouldn't have only some parameters documented in the doc block

Do you mean something specific with *strong* typehint? AFAIK PHP has only one kind of typehint. Otherwise, agreed.

I use "strong typehint" to refer to the typehints within the php code, separate from typehints in the doc blocks

if there is no return typehint, check whether anything other that an empty return is ever used - if not, it isn't needed (though void would be allowed) so that shouldn't trigger the sniff

I would keep it simple and require either a void typehint or a doc block.

That works too

Either we should require human-readable description for methods / parameters, or not require the doc block at all when the types are already fully specified.

I think both should be possible, probably the latter by default and the former as an opt-in sniff (if that’s possible?). Documenting beyond types is also important, especially on public interfaces—@param documentation should describe what the parameters mean (if it’s not obvious based on the type+parameter name, like @param string $input HTML-escaped input), @return documentation should document things that cannot be expressed in types (e.g. @return string[] [raw text, HTML-escaped text]) and both should point at edge cases (e.g. @return ?string The user input if it’s HTML-safe, null otherwise). Of course this is a burden and most people are just too lazy to do it (therefore only opt-in), but if a developer/team decides to require it, let them have a tool to enforce.

I use "strong typehint" to refer to the typehints within the php code, separate from typehints in the doc blocks

PHP calls them type declarations, not hints, as they’re more than hints (if they’re violated at run time, the program aborts, unlike documentation comment hints, which have no runtime semantics).

Change 865650 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/tools/codesniffer@master] Allow fully-typed functions to omit documentation

https://gerrit.wikimedia.org/r/865650

Change 865678 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/tools/codesniffer@master] Allow prose-only documentation for statically typed functions

https://gerrit.wikimedia.org/r/865678

Change 865650 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] FunctionCommentSniff: Allow fully-typed functions to omit documentation

https://gerrit.wikimedia.org/r/865650

Change 865678 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Allow prose-only documentation for statically typed functions

https://gerrit.wikimedia.org/r/865678

I think we can close this task – the above changes should be included in the next release.

Change 868006 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/tools/codesniffer@master] Allow fully typed __constructor without @param tags

https://gerrit.wikimedia.org/r/868006

Change 868006 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Allow fully typed __constructor without @param tags

https://gerrit.wikimedia.org/r/868006

Thanks for doing this!

I think we can close this task – the above changes should be included in the next release.

Is there a task for doing the release? I’d like to stall T325200 on it.