Repository navigation
A001 and A003 should ignore variables like __doc__ #16373
Description
Activity
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.
- addedbugAn issue describing something that isn't working, or a PR that fixes a bugAn issue describing something that isn't working, or a PR that fixes a bug
on Feb 25, 2025 I'm undecided on whether we should
- 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 - 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
- Say that shadowing
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__meansbuiltins.__doc__. Even if a module doesn’t explicitly set__doc__or use a docstring, it still has a__doc__that shadowsbuiltins.__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 thatbuiltinsdoesn’t provide:__file__,__cached__, and__path__.Reacted by Alex WaygoodFair enough, that's all pretty convincing!
Hey,
I opened a PR to fix the issue for ruleA001. Let me know if I correctly understood the final outcome of the discussion! :-)Also, should this apply to rule
A003as well?I have sometimes assigned directly to
__doc__to change its type in type-checkers fromstr | Nonetostr, without also having to assert it: microsoft/pyright#9404I 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.
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.- added a commit that references this issue
on Mar 3, 2025
Summary
builtin-variable-shadowing(A001) andbuiltin-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.Getting a false positive for A003 is more contrived but still possible. In the following example,
C.__doc__is not shadowingbuiltins.__doc__. (It does shadowa003.__doc__but that is okay because classes are allowed to have docstrings.)Version
ruff 0.9.7 (54fccb3 2025-02-20)