bug: False positive on used imports when docblock includes it with mismatching case

[bradietilley]

Partially addressing #2814.

---

I see this issue crop up about once a week if not more.

There are two tests in PHP-CS-Fixer, one that asserted case-sensitive matching in docblocks, and one that asserted case-insensitive matching in docblock. The result from both is that the imports are retained, albeit unused.

I understand the correct capitalisation of a class name should be treated as a direct reference to the class, and thus it is used. However, the incorrect capitalisation of a class name should not be treated as a direct reference to the class, and thus it should be removed as an import.

Frequency

Single-word class names are extremely common in PHP. For example Illuminate\Support\Collection, Illuminate\Http\Request, Illuminate\Http\Response, Illuminate\Support\Facades\File, App\Models\User, App\Models\Role, App\Models\Product, App\Models\Invoice.

Mentioning these single-word class names in their abstract sense will be extremely common if you're planning on (or are) using the class in your code. For example, "do something if the invoice has no product attached" references Invoice and Product as of currently.

If you never end up using the class(es) that you imported, or you end up refactoring the code and remove those classes but retain lowercase reference to the class(es) in docblock comments, then you'll be faced with this issue.

So it's quite easy: First you add a single-word class name, then talk about the abstract idea, then remove (or never add the class to your code) then run php-cs-fixer.

Example Issue + Example Fix

<?php

use App\Models\User;
use App\Models\Role;

class Something
{
    /**
    * Do something as the authorised user depending on their role
    * 
    * See Role for more info
    */
    public function doSomething(): void
    {
        //
    }
}

Before this PR, the above would have no change applied. The User import would remain imported as user is found in the doblock.

After this PR, the below would occur:

<?php

- use App\Models\User;
use App\Models\Role;

class Something
{
    /**
    * Do something as the authorised user depending on their role
    * 
    * See Role for more info
    */
    public function doSomething(): void
    {
        //
    }
}

Note: correct capitalisation of Role which is likely to be a reference to the Role class is retained.

Bug or feature?

I don't know about the thousands of other developers who use php-cs-fixer, but I would expect the import to be removed as it is in no way used in the code. If you want it retained, use the correct capitalisation.

Impact

Low impact as this only affects files where the imported class is ONLY referenced using incorrect capitalisation. So, the class is not used in any code, for any type hints, and is only referenced in a capitalisation that really doesn't correlate to the class at all.

[Wirone]

In my opinion import usage should be limited to actual types or phpDocs (signature related stuff), annotations etc. Words used in description, params' comments etc. shouldn't be considered as "import usage".

Looking at your example, I wouldn't consider See Role for more info as [App\Models\]Role usage. People have various styles of documenting, some random words matching imported classes can't be considered as usage.

[bradietilley]

I wouldn't consider See Role for more info as [App\Models]Role usage.

Oh, for sure, I don't consider any plain-text as a reference to a class either.

I've just retained the case-sensitive portion as:

• People might be relying on that because it's always been there
• 99% of the time I run into this issue it trips up on lowercase class references so to me the only problem is lower case
• I couldn't be bothered scouring to see how much of a refactor it would be to remove case sensitive matching as well.

Happy to adjust this PR to remove comments matching class names entirely (i.e. even case-sensitive) if that's the preferred approach.

[Wirone]

I believe re-working NoUnusedImportsFixer the way we discussed is too hard to continue under bug PR, and I am not even sure if we can consider current approach as a bug (rather improper feature?). I really would like to introduce better way of cleaning imports, but probably we just can't change it just like that and we need hide new approach behind opt-in configuration flag, which should be done in separate PR.

On the other hand, changed case-sensitivity like it was done here IMHO can be considered as bug fix, because I am sure words with different casing shouldn't be treated as class "usage" (even if PHP itself is case-insensitive in that regard).

Anyway, I won't merge it, I would like to hear opinion from @kubawerlos, @keradus 🙂.

[Wirone]

Thank you @bradietilley 🍻

[mvorisek]

@Wirone Thank you for this PR and I support it, but shouldn't it be marked as risky? It is very risky if applied on badly coded, never fixed code base!

see #6486 discussion

[Wirone]

@mvorisek can you provide examples why it's risky? I tried to get familiar with the issue and previous discussion when I did a review and at that point I didn't see the potential problem. I don't have time to re-read it again. I believe we can just observe how it goes 🙂.

[mvorisek]

repro https://3v4l.org/DCEmg - the use statement is now (as of v3.18) removed, I propose to mark this fixer as risky and add a configuration option or have two fixers, one CS (non-risky) and another CI (risky). When a large projects is migrated/fixed, you want to be able to run non-risky fixers first and rely on them to not make risky fixes.

[SpacePossum]

Making it risky will be a BC break if people have the rule in their configuration together with the option allowRisky set to false
as the tool will than no longer run. I think the change should be reverted so the rule remains not-risky and does not break peoples code.

[mvorisek]

Yes, revert now, release 3.18.1 and reintroduce the CS variant as another fixer (as one fixer cannot be risky only sometimes).

[Wirone]

Decision (and the potential patch release) is up to @keradus 🙂.

[keradus]

Hey folks, please avoid setting me under the wall like this.
I am not familiar of this PR, discussion in it or problem it solved. I was not the one deciding about merge , don't ask me to decide about unmerge.
If you consider the merge harmful, please revert. If not, please keep

[Wirone]

@keradus you said the other day you test everything before release, so I believe you're familiar enough with the change 😉. All that needs to be decided here is if we should consider usage described above as something we want to support (keep it safe). Merged PR solves other scenarios, so it is valuable for other people. Those with such ancient code that uses eval() may decide to opt-out from this fixer if it breaks something for them... Or fix code after Fixer's changes 😅.

If it was for me, I would keep this fixer as-is (in v3.18), and observe the real impact, not theoretical.

[mvorisek]

The problem is with any code, not just eval, I used evel to reproduce it easily in one file..., Simply any use statement not matching the exact case is removed which is definitively dangerous and should be definitively marked as risky. So I feel it should be reverted, released and then another risky fixer introduced.

[Wirone]

In contrary, I believe this change can help people spot problems with wrong casing in the imports/usage and make their code better, while fixing the bug for other people (keeping unneeded imports because of case-insensitive match). That's why I would keep it as-is and would like to hear @keradus opinion about that.

[mvorisek]

@Wirone definitely, I am for it, but I am saying it MUST be marked as risky (or introduced as another fixer marked as risky).