Rector: Ability to exclude specific file per rule

[Levivb]

Hey,

I'm currently in the process of implementing Rector in one of our projects as testcase and am running into a few issues.

At the moment there doesn't seem to be a way to exclude a specific class for a specific rule. Some rules tend to crash on some code and it would be nice to be able to use the rule but exclude some classes.

There's the possibility to exclude certain paths in the parameters key:

parameters:
  exclude_paths:
    - 'Assets/*'
    - 'ci/*'
    - 'Database/Migrations/*'

But that's an all or nothing exclusion

Would it be possible to support configuration per rule? Similar to how the PhpCS supports it.

Rector\DeadCode\Rector\ClassMethod\RemoveUnusedParameterRector:
  exclude_paths:
    - 'Some/Exceptional/Directory/*'
    - 'Some/Other/Class.php

As icing of the cake having the ability to suppress a rule on a line by line basis, or code blocks would also be pretty cool.

Code sniffer example:

// some code

// phpcs:disable Some.Rule.Not.ApplicableHere
// violating but allowed code

// more violating but allowed code
// phpcs:enable

// some more code

// phpcs:ignore Some.Rule.Not.ApplicableHere
// a single line allowed violating code

// final code

Which could be supported in rector like this:

// some code

// rector:disable Rector\DeadCode\Rector\ClassMethod\RemoveUnusedParameterRector
// violating but allowed code

// more violating but allowed code
// rector:enable

// some more code

// rector:ignore Rector\DeadCode\Rector\ClassMethod\RemoveUnusedParameterRector
// a single line allowed violating code

// final code

Let me know what you think!

[TomasVotruba]

Hi,

I wonder if that's bandaid to problem and desired feature.

Some rules tend to crash on some code and it would be nice to be able to use the rule but exclude some classes.

That should not happen and it should be fixed. What exactly is crashing?

[JanMikes]

Imho nice example of how it could work is https://github.com/Symplify/EasyCodingStandard#ignore-what-you-cant-fix:

parameters:
    skip:
        SlevomatCodingStandard\Sniffs\TypeHints\TypeHintDeclarationSniff:
            - 'packages/EasyCodingStandard/packages/SniffRunner/src/File/File.php'

[Levivb]

I just submitted the actual crash: https://github.com/rectorphp/rector/issues/1592

Ignoring some code would still be nice feature in my opinion though.
Take Rector\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector for example

That rule is incompatible with Laravel entities as it would add properties for attributes which would normally be handled by __get in Laravel. Enabling this rule would completely destroy all entities/models. The rule itself however is rather useful. Being able to exclude the path to the entities would prevent anyone using Laravel from being excluded from this rule.

[TomasVotruba]

That rule is incompatible with Laravel entities as it would add properties for attributes which would normally be handled by __get in Laravel. Enabling this rule would completely destroy all entities/models. The rule itself however is rather useful. Being able to exclude the path to the entities would prevent anyone using Laravel from being excluded from this rule.

This should work out of the box and not bother user with that. Could you provide minimal example of such code? Then we can fix it

[Levivb]

Submitted as separate bug report: https://github.com/rectorphp/rector/issues/1594 and https://github.com/rectorphp/rector/issues/1595

| Subject | Details |
| :------------- | :----------------------------------------------------------- |
| PHP version | PHP 7.2 |
| Full Command | vendor/bin/rector --config rector.yml process User.php -n |

# rector.yml
parameters:
  php_version_features: '7.2'

services:
  Rector\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector: ~

An example would be the User entity in our codebase (heavily simplified):

<?php namespace Modules\Core\Entities;

use Carbon\Carbon;
use Modules\Core\Entities\Traits\EntrustUserTrait;

class User extends BaseModel
{
    use EntrustUserTrait;

    public function getTitleAttribute()
    {
        return $this->name;
    }

    public function getBestRoleAttribute()
    {
        return $this->roles->sortBy('id')->first();
    }

    public function setInviteTokenAttribute(?string $inviteToken): void
    {
        if ($inviteToken === $this->invite_token) {
            // Guard against redundant setters (->fill etc)
            return;
        }

        $this->attributes['invite_token'] = $inviteToken;

        if ($inviteToken === null) {
            $this->invited_at = null;

            return;
        }

        $this->invited_at = Carbon::now();
    }
}

class BaseModel extends Model <- Model is Laravels default Model

traits EntrustUserTrait {
    /**
     * Many-to-Many relations with Role.
     *
     * @return \Illuminate\Database\Eloquent\Relations\BelongsToMany
     */
    public function roles()
    {
        return $this->belongsToMany(Config::get('entrust.role'), Config::get('entrust.role_user_table'), Config::get('entrust.user_foreign_key'), Config::get('entrust.role_foreign_key'));
    }

Running rector

7) Entities/User.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -41,6 +41,22 @@
     UserResolver,
     EntrustUserInterface
 {
+    /**
+     * @var mixed
+     */
+    public $name;
+    /**
+     * @var mixed
+     */
+    public $roles;
+    /**
+     * @var mixed
+     */
+    public $invite_token;
+    /**
+     * @var Carbon
+     */
+    public $invited_at;
     use Authenticatable;
     use CanResetPassword;
     use EntrustUserTrait;
    ----------- end diff -----------

Applied rectors:

 * Rector\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector
 * Rector\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector

name, invite_token and invited_at are all attributes and roles is a relation, called via Laravel through __get

Another example

Another example, which is a different case and a tad harder I suppose.
Our ServiceProvider registers a macro on the Collection class of Laravel:

class ServiceProvider {
    /**
     * @return void
     */
    private function registerCollectionMacros(): void
    {
        Collection::macro('ksort', function (): Collection {
            // macros callbacks are bound to collection so we can safely access
            // protected Collection::items
            $list = $this->items;
            ksort($list);

            return new static($list);
        });
    }

which totally understandable shows the following diff:

Rector v0.5.5
Config file: /my-project/rector.yml

 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

1 file with changes
===================

1) Providers/ServiceProvider.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -90,6 +90,10 @@
 class ServiceProvider extends ModuleServiceProvider
 {
     /**
+     * @var
+     */
+    public $items;
+    /**
      * @return void
      */
     public function boot(): void
    ----------- end diff -----------

Applied rectors:

 * Rector\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector
 * Rector\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector

The problem here is that Laravel calls these registered macro's like this:

return call_user_func_array(Closure::bind(static::$macros[$method], null, static::class), $parameters);

See https://github.com/laravel/framework/blob/master/src/Illuminate/Support/Traits/Macroable.php#L84

This means the closure isn't called with the ServiceProvider bound to $this, but actually the calling class, Collection in this case. I don't see a viable way for rector to detect magic like this, hence ignoring such code from the rule would be one of the few if not only options.

[TomasVotruba]

It looks like 2-3 issuess i at the same time. Could your create new issues with separated content?
It makes much easier to orientate and solve one at a time.

[TomasVotruba]

@JanMikes "Is there a bug? Ignore it :)" This architecture descision actualy lead to creating crappy and useless sniff like slevomat type declarations. It's not fault of the sniff's author, but rather of the tool architect.

It always reminds me this: https://github.com/hmlb/phpunit-vw :laughing:

[Levivb]

@TomasVotruba Sure. Will do
See: https://github.com/rectorphp/rector/issues/1594
See: https://github.com/rectorphp/rector/issues/1595

This report began as feature request but kinda ended up as bug report.

In the end, code varies wildly and a lot of edge cases aren't yet discovered and might not be fixable in any sane way or without introducing a lot of false negatives. Often one is subject to the tools, code, structures and frameworks which work in a way which can't easily be 'fixed' (not to say that's its wrong in the first place).

Since this seems to be a pretty new tool which has yet to adopted by the greater public I suspect a whole lot more requests to ignore certain files/lines are yet to come.

It always reminds me this: https://github.com/hmlb/phpunit-vw 😆

omg 😡. Let it be never heard of again

[TomasVotruba]

Since this seems to be a pretty new tool which has yet to adopted by the greater public I suspect a whole lot more requests to ignore certain files/lines are yet to come.

Sure, if this would be the case and there would be no other way to work with this, it would be right time to introduce such feature.

But this is cleary a bug that needs to be solved.

I'd put more reponsibility on Rector being correct (not breaking stuff), rather then developer being able to ignore every possible bug in config. CI time scales much better than human attention.

[TomasVotruba]

Closing as bugs are reported in stadnalone issues

[Levivb]

Agreed. That's a good point.

I'm interested in seeing how this goes :)

[ravanscafi]

I would love this feature of excluding specific files from specific rules.
I'm trying to run rector on a really big project and I either have to totally exclude the file or the rule.
Bugs will happen, also I might want to apply specific rules to specific folders. (e.g. don't enforce some stuff on tests)

Please revive this issue @TomasVotruba :wink:

[TomasVotruba]

Allright :) I'm open to PR