Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic/CallTimePassByReference: support anonymous classes and improve code coverage #394

Conversation

rodrigoprimo
Copy link
Contributor

@rodrigoprimo rodrigoprimo commented Mar 14, 2024

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
Copy link
Member

jrfnl commented Mar 20, 2024

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
Copy link
Member

jrfnl commented Mar 20, 2024

... 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
Copy link
Member

jrfnl commented Mar 20, 2024

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
Copy link
Contributor Author

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
Copy link
Member

jrfnl commented Mar 20, 2024

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 rodrigoprimo changed the title Generic/CallTimePassByReference: improve code coverage Generic/CallTimePassByReference: support anonymous classes and improve code coverage Mar 21, 2024
@rodrigoprimo
Copy link
Contributor Author

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.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Doing this to be able to create tests with syntax errors on separate
files.
This commit fixes a non-problematic bug when there are only empty tokens
after a variable or string token and nothing else.

Before this commit, the code would search for the next non-empty token
after a variable or string token. It would assume that there would
always be such a token and it would check if this token is a open
parenthesis token. It is possible that there are no tokens or only empty
tokens after the variable or string token when live coding.

This assumption would not cause any problems because when checking if
`$tokens[$openBracket]['code'] !== T_OPEN_PARENTHESIS`, `$openBracket`
would be `false`, and then PHP would check for the first element in the
$tokens array. The first token can never be a T_OPEN_PARENTHESIS and
so the code would bail early as expected.

This commit adds a `$openBracket === false` check to bail early as well
to make it clear that `$openBracket` can be false.
Removes a condition that checks if `nested_parenthesis` property of
the variable or short syntax array is not set. As far as I can check,
this condition can never be true. A few lines above, we are already
checking for the presence of opening and closing parentheses and the
while loop is limited to T_VARIABLE and T_OPEN_SHORT_ARRAY inside the
parentheses. Since it is limited to tokens inside the parentheses,
`nested_parenthesis` will always be set.

The removed check was added in this commit that added defensive code to
protect against syntax errors to a few sniffs:
PHPCSStandards@0e42098
This commit removes the `|| $prevCode === T_CLASS` condition from a if
statement. The removed check is not necessary as the next thing the
sniff does is to check for an opening parenthesis after `$stackPtr` and
bow out if one is not found. Class definitions will never have an
opening parentesis after its name.

Anonymous classes do have a opening parenthesis after the T_CLASS token.
They are currently not handled correctly by this sniff and this will be
addressed on another commit.
This commit changes the sniff to flag call-time pass-by-reference
arguments when instantiating an anonymous class.
@jrfnl jrfnl force-pushed the test-coverage-call-time-pass-by-reference branch from 7b483df to b91a18b Compare April 22, 2024 15:06
@jrfnl
Copy link
Member

jrfnl commented Apr 22, 2024

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

@jrfnl jrfnl added this to the 3.9.2 milestone Apr 22, 2024
@jrfnl jrfnl merged commit 8d2363d into PHPCSStandards:master Apr 22, 2024
40 checks passed
@rodrigoprimo rodrigoprimo deleted the test-coverage-call-time-pass-by-reference branch April 22, 2024 15:32
@rodrigoprimo
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants