Skip to content

A001 and A003 should ignore variables like __doc__ #16373

Description

@dscorbett

Summary

builtin-variable-shadowing (A001) and builtin-attribute-shadowing (A003) should not report violations for variables that match the names of module attributes.

For example, even though builtins.__doc__ exists, setting __doc__ in a module does not shadow it, because every module already has its own __doc__ whether you set it explicitly or not.

$ cat >a001.py <<'# EOF'
from importlib.machinery import SourceFileLoader
__doc__ = "..."
__name__ = "a001"
__loader__ = SourceFileLoader(__file__, __name__)
__package__ = None
__spec__ = None
# EOF

$ ruff --isolated check --select A001 a001.py --output-format concise
a001.py:2:1: A001 Variable `__doc__` is shadowing a Python builtin
a001.py:3:1: A001 Variable `__name__` is shadowing a Python builtin
a001.py:4:1: A001 Variable `__loader__` is shadowing a Python builtin
a001.py:5:1: A001 Variable `__package__` is shadowing a Python builtin
a001.py:6:1: A001 Variable `__spec__` is shadowing a Python builtin
Found 5 errors.

Getting a false positive for A003 is more contrived but still possible. In the following example, C.__doc__ is not shadowing builtins.__doc__. (It does shadow a003.__doc__ but that is okay because classes are allowed to have docstrings.)

$ cat >a003.py <<'# EOF'
class C:
    __doc__ = "None"
    def f() -> __doc__: ...
# EOF

$ ruff --isolated check --select A003 a003.py --output-format concise
a003.py:3:16: A003 Python builtin is shadowed by class attribute `__doc__` from line 2
Found 1 error.

Version

ruff 0.9.7 (54fccb3 2025-02-20)

Activity

  1. AlexWaygood commented on Feb 25, 2025

    @AlexWaygood
    Member

    These are arguably still antipatterns (I wouldn't recommend for users to shadow these implicit module globals either; it seems ~as bad as shadowing a builtin). But I agree that at the very least, the diagnostic message here is incorrect, since it's not really a builtin symbol being shadowed in these code snippets.

  2. added
    bugAn issue describing something that isn't working, or a PR that fixes a bug
    on Feb 25, 2025
  3. AlexWaygood commented on Feb 25, 2025

    @AlexWaygood
    Member

    I'm undecided on whether we should

    1. Say that shadowing __doc__, __name__ etc. is just outside the purview of these rules (and that it should be reported by another (new) rule), or
    2. Continue to flag these globals being shadowed with these rules, just change the error message when these globals are shadowed so that we don't mention a "builtin" being shadowed
  4. dscorbett commented on Feb 25, 2025

    @dscorbett
    Author

    I support option 1: I don’t think these attributes are within the purview of these rules. The rationale for A001 says:

    Reusing a builtin name for the name of a variable increases the difficulty of reading and maintaining the code, and can cause non-obvious errors, as readers may mistake the variable for the builtin and vice versa.

    That rationale does not apply to these 5 attributes. A reader wouldn’t mistakenly think __doc__ means builtins.__doc__. Even if a module doesn’t explicitly set __doc__ or use a docstring, it still has a __doc__ that shadows builtins.__doc__. Reusing a builtin name doesn’t increase the difficulty of reading or maintaining the code when the builtin isn’t readily accessible anyway. If it’s still an antipattern to assign to them, I’d make a new rule for that, because the rationale is different.

    It might not an antipattern to assign to all of them though. Although it’s better to use a docstring if possible, it’s fine to dynamically compute it and assign it to __doc__ explicitly if necessary. Assigning to __name__ seems common in setup.py, though I don’t know if that’s considered a good idea. The other ones don’t seem useful to set as globals; if there is ever a rule made against that, it should include the similar module attributes that builtins doesn’t provide: __file__, __cached__, and __path__.

  5. AlexWaygood commented on Feb 25, 2025

    @AlexWaygood
    Member

    Fair enough, that's all pretty convincing!

  6. VascoSch92 commented on Mar 1, 2025

    @VascoSch92
    Contributor

    Hey,
    I opened a PR to fix the issue for rule A001. Let me know if I correctly understood the final outcome of the discussion! :-)

    Also, should this apply to rule A003 as well?

  7. Avasam commented on Mar 2, 2025

    @Avasam
    Contributor

    I have sometimes assigned directly to __doc__ to change its type in type-checkers from str | None to str, without also having to assert it: microsoft/pyright#9404

    I guess there's an argument to be made about Python being run in optimized mode removing docstrings. But that also feels out of scope for A001's reasoning and could be mentioned in a replacement rule.

  8. MichaReiser commented on Mar 3, 2025

    @MichaReiser
    Member

    Makes sense to me. We could have a separate rule that warns if you assign to __doc__ that let's you know that this doesn't change the module's doc comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugAn issue describing something that isn't working, or a PR that fixes a bug

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions