fix: Import only unique symbols' short names

[Wirone]

Fixes #7632 (hopefully, but at least it will prevent changes like these done in symfony/symfony#53244)

This is quick fix for obvious bug, but I am wondering if we could provide feature for auto-aliasing imports with colliding short names (e.g. when \Foo\A and \Bar\A are found we could add use Foo\A as FooA; and use Bar\A as BarA) 🤔. Anyway, this is something for separate PR.

[coveralls]

coverage: 94.778%. remained the same
when pulling e63cdf6 on Wirone:codito/7632-fqcn-import-symbol-name-clashes
into 826207a on PHP-CS-Fixer:master.

[keradus]

non-blocking

I took a look how this uses is used and passed around most of the methods.
maybe it could be state property and avoid pass-by-reference in all the methods 😅

[Wirone]

If anything, I would prefer creating some object representing a list of imports, so it's always passed by reference. Explicit arguments are better than quasi-global state.

[mvorisek]

This is quick fix for obvious bug, but I am wondering if we could provide feature for auto-aliasing imports with colliding short names (e.g. when \Foo\A and \Bar\A are found we could add use Foo\A as FooA; and use Bar\A as BarA) 🤔. Anyway, this is something for separate PR.

until then, no importing should be done, as:

• later, I do not expect the fixer to rename A to FooA
• "Bar\A" could be must more sensual to be imported as A later than "Foo\A"

[Wirone]

until then, no importing should be done (...)

I don't agree. All possible FQCNs should be imported (in this case only first conflicting name). Nothing is going to be renamed later, because for multiple A symbols only first is imported and will remain untouched later. Only subsequent A symbols are ignored right now, but later we can provide ability to imported them too, but as aliased imports.

[mvorisek]

please reread my post, my point is the the 2nd symbol might be preffered to be imported as A but with the current idea - import only the first unique name, this is impossible to catch in a review process

[Wirone]

Fixer does not have ability to determine "sensuality" of the imports, so even if we provide aliasing feature, it would still require manual work to select which import should be non-aliased. So even if first symbol is imported now, and the other will be later, then you could catch it on the second review (because you'll see the imports are aliased, so you can compare it against non-aliased one). Delaying imports for conflicting names, as you suggested, won't fix this because you just can't choose which symbol should be imported first, it's just the first one found and processed. We would need to have an config option for choosing which parts of code should be processed (signatures, extends, implements etc), but even with such granularity it wouldn't be possible to ensure proper importing order for all cases. So, if manual work is required anyway, I would suggest you just should run check first, see potential changes, manually do "more sensual" import, and then apply fixes 🙂.

[mvorisek]

Delaying imports for conflicting names, as you suggested, won't fix this because you just can't choose which symbol should be imported first, it's just the first one found and processed

it would help, because you will see 2 aliased imports instead of 1 unaliased and one 1 aliased (especially, later only the 1 aliased, as users will fix/commit the 1st unaliased sooner because of the current behaviour)

[Wirone]

it would help, because you will see 2 aliased imports instead of 1 unaliased and one 1 aliased

No, you would see 1 regular and 1 aliased import, because imports are registered in order when they're found and if short name is not already imported, then it's imported without alias (because it's not needed at this point). Yes, actual imports are created after analysis, when all symbols are collected, so we can determine name collisions and do something about it, but I don't see the point in importing all of them as aliased because it would require the exact same manual work to un-alias one of them, like you would need to do when symbol that was imported first turn out to be "less sensual" (and you need to alias it to be able to use non-aliased import for other symbol). I think you're overcomplicating something that in any scenario requires similar, manual work.