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

A005 stdlib-module-shadowing warns that utils.logging shadows python logging module #15399

Open
dwt opened this issue Jan 10, 2025 · 9 comments · May be fixed by #15951
Open

A005 stdlib-module-shadowing warns that utils.logging shadows python logging module #15399

dwt opened this issue Jan 10, 2025 · 9 comments · May be fixed by #15951
Assignees
Labels
help wanted Contributions especially welcome rule Implementing or modifying a lint rule

Comments

@dwt
Copy link

dwt commented Jan 10, 2025

This rule warns for me that utils.logging shadows stdlib logging.

Is this intentional? The whole point why our logging module is in utils is so the names do not collide. Am I missing something?

@MichaReiser MichaReiser added question Asking for support or clarification rule Implementing or modifying a lint rule labels Jan 10, 2025
@MichaReiser
Copy link
Member

Hi. Thanks for opening the question.

This seems to be matching the upstream behavior (more as a note to myself rather than this being a justification why):

https://github.com/gforcada/flake8-builtins/blob/5484162ace8d9546513be710d70e9c148010d9cc/flake8_builtins.py#L292

I'm not 100% sure if this is the right behavior or if the rule is too eager to warn. IMO, the rule should warn if your module is named logging because it then shadows the stdlib module. Naming it utils.logging seems less problematic to me. @AlexWaygood am I overlooking something?

@mattmess1221
Copy link

I think this rule should only trigger for modules if there isn't an __init__.py file in the directory OR any module in the package is executable. e.g. it has a shebang or if __name__ == '__main__': check.

This is the only way I can see module shadowing being possible, as doing so prepends the parent directory to sys.path, overriding the stdlib.

@MichaReiser
Copy link
Member

I discussed this with @AlexWaygood in our 1:1 and we think that we should change the default to only flag modules that have the exact same full qualified name as a standard library module and instead, add an option to enable the more strict linting that flags modules that only have the same name.

The motivation for an option is that some projects might want stricter behavior to guarantee that modules don't shadow builtins when running a module from a subdirectory (e.g. cd into the utils directory and run a script from there).

@MichaReiser MichaReiser added help wanted Contributions especially welcome and removed question Asking for support or clarification labels Jan 13, 2025
@InSyncWithFoo
Copy link
Contributor

Somehow I can't replicate the problem.

  1. Library layout:

    .
    ├── pyproject.toml
    └── src
        └── test
            ├── __init__.py
            └── utils
                └── logging.py
    
    $ uvx ruff check --isolated --select A005 src
    All checks passed!
  2. App layout:

    .
    ├── hello.py
    ├── pyproject.toml
    └── utils
        └── logging.py
    
    $ uvx ruff check --isolated --select A005
    All checks passed!

This is perhaps because package is None is such cases.


One thing to note is that package is always Root and never Nested, even for this straight-out-of-the-docs case where baz/__init__.py is passed directly to Ruff:

.
├── README.md
├── pyproject.toml
└── src
    └── foo
        ├── __init__.py
        └── bar
            └── baz
                └── __init__.py

Meanwhile, test_contents uses a hardcoded Root call.

Is this a bigger bug?

@martina-oefelein
Copy link

It triggers if util is a regular package (contains an __init__.py):

.
├── pyproject.toml
└── src
    └── test
        ├── __init__.py
        └── utils
            ├── __init__.py
            └── logging.py
$ uvx ruff check --isolated --select A005 src
src/test/utils/logging.py:1:1: A005 Module `logging` shadows a Python standard-library module

@InSyncWithFoo
Copy link
Contributor

Thanks. In that case, this has something to do with the "never-Nested" problem above.

@scott-boost
Copy link

├── pyproject.toml
└── AcmeOrg
    └── http
        ├── __init__.py
        └── utils
            ├── __init__.py
            └── logging.py

interesting that flake8 didnt trigger A005 in this setup BUT ruff did
(notice that AcmeOrg doesnt contain a __init__.py file)

@rh-sp
Copy link

rh-sp commented Jan 24, 2025

Opinion

I think that the current rule is too strict, but "only fully qualified names" sounds like it could be too loose, depending on how it's interpreted. In my opinion, any root-level packages that are named the same as any stdlib root-level package should be blocked, along with all their child packages.

Example

  • abc
  • collections
    • abc
    • foobar
  • foobar ✅
    • abc ✅
    • collections ✅
      • abc ✅
  • urlparse ✅

Reasoning

I haven't been around then, but from what I've heard, in the olden days, it wasn't well-defined what import abc does when in a package. Does it refer to the root-level package abc, or the abc defined at the current package level? So in that sense, for older Pythons, it made sense to flag foobar.abc as potentially shadowing abc.

However, in modern Python, this issue has been resolved. import abc always refers to the root-level package, never foobar.abc (or collections.abc). At the same time, .abc (which can stand for foobar.abc or collections.abc, depending on context) can never shadow abc. So, foobar.abc does not need to trigger the rule.

Additionally, collections.foobar should also trigger the rule, even though the fully qualified name doesn't exist in stdlib. This is because if you import collections.foobar, while the import statement itself can only mean one thing, from what I understand in order for it to work, collections must be shadowed, so the problem is still there.

Hope that makes sense :)

@flying-sheep
Copy link
Contributor

import abc always refers to the root-level package, never foobar.abc (or collections.abc). At the same time, .abc (which can stand for foobar.abc or collections.abc, depending on context) can never shadow abc. So, foobar.abc does not need to trigger the rule.

Yeah definitely. This rule has a lot of false positives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions especially welcome rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants