False negative reimported when using an import alias

[ddangu525]

Current problem

I think it would be nice if Pylint could check if there is same import name from different packages.
For example, in the code below, import order is fine, but the 'Path' imports overlap, causing errors somewhere.

from pathlib import Path
from FastAPI import Path

Desired solution

overlapping imports found. Path

Additional context

No response

[Pierre-Sassoulas]

Thank you for your proposition, it makes sense.

[DudeNr33]

The given example already triggers the message reimported: Reimport 'Path' (imported line 8).
What is not covered yet are aliased imports, e.g.:

from pathlib import Path
from some_other_lib import CustomPath as Path

or

from pathlib import Path
import FastAPI.Path as Path

Reading the help for reimport, I am unsure if this is check actually is the right place for it:
"Used when a module is reimported multiple times."
The docstring for the method which handles the check also implies that this check is not intended for checking if the same name is imported multiple times, but if the import is necessary:

def _check_reimport(self, node, basename=None, level=None):
        """check if the import is necessary (i.e. not already done)"""

@Pierre-Sassoulas what is your opinion on this?
Is the fact that reimport triggers on the first example an actual false-positive and we should add a new check (was thinking about shadowed-import)?
Or should we adjust the help/docstring of reimport to state that it handles the case where the same names are imported multiple times?

[DanielNoord]

I think we should just adjust the docstring and description of the message. I think its intent is pretty clear and also follows what I would expect it to do.

That makes this a double issue:

1. Fix docstring and description of the message
2. Fix the false negative

[Pierre-Sassoulas]

Should we also rename the message from reimported to reimported-or-shadowed ?

[DanielNoord]

I think that creates unnecessary churn and old-message stuff. In the end the offending line still contains the word import so it is clear that the message is related to "importing and everything related to it".

[clavedeluna]

I'll work on this

[Pierre-Sassoulas]

@DanielNoord I think reimported is different than something like import-name-clashor ``reimport-shadowing", "reimported" feel like what should be raised only for:

from pathlib import Path
from pathlib import Path

Which is a lot less problematic than shadowing another import like in:

from pathlib import Path
from FastAPI import Path

I think we should separate the two use case, with a different message for each, what do you think ?

[DanielNoord]

I'm not sure. Why would you ever want to enable one of those but not the other? They both warn of significant code smells that seem very related.

I'm not against it, but just feel like unnecessary work with minimal benefit. We can focus on many other issues.

[Pierre-Sassoulas]

Let's separate the two checks between reimported and shadowed-import like @DudeNr33 suggested in its original message. Unclear message and checker doing multiple things means user have a hard time understanding and we change the check more and create more doc to accommodate the multiple things it needs to do or explain. This is more work in the end.

[LLyaudet]

Hello,
I just had a similar case with:

from some_module import something

...
def bla():
    def blabla():
        from some_module import something as something_
        ...
    # use blabla

In some very rare cases, it may make sense to add another alias in this kind of generated functions.
But after thinking about it first and reading this issue,
I think it is more convenient to have the warning as is done now in current pylint version,
and add a pylint disable comment when this is really necessary.

Best regards,
Laurent Lyaudet