Skip to content

Commit

Permalink
Restrict builtin-attribute-shadowing to actual shadowed references (#…
Browse files Browse the repository at this point in the history 
…9462)

## 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 #6524.

Closes #7806.
  • Loading branch information
@charliermarsh
charliermarsh authored Jan 11, 2024
1 parent 7fc51d2 commit 25bafd2
Show file tree
Hide file tree
Showing 8 changed files with 160 additions and 265 deletions.
42 changes: 5 additions & 37 deletions crates/ruff_linter/resources/test/fixtures/flake8_builtins/A003.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,46 +8,14 @@ def __init__(self):
self.id = 10
self.dir = "."

def str(self):
def int(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):
def method_usage(self) -> str:
pass

def attribute_usage(self) -> id:
pass
16 changes: 15 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`].
Expand All @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
8 changes: 1 addition & 7 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
12 changes: 1 addition & 11 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
211 changes: 102 additions & 109 deletions crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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
Expand All @@ -28,135 +44,112 @@ use crate::rules::flake8_builtins::helpers::shadows_builtin;
///
/// ## Example
/// ```python
/// class Shadow:
/// def int():
/// return 0
/// ```
///
/// Use instead:
/// ```python
/// class Shadow:
/// def to_int():
/// return 0
/// ```
/// class Class:
/// @staticmethod
/// def list() -> None:
/// pass
///
/// 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,
};

/// 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;
}
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;
}

// Ignore explicit overrides.
if 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(),
));
}
}

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,
}
Loading

0 comments on commit 25bafd2

Please sign in to comment.