search

squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

PHPCS annotations: selective re-enabling not working as expected #1986

Closed jrfnl closed 6 years ago

jrfnl commented 6 years ago

Example code based on: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-parts-of-a-file with some small adjustments to test this properly:

// phpcs:disable PEAR,Squiz.Arrays
$foo = [1,2,3];
bar( $foo, true );
// phpcs:enable PEAR.Functions.FunctionCallSignature
bar( $foo, true );
// phpcs:enable Squiz
$foo = [1,2,3];
// phpcs:enable

When running PHPCS over this snippet using phpcs -p -s ./test.php --standard=PEAR,Squiz --ignore-annotations, I see the following errors (ignoring the filecomment related errors):

 4 | ERROR | [x] Array with multiple values cannot be declared on a single line
   |       |     (Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed)
 5 | ERROR | [x] Space after opening parenthesis of function call prohibited
   |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
 5 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
   |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
 7 | ERROR | [x] Space after opening parenthesis of function call prohibited
   |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
 7 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
   |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
 9 | ERROR | [x] Array with multiple values cannot be declared on a single line
   |       |     (Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed)

If I repeat this without the --ignore-annotations, I only see the file comment errors, while I would expect to see:

A cursory analysis gives me the impression that selective re-enabling only works when using the exact same "selection" as for the disabling, so:

// phpcs:disable PEAR.Functions.FunctionCallSignature,Squiz.Arrays
$foo = [1,2,3];
bar( $foo, true );
// phpcs:enable PEAR.Functions.FunctionCallSignature
bar( $foo, true );
// phpcs:enable Squiz.Arrays
$foo = [1,2,3];
// phpcs:enable

... works as expected.

Using a higher/lower level Standard/Category/SniffName in the enable vs the disable command, seems to break the functionality which is in stark contrast to what the documentation suggests.

I've checked the above against various points in time between the 3.2.0 version and the current master, but it looks like this was never implemented as intended.

gsherwood commented 6 years ago

A cursory analysis gives me the impression that selective re-enabling only works when using the exact same "selection" as for the disabling

That looks right to me too, which wasn't the intention.

gsherwood commented 6 years ago

Getting this working:

// phpcs:disable PEAR,Squiz.Arrays
$foo = [1,2,3];
bar( $foo, true );
// phpcs:enable PEAR.Functions.FunctionCallSignature
bar( $foo, true );
// phpcs:enable Squiz
$foo = [1,2,3];
// phpcs:enable

Is going to be pretty hard using the current system. It only maintains a list of sniffs that are currently disabled, which allows you to re-enable a superset of sniffs (just remove entires from the list) but not a subset or a specific sniff, like in this case.

I think this will require a second whitelist to be created and used during the adding of messages, which is't ideal but I can't think of another way right now. Will think more though, and try and few things.

jrfnl commented 6 years ago

Is going to be pretty hard using the current system.

@gsherwood Yes, the only thing I could come up with while I was investigating this, was actually breaking the codes used in the "selection" down to the individual parts and setting it off against the sniffs included via the ruleset. That would, however, require both the disable logic in the Tokenizer as well as the enable logic to loop through all the "active" sniffs and disable/enable them individually.

gsherwood commented 6 years ago

I've committed a change to get this working. I ended up having to use an exception list, but I worked that into the current ignoredLines member var and did my best not to use it unless needed.

I also renamed the existing all entry in there to .all because it is possible to have a standard called All so that could have got confusing.

I'll leave this open in case you have some time to test.

jrfnl commented 6 years ago

@gsherwood Thanks for the quick turn-around on this one.

Tests are mostly looking good, though I found one weird bug.

Given this example code:

// phpcs:disable PEAR
bar( $foo, true );
// phpcs:enable PEAR.Functions.FunctionCallSignature
bar( $foo, true );

// phpcs:disable PEAR.Functions.FunctionCallSignature
bar( $foo, true );
// phpcs:enable
bar( $foo, true );

When running PHPCS over this snippet using phpcs -p -s ./test.php --standard=PEAR --ignore-annotations, I see the following errors (ignoring the comment related errors):

  4 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  4 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
  6 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  6 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
  9 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  9 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
 11 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
 11 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)

If I repeat this without the --ignore-annotations, I:

gsherwood commented 6 years ago

Tests are mostly looking good, though I found one weird bug.

I totally missed that case (selectively re-enable a sniff, then disable it again) but the latest commit fixes that. Thanks so much for testing.

jrfnl commented 6 years ago

@gsherwood Thank you. I'll try and run some more tests later.

jrfnl commented 6 years ago

@gsherwood Ok, so I think I've found another one. 😕

// phpcs:disable PEAR.Functions
bar( $foo, true );
// phpcs:enable PEAR.Functions.FunctionCallSignature
bar( $foo, true );
// phpcs:enable PEAR.Functions.ValidDefaultValue
bar( $foo, true );
// phpcs:enable PEAR
bar( $foo, true );

When running PHPCS over this snippet using phpcs -p -s ./test.php --standard=PEAR --ignore-annotations, I see the following errors (ignoring the comment related errors):

  4 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  4 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
  6 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  6 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
  8 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
  8 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)
 10 | ERROR | [x] Space after opening parenthesis of function call prohibited
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)
 10 | ERROR | [x] Expected 0 spaces before closing bracket; 1 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceBeforeCloseBracket)

If I repeat this without the --ignore-annotations, I:

gsherwood commented 6 years ago

Ok, so I think I've found another one. 😕

Yep, I figured out what I stuffed up and have fixed that one up too. Thanks for testing (again)! 😄

jrfnl commented 6 years ago

Thanks Greg. I'm going to leave it for a bit now as my imagination has run out of possible tests I can run. Fingers crossed I won't run into any other exceptions and no further bug reports come in about this ;-)

gsherwood commented 6 years ago

I'm going to leave it for a bit now as my imagination has run out of possible tests I can run.

Thanks again. I'll close as I think all the major cases are covered.