Add sniff for 'void' in @return tags

[dlh01]

This sniff attempts to generate an error when void appears in a PHPDoc @return tag, per the core documentation standard that "@return void should not be used outside of the default bundled themes." (The default themes would need to disable this sniff, of course.)

The code is largely based on the various FunctionCommentSniff instances in PHPCS. I welcome everyone's comments and suggestions for improvement.

[JDGrimes]

Thanks for the PR! ✨ This is looking good. There are just a few tweaks I'd like to see before this is merged. The other committers may also have some feedback.

It also looks like the Travis build is failing due to the fact that we apparently use @return void in WPCS. 😮

https://travis-ci.org/WordPress-Coding-Standards/WordPress-Coding-Standards/jobs/307321215#L1316-L1977

I think we'll probably want to go ahead and fix those as part of this PR.

[JDGrimes]

sprintf() is unnecessary here, since the message doesn't contain any placeholders.

Also, I think that perhaps the message should be simplified to just be something like @return void should not be used. I don't think it is necessary to mention the exception for the bundled themes.

Perhaps the message should include something like "null should be used instead, if needed, or the @return tag should be omitted".

[JDGrimes]

For thoroughness, it would be good to add some tests that also include a comment:

 * @return void Nothing.

[JDGrimes]

The version number will end up being 0.16.0 (0.15.0 is going to be dedicated to just renaming some things).

[JDGrimes]

A comment could be added here referencing the handbook rule that this sniff relates to.

[dlh01]

Thanks for the feedback. I've pushed some changes to address your comments.

On this point:

Perhaps the message should include something like "null should be used instead, if needed, or the @return tag should be omitted".

I used a slightly different set of messages because, as I understand it, null and void are not always interchangeable, but let me know if I've misunderstood what you were looking for.

[westonruter]

I can't think of case where null and void would not be identical: https://3v4l.org/a9stV

[GaryJones]

My understanding is, that when a method could return a something or void, that @return int|voidis allowed.

What's not allowed is the case of simply @return void.

[JDGrimes]

Maybe @DrewAPicture could weigh in on this.

[DrewAPicture]

I would agree with Gary. There are certainly functions that have echo vs return arguments where I would hesitate to document that it only returns.

So if it's not too hard to allow void only when there are multiple return types (outside of bundled themes) that would be preferable.

[GaryJones]

Seeing that next commit, I'm reminded that we've had this discussion before, and I guess the question is, should it be something like:

/**
 * @return int|null ...
 */

...rather than:

/**
 * @return int|void ...
 */

...when at least one path returns something, since the other implicitly returns null.

See https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md#examples-14 which seems to support this.

That way, @return void is the only possible usage of void (when there is no return statement at all in the function/method), and in this case, and allowed by the draft PSR-5, we say that the annotation should not be present.

There could be a separate message which checks for any use of void in a @return annotation, and indicate that it should be null when combined with other types.

To summarise:

• @return void - not allowed, even when there is no return statement.
• @return string|null - example, when one path returns and another echoes.
• @return null - only if the function explicitly return null;
• ??? - when all returns have no type: return;.

[jrfnl]

My two pennies:

Regarding multiple types: always document void if there are multiple types, otherwise the documentation is plain wrong and will confuse people.
From the handbook/inventory I made previously:

Return should list all possible return types

Regarding types: these are the allowed types as per the handbook/inventory I made previously:

Valid types: bool, int, string, float, object, array, resource, callback, void or FQ classname

Regarding null versus void: According to the above list, void should be used.
Personally, I'd allow null, but only if there is an explicit return null somewhere in the code.

??? - when all returns have no type: return;.

When the return does not have an explicit type and the possible types can not be easily determined, I'd think @return mixed should be acceptable, but only as a last resort.

As a side-note: As there are quite some more rules regarding the @return tag, I'm wondering if this should be a separate sniff or if all the rules regarding @return tags should be examined in one sniff.

On a personal note: I will disable this rule in all my projects as I think it's plain bad practice.
If there is no @return tag, you don't know whether someone has been sloppy in documenting the method or if the return is void. Thar's why void IMHO should always be documented.

On that note, I'd be in favour of excluding the sniff in the WPCS native ruleset in bin, rather than to remove the existing documentation.
After all, this project will not be using the documentation generation tools which WP uses.

[dlh01]

Thank you to everyone for your comments. I've pushed some updates that I think bring the PR in line with the requested changes:

• Rename the sniff to a FunctionCommentSniff with a process_return() method to accommodate other tests more easily. These names are intended to mimic the existing sniffs and methods in PHPCS.
• Allow void to appear with other return types. (In other words, disallow only @return void.)
• Exclude NoReturnVoid from the WPCS ruleset.

Please let me know if I've missed or misunderstood any of the feedback.

[GaryJones]

I can't think of case where null and void would not be identical: https://3v4l.org/a9stV

Extended with https://3v4l.org/AgenG (no change).

Changes have been implemented.

[jrfnl]

@dlh01 Thanks for making those changes. I'll have a more detailed look at the PR next week.

[mundschenk-at]

On a personal note: I will disable this rule in all my projects as I think it's plain bad practice.
If there is no @return tag, you don't know whether someone has been sloppy in documenting the method or if the return is void. Thar's why void IMHO should always be documented.

Also needed in interface definitions, because there is no method body to look at.