Handle `@param` in docblock for variables passed by reference #3813

fredden · 2023-04-28T10:00:38Z

This started as an auto-fix to remove ampersands from @param tags in docblocks within Squiz.Commenting.FunctionComment. This has evolved into "handle @param tags for variables passed by reference." We are also increasing test coverage of related-but-not-changed behaviour.

jrfnl

I'm not sure what would be the right approach here.

Fixing it like you do is one way, but is the & there actually wrong ?

An alternative way of looking at it would be to ltrim() a potential & from a param name before doing the name compare.

Not sure what is best. Maybe PSR19 can give some guidance ?

fredden · 2023-06-07T13:49:29Z

PSR-19 makes no mention of variables passed by reference. Based on this, I'll update this pull request to instead ignore this character and not auto-fix.

jrfnl

@fredden Thanks for updating this PR! Reviewed in detail now.

While this is not a PSR-19 sniff (but a Squiz one), I agree that taking guidance from PSR-19 for how to handle this is the right call. Not sure how prominent the mention of PSR-19 in the inline comments need to be though considering this is not a PSR-19 sniff.

Regarding the if/else:

The "passed by ref + has leading & in param tag" case is now special cased, which solves the original issue reported. ✅
The "has no leading & in param tag" case is also handled correctly, whether passed by ref or not. ✅
I'm just wondering about the "not passed by ref but has leading & in param tag" case. ❓

I agree that flagging a "not passed by ref but has leading & in param tag" situation is a good thing as there is clearly a mismatch. I also agree that auto-fixing this may be risking.
I'm mostly just wondering whether this should get it's own error with error code as, as things are, the following errors are thrown:

 1050 | ERROR | [ ] Doc comment for parameter "$qux" missing (Squiz.Commenting.FunctionComment.MissingParamTag)
 1062 | ERROR | [ ] Doc comment for parameter &$qux does not match actual variable name $qux (Squiz.Commenting.FunctionComment.ParamNameNoMatch)

The first of which - IMO - is incorrect as the doc comment for $qux is not missing, it just has a leading &.
The second of which is unclear/imprecise - the variable name does in actual fact match - as it's not the actual variable name which is incorrect, but the leading ampersand.

Code style nitpick: I personally wouldn't use an if/else for this.
The "default" for $paramVarName is known and needs no special handling:

$paramVarName = $param['var'];
if ($realParams[$pos]['pass_by_reference'] === true && $paramVarName[0] === '&') {
    // ...
    $paramVarName = substr($paramVarName, 1);
    // ...
}

src/Standards/Squiz/Sniffs/Commenting/FunctionCommentSniff.php

jrfnl · 2023-06-07T20:34:36Z

src/Standards/Squiz/Sniffs/Commenting/FunctionCommentSniff.php

+                    $paramVarName = $param['var'];
+                }
+
+                if ($realName !== $paramVarName) {
                    $code = 'ParamNameNoMatch';
                    $data = [
                        $param['var'],


Suggested change

$param['var'],

$paramVarName,

Extra test to see the issue & safeguard the fix:

* @param string &$case A string passed in by reference with a case mismatch. ... public function variablesPassedByReference(&$foo, &$bar, $baz, $qux, &$CASE)

I thought that case mismatches were already covered by tests, but I couldn't find any. Keeping this as $param['var'] (and not using $paramVarName) here was intentional, but I can see now that the message can be confusing. I've adjusted the messaging (in 4171ba1) and added a set of tests (in 4f5641b) to cover @param tags with case mismatches.

@jrfnl would you prefer the message to omit the & completely?

If I were making these changes, I'd keep the "param has leading & while not passed by ref" vs the "param name mismatch" checks completely separate as these are effectively checking two completely different things.

So, yes, I'd do the name compare in a way to disregard a potential leading & completely.

I've had another look at this and completely separated out the ampersand/by-reference check. It will now trigger independently of the param name checks.

fredden · 2023-06-08T13:05:19Z

MissingParamTag - I have adjusted the sniff to not throw this when the variable is prefixed incorrectly in the @param tag. Should this still be thrown when only the case mismatches? For example,

1) PHP_CodeSniffer\Standards\Squiz\Tests\Commenting\FunctionCommentUnitTest::testSniff
[LINE 1050] Expected 0 error(s) in FunctionCommentUnitTest.inc but found 1 error(s). The error(s) found were:
 -> Doc comment for parameter "$CASE" missing (Squiz.Commenting.FunctionComment.MissingParamTag)
[LINE 1063] Expected 0 error(s) in FunctionCommentUnitTest.inc but found 1 error(s). The error(s) found were:
 -> Doc comment for parameter &$case does not match case of actual variable name &$CASE (Squiz.Commenting.FunctionComment.ParamNameNoCaseMatch)

jrfnl · 2023-06-09T02:21:21Z

MissingParamTag - I have adjusted the sniff to not throw this when the variable is prefixed incorrectly in the @param tag. Should this still be thrown when only the case mismatches? For example,

Parameters in PHP are case-sensitive, so yes, case mismatches should IMO definitely be flagged.

jrfnl

@fredden Thanks for the updates. IMO, there is one more tweak needed.

jrfnl · 2023-06-15T06:58:38Z

src/Standards/Squiz/Sniffs/Commenting/FunctionCommentSniff.php

+                    // This makes sure that the 'MissingParamTag' check won't throw a false positive.
+                    $foundParams[(count($foundParams) - 1)] = $paramVarName;
+
+                    if ($realParams[$pos]['pass_by_reference'] !== true) {


Suggested change

if ($realParams[$pos]['pass_by_reference'] !== true) {

if ($realName === $paramVarName && $realParams[$pos]['pass_by_reference'] !== true) {

Extra tests to verify why I'm suggesting this change:

/** * Test for param tag containing ref, but param in declaration not being by ref. * * @param string &$foo This should be flagged as ParamNameUnexpectedAmpersandPrefix. * @param string &$bar This should be flagged as ParamNameNoMatch. * @param string &$baz This should be flagged as ParamNameNoCaseMatch. * * @return void */ function passedByRefMismatch($foo, $bra, $BAZ) { return; }

This change seems fair. I had considered doing this but decided against it at the time. The reasoning was that if the variable name was incorrect and had a rogue leading ampersand, that both faults would be flagged. With the suggested change (which I've committed in 28ed6ae), users will get one fault identified at a time, so fixing one fault won't make all errors go away, but instead will make the error message change. I can see how having two errors is less desirable, especially in the case where the param tags are out of order and changing the order alone would fix all errors.

Good points and I agree that generally speaking errors shouldn't hide behind each other, but in this case, if the variable name doesn't match, the information about whether or not the variable is passed by reference from the function signature is just plain unreliable, which is why I suggested the change.

jrfnl

✅

fredden · 2023-06-15T14:16:31Z

Thanks for your time & patience on this one @jrfnl. I appreciate the feedback etc.

jrfnl · 2023-07-11T06:59:49Z

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

fredden · 2023-07-11T07:02:21Z

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

Yes, no problem. I'll look at this today.

jrfnl · 2023-12-08T22:56:18Z

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).

jrfnl reviewed Jun 7, 2023

View reviewed changes

jrfnl added Status: awaiting feedback Standard: Squiz labels Jun 7, 2023

fredden marked this pull request as draft June 7, 2023 12:30

fredden force-pushed the auto-fix-function-var-by-reference branch from b6ae58c to 10066e3 Compare June 7, 2023 15:45

fredden changed the title ~~Allow auto-fixing '@param type &$var' to '@param type $var'~~ Handle @param in docblock for variables passed by reference Jun 7, 2023

fredden force-pushed the auto-fix-function-var-by-reference branch 2 times, most recently from 75678d6 to edf58e8 Compare June 7, 2023 16:00

fredden marked this pull request as ready for review June 7, 2023 16:09

fredden requested a review from jrfnl June 7, 2023 16:09

jrfnl requested changes Jun 7, 2023

View reviewed changes

fredden force-pushed the auto-fix-function-var-by-reference branch from a0c54c6 to 605d788 Compare June 8, 2023 13:05

fredden requested a review from jrfnl June 12, 2023 20:59

jrfnl requested changes Jun 15, 2023

View reviewed changes

fredden requested a review from jrfnl June 15, 2023 09:07

jrfnl approved these changes Jun 15, 2023

View reviewed changes

jrfnl added Type: enhancement Type: bug Focus: Comments/Docblocks and removed Status: awaiting feedback labels Jun 15, 2023

jrfnl added this to the 3.8.0 milestone Jun 21, 2023

Handle @param when var passed by reference

84621dd

fredden force-pushed the auto-fix-function-var-by-reference branch from 1b973a0 to 84621dd Compare July 11, 2023 08:28

jrfnl merged commit 97bd952 into squizlabs:master Jul 11, 2023
26 of 27 checks passed

jrfnl added a commit that referenced this pull request Jul 11, 2023

Changelog for #3813

276f68c

fredden deleted the auto-fix-function-var-by-reference branch July 11, 2023 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `@param` in docblock for variables passed by reference #3813

Handle `@param` in docblock for variables passed by reference #3813

fredden commented Apr 28, 2023 •

edited

jrfnl left a comment

fredden commented Jun 7, 2023

jrfnl left a comment •

edited

jrfnl Jun 7, 2023

fredden Jun 8, 2023

jrfnl Jun 9, 2023

fredden Jun 12, 2023

fredden commented Jun 8, 2023

jrfnl commented Jun 9, 2023

jrfnl left a comment

jrfnl Jun 15, 2023

fredden Jun 15, 2023

jrfnl Jun 15, 2023

jrfnl left a comment

fredden commented Jun 15, 2023

jrfnl commented Jul 11, 2023

fredden commented Jul 11, 2023

jrfnl commented Dec 8, 2023

	if ($realParams[$pos]['pass_by_reference'] !== true) {
	if ($realName === $paramVarName && $realParams[$pos]['pass_by_reference'] !== true) {

Handle @param in docblock for variables passed by reference #3813

Handle @param in docblock for variables passed by reference #3813

Conversation

fredden commented Apr 28, 2023 • edited

jrfnl left a comment

Choose a reason for hiding this comment

fredden commented Jun 7, 2023

jrfnl left a comment • edited

Choose a reason for hiding this comment

jrfnl Jun 7, 2023

Choose a reason for hiding this comment

fredden Jun 8, 2023

Choose a reason for hiding this comment

jrfnl Jun 9, 2023

Choose a reason for hiding this comment

fredden Jun 12, 2023

Choose a reason for hiding this comment

fredden commented Jun 8, 2023

jrfnl commented Jun 9, 2023

jrfnl left a comment

Choose a reason for hiding this comment

jrfnl Jun 15, 2023

Choose a reason for hiding this comment

fredden Jun 15, 2023

Choose a reason for hiding this comment

jrfnl Jun 15, 2023

Choose a reason for hiding this comment

jrfnl left a comment

Choose a reason for hiding this comment

fredden commented Jun 15, 2023

jrfnl commented Jul 11, 2023

fredden commented Jul 11, 2023

jrfnl commented Dec 8, 2023

Handle `@param` in docblock for variables passed by reference #3813

Handle `@param` in docblock for variables passed by reference #3813

fredden commented Apr 28, 2023 •

edited

jrfnl left a comment •

edited