Php-cs-fixer: NoUnusedImportsFixer - Ignore references in comments

[julienfalque]

no_unused_imports fixer searches for references to the imported elements almost everywhere in the code, including comments. This sometimes leads to not removing use statements that are unused because the same word was found in a comment, despite the comment being unrelated. See https://github.com/symfony/symfony/pull/22962#discussion_r119100703 for an example.

This check is done using a regular expression on the PHP code, which is the reason why the fixer can find false positives quite easily. IMO this is a bug and we should improve the regular expression.

But if we consider this a behavior change, then it would be nice to introduce a strict mode via a configuration option. When enabled, the fixer would only check usage of the imported elements in actual code and ignore comments completely (or look for PHPDocs/custom annotations only, e.g. @param Foo or @Annotation()).

[SpacePossum]

so skip comments all together and analyze PHPDocs?

[GrahamCampbell]

👍 Would be good to be able to turn on case sensitive only matching in phpdoc too.

[julienfalque]

so skip comments all together and analyze PHPDocs?

I would add an option that accepts multiple values to specify where the fixer should search for usages:

• code: actual PHP code usage of the imported elements;
• phpdoc_types: usages in PHPDoc types in annotations like @param, @return and so on;
• phpdoc_descriptions: references in PHPDoc summaries and descriptions;
• annotations: usages like @Foo, @Bar\Baz, ... in DocComments;
• comments: references in comments not covered by previous values.

By default it would have all these values to keep current behavior.

WDYT?

Would be good to be able to turn on case sensitive only matching in phpdoc too.

Not sure about that. PHP itself isn't case sensitive so I think this fixer should remain case insensitive.

[keradus]

:+1: for both options.
even if php doesn't give a shit about casing, there are plenty of devs who do. yet indeed, option disabled by default

[GrahamCampbell]

Not sure about that. PHP itself isn't case sensitive so I think this fixer should remain case insensitive.

even if php doesn't give a shit about casing, there are plenty of devs who do. yet indeed, option disabled by default

The php autoloader is case sensitive if you are on a case sensitive filesystem though.

[GrahamCampbell]

So, there are instances when the casing of a class does matter, is my point. The convention is also that casing matters, as @keradus elegantly said. :trollface:

[ellisio]

Just stumbled on this trying to figure out why a use is not being removed. I expect the following:

<?php

namespace App\Http\Controllers;

use App\Http\Requests\StoreRequest;
use Illuminate\Http\Request;

class StoreController extends Controller
{
    /**
     * POST /store
     *
     * @param \App\Http\Requests\StoreRequest $request
     */
    public function __invoke(StoreRequest $request)
    {
        // Do something
    }
}

To be drop the unused \Illuminate\Http\Request like so:

<?php

namespace App\Http\Controllers;

use App\Http\Requests\StoreRequest;

class StoreController extends Controller
{
    /**
     * POST /store
     *
     * @param \App\Http\Requests\StoreRequest $request
     */
    public function __invoke(StoreRequest $request)
    {
        // Do something
    }
}

However, due to the phpdoc having $request it fails. This is _only_ because of the phpdoc. The $request argument on the __invoke() line does not impact this rule.

As soon as I change, or remove, $request in the phpdoc the fixer works as expected.

[SpacePossum]

thanks for reporting @ellisio , your case can be fixed independent of this issue, please see https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/4736