mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-08-19 01:50:38 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Restrict builtin-attribute-shadowing to actual shadowed references (#9462)

Browse source 
## Summary

This PR attempts to improve `builtin-attribute-shadowing` (`A003`), a
rule which has been repeatedly criticized, but _does_ have value (just
not in the current form).

Historically, this rule would flag cases like:

```python
class Class:
    id: int
```

This led to an increasing number of exceptions and special-cases to the
rule over time to try and improve it's specificity (e.g., ignore
`TypedDict`, ignore `@override`).

The crux of the issue is that given the above, referencing `id` will
never resolve to `Class.id`, so the shadowing is actually fine. There's
one exception, however:

```python
class Class:
    id: int

    def do_thing() -> id:
        pass
```

Here, `id` actually resolves to the `id` attribute on the class, not the
`id` builtin.

So this PR completely reworks the rule around this _much_ more targeted
case, which will almost always be a mistake: when you reference a class
member from within the class, and that member shadows a builtin.

Closes https://github.com/astral-sh/ruff/issues/6524.

Closes https://github.com/astral-sh/ruff/issues/7806.
This commit is contained in:
Charlie Marsh 2024-01-11 12:59:40 -05:00 • committed by GitHub
parent 7fc51d29c5
commit 25bafd2d66
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 167 additions and 272 deletions
Download patch file Download diff file Expand all files Collapse all files

50
crates/ruff_linter/resources/test/fixtures/flake8_builtins/A003.py vendored
View file

@ -8,46 +8,14 @@ class MyClass:
self.id = 10
self.dir = "."
def str(self):
pass
from typing import TypedDict
class MyClass(TypedDict):
id: int
from threading import Event
class CustomEvent(Event):
def set(self) -> None:
...
def str(self) -> None:
...
from logging import Filter, LogRecord
class CustomFilter(Filter):
def filter(self, record: LogRecord) -> bool:
...
def str(self) -> None:
...
from typing_extensions import override
class MyClass:
@override
def str(self):
pass
def int(self):
pass
def str(self):
pass
def method_usage(self) -> str:
pass
def attribute_usage(self) -> id:
pass

16
crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs
View file

@ -7,7 +7,8 @@ use crate::checkers::ast::Checker;
use crate::codes::Rule;
use crate::fix;
use crate::rules::{
flake8_pyi, flake8_type_checking, flake8_unused_arguments, pyflakes, pylint, ruff,
flake8_builtins, flake8_pyi, flake8_type_checking, flake8_unused_arguments, pyflakes, pylint,
ruff,
};
/// Run lint rules over all deferred scopes in the [`SemanticModel`].
@ -27,6 +28,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
Rule::UndefinedLocal,
Rule::UnusedAnnotation,
Rule::UnusedClassMethodArgument,
Rule::BuiltinAttributeShadowing,
Rule::UnusedFunctionArgument,
Rule::UnusedImport,
Rule::UnusedLambdaArgument,
@ -297,6 +299,18 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
ruff::rules::asyncio_dangling_binding(scope, &checker.semantic, &mut diagnostics);
}
if let Some(class_def) = scope.kind.as_class() {
if checker.enabled(Rule::BuiltinAttributeShadowing) {
flake8_builtins::rules::builtin_attribute_shadowing(
checker,
scope_id,
scope,
class_def,
&mut diagnostics,
);
}
}
if matches!(scope.kind, ScopeKind::Function(_) | ScopeKind::Lambda(_)) {
if checker.enabled(Rule::UnusedVariable) {
pyflakes::rules::unused_variable(checker, scope, &mut diagnostics);

8
crates/ruff_linter/src/checkers/ast/analyze/expression.rs
View file

@ -242,13 +242,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
checker.diagnostics.push(diagnostic);
}
}
if let ScopeKind::Class(class_def) = checker.semantic.current_scope().kind {
if checker.enabled(Rule::BuiltinAttributeShadowing) {
flake8_builtins::rules::builtin_attribute_shadowing(
checker, class_def, id, *range,
);
}
} else {
if !checker.semantic.current_scope().kind.is_class() {
if checker.enabled(Rule::BuiltinVariableShadowing) {
flake8_builtins::rules::builtin_variable_shadowing(checker, id, *range);
}

12
crates/ruff_linter/src/checkers/ast/analyze/statement.rs
View file

@ -347,17 +347,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::FStringDocstring) {
flake8_bugbear::rules::f_string_docstring(checker, body);
}
if let ScopeKind::Class(class_def) = checker.semantic.current_scope().kind {
if checker.enabled(Rule::BuiltinAttributeShadowing) {
flake8_builtins::rules::builtin_method_shadowing(
checker,
class_def,
name,
decorator_list,
name.range(),
);
}
} else {
if !checker.semantic.current_scope().kind.is_class() {
if checker.enabled(Rule::BuiltinVariableShadowing) {
flake8_builtins::rules::builtin_variable_shadowing(checker, name, name.range());
}

217
crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs
View file

@ -1,11 +1,10 @@
use ruff_python_ast as ast;
use ruff_python_ast::{Arguments, Decorator};
use ruff_text_size::TextRange;
use ruff_diagnostics::Diagnostic;
use ruff_diagnostics::Violation;
use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::SemanticModel;
use ruff_python_ast as ast;
use ruff_python_semantic::{BindingKind, Scope, ScopeId};
use ruff_source_file::SourceRow;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
use crate::rules::flake8_builtins::helpers::shadows_builtin;
@ -20,6 +19,23 @@ use crate::rules::flake8_builtins::helpers::shadows_builtin;
/// non-obvious errors, as readers may mistake the attribute for the
/// builtin and vice versa.
///
/// Since methods and class attributes typically cannot be referenced directly
/// from outside the class scope, this rule only applies to those methods
/// and attributes that both shadow a builtin _and_ are referenced from within
/// the class scope, as in the following example, where the `list[int]` return
/// type annotation resolves to the `list` method, rather than the builtin:
///
/// ```python
/// class Class:
/// @staticmethod
/// def list() -> None:
/// pass
///
/// @staticmethod
/// def repeat(value: int, times: int) -> list[int]:
/// return [value] * times
/// ```
///
/// Builtins can be marked as exceptions to this rule via the
/// [`flake8-builtins.builtins-ignorelist`] configuration option, or
/// converted to the appropriate dunder method. Methods decorated with
@ -28,135 +44,112 @@ use crate::rules::flake8_builtins::helpers::shadows_builtin;
///
/// ## Example
/// ```python
/// class Shadow:
/// def int():
/// return 0
/// ```
/// class Class:
/// @staticmethod
/// def list() -> None:
/// pass
///
/// Use instead:
/// ```python
/// class Shadow:
/// def to_int():
/// return 0
/// ```
///
/// Or:
/// ```python
/// class Shadow:
/// # Callable as `int(shadow)`
/// def __int__():
/// return 0
/// @staticmethod
/// def repeat(value: int, times: int) -> list[int]:
/// return [value] * times
/// ```
///
/// ## Options
/// - `flake8-builtins.builtins-ignorelist`
///
/// ## References
/// - [_Is it bad practice to use a built-in function name as an attribute or method identifier?_](https://stackoverflow.com/questions/9109333/is-it-bad-practice-to-use-a-built-in-function-name-as-an-attribute-or-method-ide)
/// - [_Why is it a bad idea to name a variable `id` in Python?_](https://stackoverflow.com/questions/77552/id-is-a-bad-variable-name-in-python)
#[violation]
pub struct BuiltinAttributeShadowing {
kind: Kind,
name: String,
row: SourceRow,
}
impl Violation for BuiltinAttributeShadowing {
#[derive_message_formats]
fn message(&self) -> String {
let BuiltinAttributeShadowing { name } = self;
format!("Class attribute `{name}` is shadowing a Python builtin")
let BuiltinAttributeShadowing { kind, name, row } = self;
match kind {
Kind::Attribute => {
format!("Python builtin is shadowed by class attribute `{name}` from {row}")
}
Kind::Method => {
format!("Python builtin is shadowed by method `{name}` from {row}")
}
}
}
}
/// A003
pub(crate) fn builtin_attribute_shadowing(
checker: &mut Checker,
checker: &Checker,
scope_id: ScopeId,
scope: &Scope,
class_def: &ast::StmtClassDef,
name: &str,
range: TextRange,
diagnostics: &mut Vec<Diagnostic>,
) {
if shadows_builtin(
name,
&checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) {
// Ignore shadowing within `TypedDict` definitions, since these are only accessible through
// subscripting and not through attribute access.
if class_def
.bases()
.iter()
.any(|base| checker.semantic().match_typing_expr(base, "TypedDict"))
{
return;
}
for (name, binding_id) in scope.all_bindings() {
let binding = checker.semantic().binding(binding_id);
checker.diagnostics.push(Diagnostic::new(
BuiltinAttributeShadowing {
name: name.to_string(),
},
range,
));
// We only care about methods and attributes.
let kind = match binding.kind {
BindingKind::Assignment | BindingKind::Annotation => Kind::Attribute,
BindingKind::FunctionDefinition(_) => Kind::Method,
_ => continue,
};
if shadows_builtin(
name,
&checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) {
// Ignore explicit overrides.
if class_def.decorator_list.iter().any(|decorator| {
checker
.semantic()
.match_typing_expr(&decorator.expression, "override")
}) {
return;
}
// Class scopes are special, in that you can only reference a binding defined in a
// class scope from within the class scope itself. As such, we can safely ignore
// methods that weren't referenced from within the class scope. In other words, we're
// only trying to identify shadowing as in:
// ```python
// class Class:
// @staticmethod
// def list() -> None:
// pass
//
// @staticmethod
// def repeat(value: int, times: int) -> list[int]:
// return [value] * times
// ```
for reference in binding
.references
.iter()
.map(|reference_id| checker.semantic().reference(*reference_id))
.filter(|reference| {
checker
.semantic()
.first_non_type_parent_scope_id(reference.scope_id())
== Some(scope_id)
})
{
diagnostics.push(Diagnostic::new(
BuiltinAttributeShadowing {
kind,
name: name.to_string(),
row: checker.compute_source_row(binding.start()),
},
reference.range(),
));
}
}
}
}
/// A003
pub(crate) fn builtin_method_shadowing(
checker: &mut Checker,
class_def: &ast::StmtClassDef,
name: &str,
decorator_list: &[Decorator],
range: TextRange,
) {
if shadows_builtin(
name,
&checker.settings.flake8_builtins.builtins_ignorelist,
checker.source_type,
) {
// Ignore some standard-library methods. Ideally, we'd ignore all overridden methods, since
// those should be flagged on the superclass, but that's more difficult.
if is_standard_library_override(name, class_def, checker.semantic()) {
return;
}
// Ignore explicit overrides.
if decorator_list.iter().any(|decorator| {
checker
.semantic()
.match_typing_expr(&decorator.expression, "override")
}) {
return;
}
checker.diagnostics.push(Diagnostic::new(
BuiltinAttributeShadowing {
name: name.to_string(),
},
range,
));
}
}
/// Return `true` if an attribute appears to be an override of a standard-library method.
fn is_standard_library_override(
name: &str,
class_def: &ast::StmtClassDef,
semantic: &SemanticModel,
) -> bool {
let Some(Arguments { args: bases, .. }) = class_def.arguments.as_deref() else {
return false;
};
match name {
// Ex) `Event.set`
"set" => bases.iter().any(|base| {
semantic
.resolve_call_path(base)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["threading", "Event"]))
}),
// Ex) `Filter.filter`
"filter" => bases.iter().any(|base| {
semantic
.resolve_call_path(base)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["logging", "Filter"]))
}),
_ => false,
}
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
enum Kind {
Attribute,
Method,
}

70
crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A003_A003.py.snap
View file

@ -1,68 +1,22 @@
---
source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs
---
A003.py:2:5: A003 Class attribute `ImportError` is shadowing a Python builtin
|
1 | class MyClass:
2 | ImportError = 4
| ^^^^^^^^^^^ A003
3 | id: int
4 | dir = "/"
|
A003.py:3:5: A003 Class attribute `id` is shadowing a Python builtin
|
1 | class MyClass:
2 | ImportError = 4
3 | id: int
| ^^ A003
4 | dir = "/"
|
A003.py:4:5: A003 Class attribute `dir` is shadowing a Python builtin
|
2 | ImportError = 4
3 | id: int
4 | dir = "/"
| ^^^ A003
5 |
6 | def __init__(self):
|
A003.py:11:9: A003 Class attribute `str` is shadowing a Python builtin
A003.py:17:31: A003 Python builtin is shadowed by method `str` from line 14
|
9 | self.dir = "."
10 |
11 | def str(self):
| ^^^ A003
12 | pass
15 | pass
16 |
17 | def method_usage(self) -> str:
| ^^^ A003
18 | pass
|
A003.py:29:9: A003 Class attribute `str` is shadowing a Python builtin
A003.py:20:34: A003 Python builtin is shadowed by class attribute `id` from line 3
|
27 | ...
28 |
29 | def str(self) -> None:
| ^^^ A003
30 | ...
|
A003.py:40:9: A003 Class attribute `str` is shadowing a Python builtin
|
38 | ...
39 |
40 | def str(self) -> None:
| ^^^ A003
41 | ...
|
A003.py:52:9: A003 Class attribute `int` is shadowing a Python builtin
|
50 | pass
51 |
52 | def int(self):
| ^^^ A003
53 | pass
18 | pass
19 |
20 | def attribute_usage(self) -> id:
| ^^ A003
21 | pass
|

48
crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap
View file

@ -1,49 +1,13 @@
---
source: crates/ruff_linter/src/rules/flake8_builtins/mod.rs
---
A003.py:2:5: A003 Class attribute `ImportError` is shadowing a Python builtin
|
1 | class MyClass:
2 | ImportError = 4
| ^^^^^^^^^^^ A003
3 | id: int
4 | dir = "/"
|
A003.py:11:9: A003 Class attribute `str` is shadowing a Python builtin
A003.py:17:31: A003 Python builtin is shadowed by method `str` from line 14
|
9 | self.dir = "."
10 |
11 | def str(self):
| ^^^ A003
12 | pass
|
A003.py:29:9: A003 Class attribute `str` is shadowing a Python builtin
|
27 | ...
28 |
29 | def str(self) -> None:
| ^^^ A003
30 | ...
|
A003.py:40:9: A003 Class attribute `str` is shadowing a Python builtin
|
38 | ...
39 |
40 | def str(self) -> None:
| ^^^ A003
41 | ...
|
A003.py:52:9: A003 Class attribute `int` is shadowing a Python builtin
|
50 | pass
51 |
52 | def int(self):
| ^^^ A003
53 | pass
15 | pass
16 |
17 | def method_usage(self) -> str:
| ^^^ A003
18 | pass
|

18
crates/ruff_python_semantic/src/model.rs
View file

@ -984,6 +984,11 @@ impl<'a> SemanticModel<'a> {
scope.parent.map(|scope_id| &self.scopes[scope_id])
}
/// Returns the ID of the parent of the given [`ScopeId`], if any.
pub fn parent_scope_id(&self, scope_id: ScopeId) -> Option<ScopeId> {
self.scopes[scope_id].parent
}
/// Returns the first parent of the given [`Scope`] that is not of [`ScopeKind::Type`], if any.
pub fn first_non_type_parent_scope(&self, scope: &Scope) -> Option<&Scope<'a>> {
let mut current_scope = scope;
@ -997,6 +1002,19 @@ impl<'a> SemanticModel<'a> {
None
}
/// Returns the first parent of the given [`ScopeId`] that is not of [`ScopeKind::Type`], if any.
pub fn first_non_type_parent_scope_id(&self, scope_id: ScopeId) -> Option<ScopeId> {
let mut current_scope_id = scope_id;
while let Some(parent_id) = self.parent_scope_id(current_scope_id) {
if self.scopes[parent_id].kind.is_type() {
current_scope_id = parent_id;
} else {
return Some(parent_id);
}
}
None
}
/// Return the [`Stmt`] corresponding to the given [`NodeId`].
#[inline]
pub fn node(&self, node_id: NodeId) -> &NodeRef<'a> {