Add the possibility to ignore rector rules at the file level by carlos-granados · Pull Request #5952 · rectorphp/rector-src

carlos-granados · 2024-06-07T13:54:10Z

Currently it is possible to skip a file or list of files when applying rector rules and it is also possible to skip a particular rule. You can also skip a particular rule for a file or a list of files. But it is not possible to skip a single case of applying a rule in a file without also skipping all the other cases of that rule in that file.

This PR follows a similar solution implemented by PHPStan with their @phpstan-ignore and @phpstan-ignore-next-line tags and implements two comment tags:

@rector-ignore-next-line will let you skip any rector rule that might be applied to the node defined in the next line
@rector-ignore RULE_NAME will let you skip any particular rector rule that might be applied to the node defined in the next line. RULE_NAME needs to be the name of the class for that particular rule (for example NumericReturnTypeFromStrictScalarReturnsRector) without a namespace. If you want to skip more than one rule you can list them separated with commas

Includes a test for the new functionality, see the comments in the PR for more info

samsonasik · 2024-06-07T14:05:17Z

+use Rector\Skipper\Contract\SkipVoterInterface;
+use Rector\Skipper\Skipper\SkipSkipper;
+
+final readonly class CommentSkipVoter implements SkipVoterInterface


use a dedicated service instead, and use on Skipper::shouldSkipCurrentNode() to avoid bc break due to new ?Node $node parameter addition.

carlos-granados · 2024-06-07T13:54:44Z

     * @var array<class-string<SkipVoterInterface>>
     */
-    private const SKIP_VOTER_CLASSES = [ClassSkipVoter::class];
+    private const SKIP_VOTER_CLASSES = [ClassSkipVoter::class, CommentSkipVoter::class];


We add a new SkipVoter that will look at the comments for any node

carlos-granados · 2024-06-07T13:55:35Z

    public function match(string | object $element): bool;

-    public function shouldSkip(string | object $element, string $filePath): bool;
+    public function shouldSkip(string | object $element, string $filePath, ?Node $node): bool;


We modify the SkipVoterInterface so that the node is also passed to the shouldSkip function

That bc break, additional functionality can be added Skipper::shouldSkipCurrentNode() to avoid bc break.

Do you mean adding this to the interface? 🤔

carlos-granados · 2024-06-07T13:55:58Z

    }

-    public function shouldSkip(string | object $element, string $filePath): bool
+    public function shouldSkip(string | object $element, string $filePath, ?Node $node): bool


Modify the existing voter to follow the new interface

carlos-granados · 2024-06-07T13:56:39Z

+use Rector\Skipper\Contract\SkipVoterInterface;
+use Rector\Skipper\Skipper\SkipSkipper;
+
+final readonly class CommentSkipVoter implements SkipVoterInterface


This is the new voter. It calls a new function in the SkipSkipper helper which is the one that actually checks the comments

carlos-granados · 2024-06-07T13:57:04Z


 final readonly class SkipSkipper
 {
+    private const RECTOR_IGNORE_NEXT_LINE_TAG = '@rector-ignore-next-line';


These are the two tags that we expect to see in the comments

carlos-granados · 2024-06-07T14:03:36Z

+        return $this->myVariable;
+    }
+
+    // @rector-ignore AddParamTypeFromPropertyTypeRector, AddVoidReturnTypeWhereNoReturnRector


It should be possible to list more than one rule

carlos-granados · 2024-06-07T14:03:52Z

+        $this->myVariable = $myVariable;
+    }
+
+    // @rector-ignore AddParamTypeFromPropertyTypeRector


It should also be possible to have more than one comment

carlos-granados · 2024-06-07T14:04:14Z

+    }
+
+    /**
+     * @rector-ignore AddParamTypeFromPropertyTypeRector


Or use a comment with several lines

carlos-granados · 2024-06-07T14:04:54Z

+        $this->myVariable = $myVariable;
+    }
+
+    // @rector-ignore-next-line


If we use the ignore next line tag, it should ignore all rules

carlos-granados · 2024-06-07T14:05:10Z

+        $this->myVariable = $myVariable;
+    }
+
+    // @rector-ignore AddParamTypeFromPropertyTypeRector


If we ignore one rule, all other rules should still apply

samsonasik · 2024-06-07T14:10:28Z

+{
+    private int $myVariable;
+
+    // @rector-ignore NumericReturnTypeFromStrictScalarReturnsRector


I think this needs to be fqcn so exact class name will be validated to avoid 2 class (core vs override custom rule conflict)

Having to use the FQCN would not be very convenient as you would end up with comments like

// @rector-ignore Rector\TypeDeclaration\Rector\ClassMethod\NumericReturnTypeFromStrictScalarReturnsRector

I tried to implement it so that you should import the class by doing something like:

use Rector\TypeDeclaration\Rector\ClassMethod\NumericReturnTypeFromStrictScalarReturnsRector; ... // @rector-ignore NumericReturnTypeFromStrictScalarReturnsRector

My problem was then converting this to a FQCN when reading the comment so that I could compare it to the class name of the current rule. I could not find a way to do this, any pointers would be helpful

carlos-granados · 2024-06-07T14:15:35Z

@samsonasik saw your comments about the BC break but not sure how to proceed here. The shouldSkip function is called from the shouldSkipElementAndFilePath method. This method expects a list of skipVoters, all of them following the SkipVoterInterface interface. I am not sure how I could use another service to provide the node to the CommentSkipVoter. Can you elaborate?

samsonasik · 2024-06-07T14:28:23Z

The new service should not implements SkipVoterInterface, but a dedicated service, like PathSkipper class:

rector-src/src/Skipper/Skipper/PathSkipper.php

Line 10 in a22c3b9

final readonly class PathSkipper

then, use it on after this line:

rector-src/src/Skipper/Skipper/Skipper.php

Line 70 in a22c3b9

return $this->rectifiedAnalyzer->hasRectified($rectorClass, $node);

should be something like:

-return $this->rectifiedAnalyzer->hasRectified($rectorClass, $node);
+if ($this->rectifiedAnalyzer->hasRectified($rectorClass, $node)) {
+    return true;
+}
+
+return $this->commentSkipper->shouldSkip($rectorClass, $node);

carlos-granados · 2024-06-07T14:31:51Z

@samsonasik understood now, I will work to modify this PR to follow your suggestion

samsonasik · 2024-06-07T15:09:31Z

Btw, the similar functionality was exists with @noRector doc, it was removed by @TomasVotruba at

[config] Deprecate and remove @noRector annotation, as not reliable DX choice; use skip() method instead #3148

You can check the reasoning there, and if there is new proposal for it, I will let @TomasVotruba decide

carlos-granados · 2024-06-07T15:11:53Z

Btw, the similar functionality was exists with @noRector doc, it was removed by @TomasVotruba at

[config] Deprecate and remove @noRector annotation, as not reliable DX choice; use skip() method instead #3148

You can check the reasoning there, and if there is new proposal for it, I will let @TomasVotruba decide

Didn't know about this. Will check it!

carlos-granados · 2024-06-12T16:47:16Z

@samsonasik added two changes:

Converted the CommentSkipVoter into a CommentSkipper service
Use FQN when comparing excluded rules. You can either use:

// @rector-ignore Rector\TypeDeclaration\Rector\ClassMethod\NumericReturnTypeFromStrictScalarReturnsRector

or

use Rector\TypeDeclaration\Rector\ClassMethod\NumericReturnTypeFromStrictScalarReturnsRector;
...
// @rector-ignore NumericReturnTypeFromStrictScalarReturnsRector

samsonasik · 2024-06-13T04:18:33Z

If docblock tag is generic, classnames can be collected from PhpDocInfo, something like this

rector-src/src/PostRector/Rector/UnusedImportRemovingPostRector.php

Line 148 in de31e6f

$genericTagClassNames = $phpDocInfo->getGenericTagClassNames();

carlos-granados · 2024-06-13T06:45:42Z

If docblock tag is generic, classnames can be collected from PhpDocInfo, something like this

rector-src/src/PostRector/Rector/UnusedImportRemovingPostRector.php

Line 148 in de31e6f

$genericTagClassNames = $phpDocInfo->getGenericTagClassNames();

The problem is that we want to be able to use comments which are not docblocks and these do not get parsed in the same way

samsonasik · 2024-06-13T06:52:48Z

The generic is something like @see SomeClass which pointed to full classname already, if @norector detected as generic, next value will getted as classnames, you can then filter by generic name, so it pointed to correct target generic type node.

TomasVotruba · 2024-06-17T09:24:57Z

Hi @carlos-granados , thanks for patience and appologies for my late reply.
I took a time to think about this deeply and was distracted by other work and open-source.

Using dev tool configuration in code seems like a wrong design. That's why I removed it in the past: #5952 (comment)

Code Sniffer/PHPStan have such a feature, but it doesn't bring any new value to the tool. In practise, it also makes false positive ignored hard to spot, as part of the code. We often have to go through it one by one and check, and check if still valid. PHPStan reports such cases, but Rector does not. It would also cost us a performance hit.

Having one place to ignore errors is the best design choice, as there place of least surprise. We avoid cases like "oh, we forgot to check every file for the docblock and that's why this upgrade rule was missed".

Another problem is, that when we had such a feature, it was used as "ignore every rule", only because it was easy and possible.

That's why I'm rejecting this feature.

Thanks for understanding 🙏

carlos-granados · 2024-06-18T10:26:46Z

No problem, thanks for the great explanation. And, in any case, creating this PR has allowed me to learn more about Rector, which is always good 😊

…

On Mon, 17 Jun 2024, 05:25 Tomas Votruba, ***@***.***> wrote: Hi @carlos-granados <https://github.com/carlos-granados> , thanks for patience and appologies for my late reply. I took a time to think about this deeply and was distracted by other work and open-source. Using dev tool configuration in code seems like a wrong design. Code Sniffer/PHPStan have such a feature, but it didn't bring any new value to the tool. In practise, it also makes false positive ignored hard to spot, as part of the code. We often have to go through it one by one and check, and check if still valid. PHPStan reports such cases, but Rector does not. It would also cost us a performance hit. Having one place to ignore errors is the best design choice, as there place of least surprise. We avoid cases like "oh, we forgot to check every file for the docblock and that's why this upgrade rule was missed". Another problem is, that when we had such a feature, it was used as "ignore every rule", only because it was easy and possible. That's why I'm rejecting this feature. Thanks for understanding 🙏 — Reply to this email directly, view it on GitHub <#5952 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKRVQW6XSWERRAGSVO6QMTZH2TQBAVCNFSM6AAAAABI6W6CRWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZSHA2TGMBSG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

carlos-granados · 2024-10-04T11:57:27Z

If anyone else is interested in this option, it is available in this fork: https://github.com/carlos-granados/rector

Add the possibility to ignore rector rules at the file level

a4a5f90

samsonasik reviewed Jun 7, 2024

View reviewed changes

Comment thread src/Skipper/Contract/SkipVoterInterface.php Outdated

samsonasik reviewed Jun 7, 2024

View reviewed changes

Comment thread src/Skipper/Skipper/SkipSkipper.php Outdated

samsonasik reviewed Jun 7, 2024

View reviewed changes

carlos-granados commented Jun 7, 2024

View reviewed changes

samsonasik reviewed Jun 7, 2024

View reviewed changes

Use NewLineSplitter

56c22ff

carlos-granados added 2 commits June 12, 2024 18:02

Convert the CommentSkipVoter into a CommentSkipper service

f9cfab9

Use FQN when comparing excluded rules

ddf1ca7

Fix code

8038a8b

TomasVotruba closed this Jun 17, 2024

carlos-granados deleted the rector-ignore branch August 22, 2024 11:41

Uh oh!

Conversation

carlos-granados commented Jun 7, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-granados commented Jun 7, 2024

Uh oh!

samsonasik commented Jun 7, 2024

Uh oh!

carlos-granados commented Jun 7, 2024

Uh oh!

samsonasik commented Jun 7, 2024

Uh oh!

carlos-granados commented Jun 7, 2024

Uh oh!

carlos-granados commented Jun 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samsonasik commented Jun 13, 2024

Uh oh!

carlos-granados commented Jun 13, 2024

Uh oh!

samsonasik commented Jun 13, 2024

Uh oh!

TomasVotruba commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlos-granados commented Jun 18, 2024 via email

Uh oh!

carlos-granados commented Oct 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

carlos-granados commented Jun 12, 2024 •

edited

Loading

TomasVotruba commented Jun 17, 2024 •

edited

Loading