Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

false positive 'useless-import-alias' error for mypy-compatible explicit re-exports #6006

Closed
jab opened this issue Mar 27, 2022 · 11 comments · Fixed by #6151 or #8124
Closed

false positive 'useless-import-alias' error for mypy-compatible explicit re-exports #6006

jab opened this issue Mar 27, 2022 · 11 comments · Fixed by #6151 or #8124
Labels
Enhancement ✨ Improvement to a component Needs decision 🔒 Needs a decision before implemention or rejection Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc.

Comments

@jab
Copy link
Contributor

jab commented Mar 27, 2022

Bug description

Suppose a package has the following layout:

package/
  _submodule1.py  # defines Api1
  _submodule2.py  # defines Api2
  __init__.py     # imports and re-exports Api1 and Api2

Since the submodules here implement public APIs, __init__.py imports and re-exports them, expecting users to import them from the public, top-level package, e.g. from package import Api1.

Since the implementations of Api1 and Api2 are complex, they are split into _submodule1.py and _submodule2.py for better maintainability and separation of concerns.

So __init__.py looks like this:

from ._submodule1 import Api1 as Api1
from ._submodule2 import APi2 as Api2

The reason for the as aliases here is to be explicit that these imports are for the purpose of re-export (without having to resort to defining __all__, which is error-prone). Without the as aliases, popular linters such as mypy will raise an "implicit re-export" error (docs -- part of mypy --strict).

However, pylint does not currently understand this usage, and raises "useless-import-alias" errors.

Example real-world code triggering pylint false positive errors: https://github.com/jab/bidict/blob/caf703e959ed4471bc391a7794411864c1d6ab9d/bidict/__init__.py#L61-L78

Configuration

No response

Command used

pylint

Pylint output

************* Module bidict
bidict/__init__.py:61:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:61:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:62:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:62:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:62:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:63:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:63:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:64:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:65:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:66:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:66:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:67:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:68:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:69:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:69:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:69:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:70:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:70:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:70:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:70:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:70:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:71:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:71:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:72:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:72:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:72:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:73:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)
bidict/__init__.py:74:0: C0414: Import alias does not rename original package (useless-import-alias)

Expected behavior

No "useless-import-alias" errors should be flagged.

Pylint version

pylint 2.13.2
astroid 2.11.2
Python 3.10.2 (main, Feb  2 2022, 07:36:01) [Clang 12.0.0 (clang-1200.0.32.29)]

OS / Environment

No response

Additional dependencies

No response

@jab jab added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Mar 27, 2022
@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Mar 27, 2022

The reason for the as aliases here is to be explicit that these imports are for the purpose of re-export (without having to resort to defining all, which is error-prone).

I think __all__is the way to be explicit about the API of a module.That way you have the API documented in one place at the top of the module without having to check what exactly is imported with import x as x. I never heard about import x as x and never did the implementer of the check, but I saw the mypy documentation you linked, let's see how widely used this is.

@Pierre-Sassoulas Pierre-Sassoulas added Discussion 🤔 False Positive 🦟 A message is emitted but nothing is wrong with the code and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Mar 27, 2022
@cdce8p
Copy link
Member

cdce8p commented Mar 27, 2022

I don't think there is a way for pylint to detect if the reexport is intended or not. Maybe we could ignore __init__.py files 🤔 However, that might be unexpected to the general user.

Probably the easiest solution in your case would be to add a module level pylint: disable=useless-import-alias (before any imports).

@jab
Copy link
Contributor Author

jab commented Mar 28, 2022

Yeah, other linters like mypy specifically support as rather than just __all__ for this, since so many people have been burned by using __all__.

__all__ requires maintaining exports as a list of strings, which are all-too-easy to typo (and often tools and IDEs can’t detect when this happens), and also separately (and often far away) from where they’re imported / defined, which is also fragile and error-prone.

@Pierre-Sassoulas
Copy link
Member

tools and IDEs can’t detect when this happens

Yeah I remember when I used liclipse (eclipse + pydev) this was a pain. This is an issue with the IDE though, Pycharm Community Edition handle this correctly.

@jab
Copy link
Contributor Author

jab commented Mar 28, 2022

Sure, some IDEs can help with typos in __all__, but that's only one part of the problem with __all__. More problematic is that it forces you to maintain exports separately from where they're imported / defined, which makes it too easy for __all__ to drift out of sync as changes are made to the intended exports.

@Pierre-Sassoulas Pierre-Sassoulas added Documentation 📗 and removed Discussion 🤔 False Positive 🦟 A message is emitted but nothing is wrong with the code labels Apr 3, 2022
@Pierre-Sassoulas
Copy link
Member

As @cdce8p said, the solution is to disable. I think this is going to stay that way because I don't see how pylint can guess the intent of the implementer. We could make this check optional but I think if you're making library API using this you're more able to disable the check than a beginner making a genuine mistake is to activate it. We're going to document this in the useless-import-alias documentation.

@jab
Copy link
Contributor Author

jab commented Apr 3, 2022

Ok, thanks for that. As usage of mypy and mypy-style explicit re-imports continues to grow, it would be interesting to know how many pylint users end up having to disable useless-import-alias, and whether that amount ever crosses some threshold for being better as an opt-in rather than an opt-out. Not sure how much usage data you collect though for such decisions (e.g. by looking at usage from open source codebases).

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Apr 3, 2022

crosses some threshold for being better as an opt-in rather than an opt-out

An alternative solution would be to not raise this message in __init__.py.

Not sure how much usage data you collect though for such decisions (e.g. by looking at usage from open source codebases).

To be frank, it's mostly opened issues and the thumbs-up / comments those issues gather. I'm not looking specifically at open sources projects for each messages it's really time consuming. A well researched comments on this issue with stats and sources, a proposition that is easily implementable with a better result than what we have currently, or this issue gathering 50+ thumbs up and a lot of attention would definitely make us reconsider.

@jab
Copy link
Contributor Author

jab commented Apr 3, 2022

Got it, good to know.

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.14.0 milestone May 17, 2022
@jab
Copy link
Contributor Author

jab commented Nov 2, 2022

Just noticed https://github.com/microsoft/pyright/releases/tag/1.1.278

Changed the reportUnusedImport check to not report an error for "from y import x as x" since x is considered to be re-exported in this case. Previously, this case was exempted only for type stubs.

One more tool (pyright) flipping in this direction, fwiw.

@Pierre-Sassoulas
Copy link
Member

We'll need an option to exclude __init__ for the check if this become widespread. Reopening in order to not duplicate info.

@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Needs decision 🔒 Needs a decision before implemention or rejection Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. and removed Documentation 📗 labels Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Needs decision 🔒 Needs a decision before implemention or rejection Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc.
Projects
None yet
3 participants