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

[dwt]

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]

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]

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]

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).

[InSyncWithFoo]

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]

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]

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

[scott-boost]

├── 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]

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]

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.

[dwt]

🎉

[glopesdev]

@ntBre We are having the opposite problem where our CI started emitting more warnings following the latest release of ruff, in places where it wasn't emitting before.

Is there documentation somewhere explaining how exactly to turn on the new "default" behavior?

[glopesdev]

For those randomly checking here, the new config for pyproject.toml should be:

[tool.ruff]
lint.flake8-builtins.builtins-strict-checking = false

Still not clear to me why an issue which was supposed to reduce the number of default built-in warnings ended up producing more warnings under the default configuration.

[ntBre]

It will be a breaking change to update the stable default, so that will have to wait for the next minor version (0.10.0). In preview mode, the default should already be lint.flake8-builtins.builtins-strict-checking = false.

But yes, as part of this change there was also a bug fix to bring our implementation of A005 in line with the upstream, which does produce more warnings in some cases now.

[ntBre]

Oh and yes, that was my mistake on the documentation. I have an open PR (#16097) to make that easier to find that will hopefully be merged today!

[ntBre]

There's also a related breaking change for 0.10.0 to make the new option and the other flake8-builtins options less verbose in #16092. In this case, I think the new option name will be lint.flake8-builtins.strict-checking, but this is not currently available in preview.