From 25bafd2d66abe2b93f8e3199b8c60a8549523139 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 11 Jan 2024 12:59:40 -0500 Subject: [PATCH] Restrict `builtin-attribute-shadowing` to actual shadowed references (#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 https://github.com/astral-sh/ruff/issues/6524. Closes https://github.com/astral-sh/ruff/issues/7806. --- .../test/fixtures/flake8_builtins/A003.py | 42 +--- .../checkers/ast/analyze/deferred_scopes.rs | 16 +- .../src/checkers/ast/analyze/expression.rs | 8 +- .../src/checkers/ast/analyze/statement.rs | 12 +- .../rules/builtin_attribute_shadowing.rs | 211 +++++++++--------- ..._flake8_builtins__tests__A003_A003.py.snap | 70 +----- ...sts__A003_A003.py_builtins_ignorelist.snap | 48 +--- crates/ruff_python_semantic/src/model.rs | 18 ++ 8 files changed, 160 insertions(+), 265 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_builtins/A003.py b/crates/ruff_linter/resources/test/fixtures/flake8_builtins/A003.py index 66d83c229d72f5..b2bb8b872ffc41 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_builtins/A003.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_builtins/A003.py @@ -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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index e6c46334dd7af7..ceb40016ef10f5 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -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); diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 474ec32b207898..953b8884c14b31 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -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); } diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 0165e431bc4223..5d8c4294165fd2 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -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()); } diff --git a/crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs b/crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs index ffe8384186321c..793d287c65d2de 100644 --- a/crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs +++ b/crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs @@ -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 -/// ``` -/// -/// 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, ) { - 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, } diff --git a/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A003_A003.py.snap b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A003_A003.py.snap index 4e871c06910f3d..f89f5abeda06d0 100644 --- a/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A003_A003.py.snap +++ b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A003_A003.py.snap @@ -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 - | - 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 +A003.py:17:31: A003 Python builtin is shadowed by method `str` from line 14 | -38 | ... -39 | -40 | def str(self) -> None: - | ^^^ A003 -41 | ... +15 | pass +16 | +17 | def method_usage(self) -> str: + | ^^^ A003 +18 | pass | -A003.py:52:9: A003 Class attribute `int` is shadowing a Python builtin +A003.py:20:34: A003 Python builtin is shadowed by class attribute `id` from line 3 | -50 | pass -51 | -52 | def int(self): - | ^^^ A003 -53 | pass +18 | pass +19 | +20 | def attribute_usage(self) -> id: + | ^^ A003 +21 | pass | diff --git a/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap index d533b2ac7c1dd4..06d0956121ba76 100644 --- a/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap +++ b/crates/ruff_linter/src/rules/flake8_builtins/snapshots/ruff_linter__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap @@ -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 - | - 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 +A003.py:17:31: A003 Python builtin is shadowed by method `str` from line 14 | -50 | pass -51 | -52 | def int(self): - | ^^^ A003 -53 | pass +15 | pass +16 | +17 | def method_usage(self) -> str: + | ^^^ A003 +18 | pass | diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 27b6bed31e640d..3e15e62aff7d26 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -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 { + 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 { + 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> {