Fix Squiz.Commenting.FunctionComment.InvalidNoReturn false positive when return type is never

[axlon]

The Squiz.Commenting.FunctionComment.InvalidNoReturn causes false positives when it encounters the never return type.

Tested this locally with:

<rule ref="Squiz.Commenting.FunctionComment.InvalidNoReturn"/>

/**
 * @return never
 */
private function foo()
{
    throw new \RuntimeException('This method never returns');
}

[jrfnl]

@axlon Thanks for this PR. Could you please add some tests to cover this change ? Let me know if you have questions on how to do this.

[axlon]

@jrfnl I managed to add the test, please let me know if anything needs changing 👍

[jrfnl]

@axlon Thanks for adding that test. All looks good!

[jrfnl]

@axlon I'm sorry, I spoke too soon. Looks like the tests are failing now. Could you please have a look ?

[axlon]

@jrfnl I made the changes you requested, hope everything is ok, could you start the workflow for tests? Currently don't have a PHP 7.0 environment available to test on.

EDIT: one thing I changed from before is that an error should now be emitted if a function has a documented return type of never but has a return statement (which already happened for void functions). I couldn't figure out how write a test for this, could you point me in the right direction?

-------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------
 182 | ERROR | Function return type is never, but function contains return statement
-------------------------------------------------------------------------------------------

[jrfnl]

one thing I changed from before is that an error should now be emitted if a function has a documented return type of never but has a return statement (which already happened for void functions). I couldn't figure out how write a test for this, could you point me in the right direction?

-------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------
 182 | ERROR | Function return type is never, but function contains return statement
-------------------------------------------------------------------------------------------

This change evolves this PR from a bug fix to an enhancement and will definitely need more tests.

It also looks like this new part is unfinished as it doesn't seem to work the way it should ?

Could you please have another look at this ?

[axlon]

@jrfnl

This change evolves this PR from a bug fix to an enhancement and will definitely need more tests.

I think you are right, it definitely became a lot larger than I intended and I will need to immerse myself a bit more into writing tests for this library. I think I will revert this to only being a bugfix for now, and will follow up with a full fledged PR later. 👍

[axlon]

@jrfnl I think the PR is ready for review now

[jrfnl]

@axlon Would you mind rebasing this PR (and possibly squashing the commits) to get it past the merge conflict ?

[axlon]

@jrfnl Done!

[jrfnl]

Thanks @axlon ! The isolated bug fix, which this PR is now (again), looks good.

Just checking: do you intend to submit a PR for the follow-up which was discussed in the comments as well ?

[axlon]

@jrfnl Thanks! Yes, I still intend to create a followup PR whenever I find the time, but if someone else picks it up before me that is fine by me as well

[jrfnl]

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).