[4.0.0] Excluding/severity 0 should not load a rule

kkmuffme commented 11 months ago

Describe the bug

Excluding a rule/setting severity 0, should NOT load the rule, if it hasn't been loaded yet.

To reproduce

exclude any rule in any ruleset. Run with "-vv" and see

                    Processing rule "Foo.Bar"
                        => /some/path/foo/bar.php
                        => severity set to 0

Expected behavior

No processing of the rule.

Versions (please complete the following information)

PHP_CodeSniffer version 3.7.2 and before

Additional context

Just like phpcs-only/phpcbf-only="true" will not process the specified rule, excluding the rule shouldn't either. This is important, because sometimes you want to use an external ruleset, where some rules don't work (loading them will throw PHP notices with your/newer PHP versions) or are slowing down phpcs a lot. Currently the only way to do that is to copy the whole ruleset instead and not include those buggy rules, which creates bloated, hard to maintain code.

Please confirm:

[x] I have searched the issue list and am not opening a duplicate issue.
[x] I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
[x] I have verified the issue still exists in the master branch of PHP_CodeSniffer.

jrfnl commented 10 months ago

@kkmuffme Thank you for creating this issue. This is, however, not a bug, but a feature request.

In my opinion, if someone is willing to work on this, I think it may be a nice candidate for the 4.0 release, though things are not as simple as posed in the description.

Just like phpcs-only/phpcbf-only="true" will not process the specified rule, excluding the rule shouldn't either.

Well, there is a difference between those two types of "excluding", which in my mind, explains/justifies the current behaviour.

When phpcs-only/phpcbf-only="true" is used, it is quite straight-forward to determine whether the ruleset directive should be processed as you can check how PHPCS was invoked and skip the complete directive based on that.

With a severity change, this is nowhere near as straight-forward as the severity can be changed for an individual error code (Stnd.Cat.Sniff.Code), for all error codes in a sniff (Stnd.Cat.Sniff), for a complete category of sniffs (Stnd.Cat) or even for a complete standard (Stnd). The same applies when using exclude name=..., which effectively does the same thing.

When a severity change is made for an individual error code, PHPCS has no awareness of whether the sniff has only one error code or multiple, so will need to include the sniff anyway and handle the severity change at the point of throwing an error/warning.

When a severity change is made for a complete sniff, category or standard, that is different and not loading the sniff would become an option, but that would only be possible if the ruleset processing is changed significantly.

Currently, a ruleset is read top-to-bottom and processed as such as well.

That means that each directive is read and then processed, before the next directive is read, i.e. the sniffs will be loaded once the directive including them is read.

So:

    <rule ref="PSR1">
        <exclude name="Generic"/>
        <exclude name="Squiz.Classes.ValidClassName"/>
    </rule>

... will read the rule directive and only include those sniffs from the PSR1 ruleset which are not excluded.

While:

    <rule ref="PSR1"/>
    <rule ref="PSR1.Classes.ClassDeclaration">
        <severity>0</severity>
    </rule>

... will first read the PSR1 directive and include all sniffs from the PSR1 ruleset. Only after that, it will read the PSR1.Classes.ClassDeclaration directive and set the severity to 0 to be handled at the point of throwing the error/warning as the PSR1.Classes.ClassDeclaration sniff will already be included at that point.

To make the change you are proposing, the ruleset would need to be read and interpreted completely first and only after the complete ruleset has been interpreted, sniffs should be loaded.

As this is a significant change in how the ruleset is handled, which may have unforeseen consequences/side-effects, making this change in the 3.x series is not an option (IMO) and significant testing would need to be done (and unit tests written) to accept a change as proposed here for 4.0.

I'm leaving this open for now in case anyone is interested in working on this.

I do think this is an interesting proposal though, as this could have significant (positive) impact on the performance of PHPCS.

As for the following:

loading them will throw PHP notices with your/newer PHP versions

You may want to run PHPCS with -d error_reporting=E_ALL^E_DEPRECATED added to the command line args. That should solve that issue.

kkmuffme commented 10 months ago

the ruleset would need to be read and interpreted completely first and only after the complete ruleset has been interpreted, sniffs should be loaded.

Yes, looks like it. In that case, you could even go further to improve performance - instead of checking 0, don't load any errors/warnings whose severity is set below the threshold: phpcs --error-severity=5 --warning-severity=8 /path/to/code

Which means the config won't load any rules that have a severity below 5/8

You may want to run PHPCS with -d error_reporting=E_ALL^E_DEPRECATED added to the command line args. That should solve that issue.

The problem is that not all errors fall in the "deprecated" category, as especially PHP 8 has a couple new errors thrown where PHP 7 returned false instead, and snoozing those would potentially mask bugs/issues that need to be investigated.

squizlabs / PHP_CodeSniffer