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

Add autofix support for Squiz.Operators.ValidLogicalOperatorsSniff #613

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Koopzington
Copy link

@Koopzington Koopzington commented Sep 17, 2024

Description

Here's my slightly belated retarget of squizlabs/PHP_CodeSniffer#1370
The changes enable autofixing for cases where code behaviour won't change by replacing and with && as well as or with ||.

Suggested changelog entry

Added

  • The following sniff(s) have received autofix support:
    • Squiz.Operators.ValidLogicalOperatorsSniff

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.

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 @Koopzington, thank you for porting this PR over and for your willingness to contribute to PHPCS.

While I like the principle of the change, like Greg before me, I'm hesistant about merging the PR as-is. My hesistation is mostly because, IMO, the tests are not comprehensive enough.
All the test cases use quite simple code samples, while in real world code, things get messy and I don't currently have enough confidence that your changes will hold up in that scenario.

To help find more test cases, I've ran the sniff as per this PR over the top 2000 Packagist packages.
I've posted the results here: https://gist.github.com/jrfnl/ef7144c0d9b6fedf24dcb8f1d62685fe
Note: For this test run, I've changed the error code for the auto-fixable error to allow for seeing how many fixable and non-fixable issues were found. This is not a change which should be applied in the PR.

I'd like to suggest going through those results with a fine toothcomb to a) find any false positives which need to be handled (false positive in the sense of fixable, while it shouldn't be) and b) find test cases which should be added to the sniff tests (in anonymized form).
While doing so, keep in mind that the Packagist top 2000 are generally well-coded packages, so running the sniff over less well-coded packages may provide more useful input, but this is what I had available. Having said that, I would wager you can still find some inspiration in the results for improving the tests.

I've also left some remarks/questions inline with additional feedback.

I look forward to the next iteration of the PR.

P.S.: Mind - you may need to rebase the PR on the current master to get a passing build due to some hickups in the CI which have been fixed in the mean time.

T_YIELD,
T_YIELD_FROM,
T_PRINT,
];
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason not to use the Tokens::$assignmentTokens token array when building this list ?

$foo = ($a or $b xor $c);
$foo = ($a and $b or $c);
print $a and $b;
yield $a and $b;
?>
Copy link
Member

Choose a reason for hiding this comment

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

As this is a fixer, which could potentially lead to a change in behaviour in an application, I'd like to see a lot more variation in the tests to verify that the determination whether something is fixable is done correctly.

The fast majority of the above tests are quite simple statements, while real life code gets a lot more complex and varied.

Some examples of syntaxes I'm missing in the tests:

  • ternary expressions
  • use in an array key
  • use in an array value
  • use in a match condition
  • use in a match return expression
  • combinations of multiple operators, think: a ternary with an inline print expression

Comment on lines +101 to +103
if ($tokens[$index]['code'] === T_OPEN_PARENTHESIS) {
$index = ($phpcsFile->findEndOfStatement($index + 1) + 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason why findEndOfStatement() is used here instead of skipping to the parentheses closer via the $tokens[$index]['parenthesis_closer'] index (if it exists and if it doesn't exist, that's saying something too) ?

$index = ($phpcsFile->findEndOfStatement($index + 1) + 1);
}

if (in_array($tokens[$index]['code'], $blockList, true) === true) {
Copy link
Member

Choose a reason for hiding this comment

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

Performance nitpick: please set up the blocklist to allow for using isset() instead of in_array().

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