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.63k stars 1.48k forks source link

Squiz.WhiteSpace.ScopeKeywordSpacing does not report tabs after "private" #3901

Closed Daimona closed 9 months ago

Daimona commented 9 months ago

Describe the bug

The Squiz.WhiteSpace.ScopeKeywordSpacing is used to enforce a single space after scope keywords, which include visibility modifiers on method. While the sniff correctly emits an issue if the public keyword is followed by a tab, it doesn't do that for the private keyword. Judging from the error message, it looks like the sniff might be computing the length of the spacing, instead of checking the characters.

Code sample

class MyClass {
    public  function getPublicFoo() { // <-- tab between "public" and "function"
        return 'foo';
    }
    private function getPrivateFoo() { // <-- tab between "private" and "function"
        return 'foo';
    }
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
  <rule ref="Squiz.WhiteSpace.ScopeKeywordSpacing" />
</ruleset>

To reproduce

Steps to reproduce the behavior:

  1. Create a file called test.php with the code sample above...
  2. Run phpcs test.php
  3. See error message displayed
    4 | ERROR | [x] Scope keyword "public" must be followed by a single space; found 2
    |       |     (Squiz.WhiteSpace.ScopeKeywordSpacing.Incorrect)
    PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY

Expected behavior

PHPCS should emit an error for the getPrivateFoo function, similar to the one emitted for getPublicFoo.

Versions (please complete the following information)

Operating System Ubuntu 22
PHP version 8.2.10
PHP_CodeSniffer version master
Standard custom
Install type composer

Please confirm:

jrfnl commented 9 months ago

@Daimona Thanks for the report. This is however not a false positive as tabs vs spaces is not the concern of this sniff.

When you use inline tabs, the size of the tab is determined by the context and the tabWidth setting.

In your case, I presume you have this set to --tab-width=4 ?

That would result (correctly) in:

$ phpcs ./phpcs-3901.php --standard=Squiz --sniffs=Squiz.WhiteSpace.ScopeKeywordSpacing --tab-width=4

FILE: phpcs-3901.php
------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------
 4 | ERROR | [x] Scope keyword "public" must be followed by a single space; found 2
------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------

While if you would set the tabWidth to 6, it would result in:

$ phpcs ./phpcs-3901.php --standard=Squiz --sniffs=Squiz.WhiteSpace.ScopeKeywordSpacing --tab-width=6

FILE: phpcs-3901.php
-------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-------------------------------------------------------------------------------------
 4 | ERROR | [x] Scope keyword "public" must be followed by a single space; found 6
 7 | ERROR | [x] Scope keyword "private" must be followed by a single space; found 5
-------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------

If you want to forbid tabs for anything but indentation, I suggest you add a sniff to that effect to your ruleset.

The PHPCSExtra package, for instance, has the Universal.WhiteSpace.DisallowInlineTabs sniff to do just that.

I suggest closing this issue.

Daimona commented 9 months ago

This is however not a false positive as tabs vs spaces is not the concern of this sniff.

Oh, sorry, I understand now. I thought "single space" meant "exactly one space (U+0020) character", but indeed, that was a bad assumption on my end.

In your case, I presume you have this set to --tab-width=4 ?

I did not set that explicitly, but I just realized that the wrapper I was using to test this did that internally. Sorry about that!

If you want to forbid tabs for anything but indentation, I suggest you add a sniff to that effect to your ruleset.

The PHPCSExtra package, for instance, has the Universal.WhiteSpace.DisallowInlineTabs sniff to do just that.

I'll look into that, thank you!