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

Allow ordering/prio of sniffs #3620

Closed dereuromark closed 2 years ago

dereuromark commented 2 years ago

Describe the bug

A race condition or rather (uncontrollable) order of sniff execution leads to invalid code for us.

Code sample

namespace Spryker;

use Org\App\BazBatInterface; // seen as unused

class Method
{
    protected \Org\App\BazBatInterface $bazBat;
}

is reported as

FOUND 2 ERRORS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------------------------------------------
 10 | ERROR | [x] Type Org\App\BazBatInterface is not used in this file. (SlevomatCodingStandard.Namespaces.UnusedUses.UnusedUse)
 14 | ERROR | [x] Use statement Org\App\BazBatInterface for class BazBatInterface should be in use block.
    |       |     (Spryker.Namespaces.UseStatement.Property)
-----------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY

And fixed to

namespace Spryker;

// seen as unused

class Method
{
    protected BazBatInterface $bazBat;
}

Now the code is broken as the use statement is gone and the FQCN is not there anymore either.

If the 2nd (adding use statement) could be set to a higher prio it would first just remove the typehint part inline and then the 2nd sniff should see the actual use statement it is not superfluous anymore and would then not remove it I assume.

Is there any plan on how to tackle these order issues?

jrfnl commented 2 years ago

@dereuromark The fixer will not allow two sniffs to change the same tokens in one loop and will delegate one of the fixes to the next loop.

Now, in this case, the sniffs aren't changing the same tokens, but are basing the need for the change on the same tokens.

So, while this may not be a solution, but more of a "work around" - if either (or both) of the sniffs would also "touch" the tokens they are basing their assessment for the need for the actual fix in the fixer changeset (by replacing the token contents with itself, i.e. not actually changing it), that would force the other sniff to the next loop, where it would then be able to re-evaluate the file based on the state after the first fix was applied.

Does that make sense ? Does that help in any way ?

dereuromark commented 2 years ago

That sounds like a neat trick indeed. Thank you. I then guess this ticket itself wont be to interesting for the main audience? We can close then.