Generic/CallTimePassByReference: support anonymous classes and improve code coverage

[rodrigoprimo]

Description

This PR changes the Generic.Functions.CallTimePassByReference sniff to flag call-time pass-by-reference when instantiating an anonymous class. It also fixes a non-problematic bug, improves code coverage and removes an unreachable condition.

Regarding the unreachable condition, I worry that I might be missing something and the condition is actually reachable. I can drop the commit if during the review you are not sure if it is unreachable or not.

While working on this PR, I could see a few potential improvements to this sniff (like bailing early for all OO scope tokens and not just classes, but there is something that I want to check before implementing those improvements (likely on a separate PR). PHPCS supports PHP >= 5.4. Call-time pass-by-reference was removed in PHP 5.4. This means that this sniff checks for syntax that causes a fatal error in all supported PHP versions and thus I wonder if it should be deprecated? The only use case that I can think of for this sniff is if someone decides to modernize PHP < 5.4 code and I'm unsure if this is a reason to keep this sniff or not.

Suggested changelog entry

Generic.Functions.CallTimePassByReference: fixed non-problematic bug when $openBracket is false
Generic.Functions.CallTimePassByReference: flag call-time pass-by-reference when instantiating an anonymous class

Related issues/external references

Part of #146

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

Call-time pass-by-reference was removed in PHP 5.4. This means that this sniff checks for syntax that cause a fatal error in all supported PHP versions and thus I wonder if it should be deprecated?

The minimum supported PHP version of this codebase is completely unrelated to the supported PHP version(s) of the code base under scan, so that's never a valid argument.
By that same logic all sniffs checking for things only allowed in PHP > 5.4 shouldn't exist as this codebase can not use that syntax...

The only use case that I can think of for this sniff is if someone decides to modernize PHP < 5.4 code and I'm unsure if this is a reason to keep this sniff or not.

That's not the only usecase, it is also useful to prevent new issues from entering a codebase, which could just be a simple oversight from whomever wrote the code or it could be that the person who wrote the code is not that familiar with PHP.

More than anything, I see no reason to remove it as long as there are people who use the sniff.

[jrfnl]

... like bailing early for all OO scope tokens and not just classes

Actually, AFAICS the || $prevCode === T_CLASS can be removed as the very next thing the sniff does, is check for an open parenthesis after the $stackPtr and bow out when none is found and OO declarations will never [*] have an open parenthesis after their name, so the sniff would bow out anyway.
[*] That's aside from anonymous classes - new class(), in which case the sniff should check the passed parameters.

[jrfnl]

Regarding the unreachable condition, I worry that I might be missing something and the condition is actually reachable. I can drop the commit if during the review you are not sure if it is unreachable or not.

Did you do a git blame to figure out why the code was added ?

[rodrigoprimo]

The minimum supported PHP version of this codebase is completely unrelated to the supported PHP version(s) of the code base under scan, so that's never a valid argument.
By that same logic all sniffs checking for things only allowed in PHP > 5.4 shouldn't exist as this codebase can not use that syntax...

Thanks for clarifying that the sniff should not be deprecated and providing good reasons for that.

That's not the only usecase, it is also useful to prevent new issues from entering a codebase, which could just be a simple oversight from whomever wrote the code or it could be that the person who wrote the code is not that familiar with PHP.

I assumed this use case was not relevant since the call-time pass-by-reference causes a parse error in PHP >= 5.4 (I'm saying this just to share my thought process, and not to continue arguing for the deprecation of this sniff).

Actually, AFAICS the || $prevCode === T_CLASS can be removed as the very next thing the sniff does, is check for an open parenthesis after the $stackPtr and bow out when none is found and OO declarations will never [*] have an open parenthesis after their name, so the sniff would bow out anyway.

Good point. I added a new commit removing || $prevCode === T_CLASS. I did not add tests as I believe the test on line 2 is enough.

[*] That's aside from anonymous classes - new class(), in which case the sniff should check the passed parameters.

What you have in mind is that the sniff should be triggered by the code below?

$anon = new class(&$a) {};

I guess that the rationale is that, even if this code is not valid in any version of PHP (no version that supports both call-time pass-by-reference and anonymous class), it is still useful for users if the sniff flags this code to prevent new issues from entering the codebase?

Did you do a git blame to figure out why the code was added ?

Yes, I mentioned it in the commit message but I could have mentioned it in the PR description:

The removed check was added in this commit that added defensive code to protect against syntax errors to a few sniffs: 0e42098

It is not clear to me by checking the commit if there is a valid case that this code is protecting against.

[jrfnl]

I assumed this use case was not relevant since the call-time pass-by-reference causes a parse error in PHP >= 5.4

Well, unfortunately the fast majority of projects do not have a CI process set up or only a limited one (only PHPCS), so the chances of this not being caught by linting/tests can be pretty high, which means the fatal would bring down production.

Good point. I added a new commit removing || $prevCode === T_CLASS. I did not add tests as I believe the test on line 2 is enough.

[*] That's aside from anonymous classes - new class(), in which case the sniff should check the passed parameters.

What you have in mind is that the sniff should be triggered by the code below?

That test is sufficient for class declarations, but you mentioned other OO declarations as the trigger for your original remark, so maybe tests should be added to guard against false positives for traits/enums/interfaces declarations ?

And yes, also to guard against false negatives for anonymous classes.

I guess that the rationale is that, even if this code is not valid in any version of PHP (no version that supports both call-time pass-by-reference and anonymous class), it is still useful for users if the sniff flags this code to prevent new issues from entering the codebase?

Whether the syntaxes are mutually exclusive is irrelevant. This sniff should do what its purpose is: detect call time pass by reference and even when used in an anonymous class declaration, it is still call time pass by reference.

On that note: can you think of situations where the $stackPtr could be followed by a ( and not be a function call ? If so, please add tests for those too.

Did you do a git blame to figure out why the code was added ?

Yes, I mentioned it in the commit message but I could have mentioned it in the PR description:

Please always add all relevant information in the PR description. Don't make me go searching for things.

The removed check was added in this commit that added defensive code to protect against syntax errors to a few sniffs: 0e42098

It is not clear to me by checking the commit if there is a valid case that this code is protecting against.

Did you check the issue the commit references for the bug report ?

[rodrigoprimo]

That test is sufficient for class declarations, but you mentioned other OO declarations as the trigger for your original remark, so maybe tests should be added to guard against false positives for traits/enums/interfaces declarations ?

I added a new commit with tests to guard against false positives for traits/enums/interfaces. I used the test for classes as a reference.

On that note: can you think of situations where the $stackPtr could be followed by a ( and not be a function call ? If so, please add tests for those too.

The only other situation I could think of is class instantiation. So, I added a few tests to cover that in the same commit I mentioned above.

And yes, also to guard against false negatives for anonymous classes.

In a separate commit, I changed the sniff to flag call-time pass-by-reference in anonymous classes and updated the PR title and description to reflect that. The implementation was fairly simple. I hope I'm not missing something.

Please always add all relevant information in the PR description. Don't make me go searching for things.

Sure, and I'm sorry for doing it this time.

Did you check the issue the commit references for the bug report ?

Yes and no. The first time I checked, I failed to notice that I was checking the wrong issue. The commit message points to issue 316 in this repository. I didn't see the connection between the two and thought I had reached a dead end.

Only now, I realized that I should have checked issue 316 in the old repository: squizlabs/PHP_CodeSniffer#316. The issue mentions two code snippets that would cause PHPCS to generate PHP notices. Unfortunately, one of the snippets is not available anymore. I confirmed that the sniff correctly handles the code snippet that is still available even if the condition that I removed.

[jrfnl]

Hi @rodrigoprimo Looking good!

Thanks for making these changes.

There is one additional syntax I can think of which this sniff is currently not picking up on, while it probably should be:

new static(&a);

(and the same with self/parent)

Not a blocker for this PR, but probably a nice to have for a follow-up PR. Would you be willing to work on that ?

[jrfnl]

Rebased (to get passed the merge conflict) and squashed a few commits together, no functional changes. Will merge once the build passes now.

[rodrigoprimo]

Would you be willing to work on that ?

Yes, I will prepare a PR to change the sniff to support the cases that you mentioned.