pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.24k stars 1.12k forks source link

__all__ is ignored for wildcard imports #3089

Open SeppMe opened 5 years ago

SeppMe commented 5 years ago

This might be related to https://github.com/PyCQA/pylint/issues/1312 but results in a different diagnostic.

I have a file foo.py:

import os

__all__ = ['Foo']

class Foo:
    pass

and an __init__.py:

from .foo import *  # PyLint knows that wildcard imports are ok in __init__.py

def _my_secret_private_function()
    import os
    pass

PyLint will falsely diagnose "Redefining name 'os' from outer scope (line 1) (redefined-outer-name)" in the line where os is imported in __init__.py, despite that name not being available at that point due to the __all__ attribute in foo.py.

PyLint version is 2.3.1, astroid is 2.2.5,

PCManticore commented 5 years ago

Thanks for reporting an issue. Unfortunately we decided a couple of years ago to not consider __all__ any longer as we had trouble following the corresponding identifiers in the situations where __all__ was modified dynamically (which apparently a lot of libraries do). As such, regardless of what is present in __all__, all identifiers that can be viewed from an import are taken in consideration when emitting an import related error. It's something that's not worth fixing given the plethora of false positives that we'll have if we'd start handling __all__ again.

sevdog commented 2 years ago

Can this be reconsidered?

It is not a good way to have to disable redefined-outer-name every time there is a wildcard import since it may shadow a real redefinition if by any means a new element is added.

DanielNoord commented 2 years ago

@sevdog Although I was not part of the original discussion the issue of dynamically changing __all__ and pylint inferring these changes still stands. We don't have any good control flow recognition in pylint so we don't really know what is and what isn't in a tuple at any given point. Seeing as the underlying problem still exists I don't think this would be on the horizon anytime soon. Sorry about that!

sevdog commented 2 years ago

Just to have an other case which I encountered to be considered.

# file a.py
import datetime
__all__ = ('not_datetime',)

# file b.py
from datetime import datetime
from .a import *

d = datetime(2022, 1, 1)  # raises E1102 (not-callable) 

The error E1102 is reaise because pylint is ignoring the __all__ variable of first package and thus belives it shadows datetime in the second-one while this does not happen! This can be avoided by switching imports, but this will go againsts PEP-8 recommends (place first standardlib imports). This case was particular annoying since in standardlib datetime is both the name of a package than a class defined in that package, but it may happen in other cases.

I hope that the problem with wildcard import should be resolved.

Pierre-Sassoulas commented 2 years ago

I don't think it's worth it because importing with wildcards is discouraged anyway for obvious reasons (it's confusing, and hard to reason about). pylint has trouble understanding what's happening but a human would also have trouble doing it.

If someone want to fix it, we'll review and merge but we have a lots of considerably higher priority issues.

OJFord commented 2 years ago

we decided a couple of years ago to not consider __all__ any longer as we had trouble [...] where all was modified dynamically

@PCManticore - why not consider it only when immutable then?

e.g. OP's ['Foo'] wouldn't work, but ('Foo',) would.

SubaruArai commented 1 year ago

@PCManticore Can you reopen this issue? @OJFord 's solution seems a good compromise.

SubaruArai commented 1 year ago

Thank you!