From 11dd457821d3b80b3b870bdc22352a5e2696e778 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 25 Jul 2023 12:03:04 -0400 Subject: [PATCH 1/2] Type --- .../test/fixtures/flake8_builtins/A003.py | 17 ++++++++++ .../src/checkers/ast/analyze/statement.rs | 3 +- .../rules/builtin_attribute_shadowing.rs | 31 ++++++++++++++++++- 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py b/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py index d2eaa3e393d4e..715b95c65f88c 100644 --- a/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py +++ b/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py @@ -39,3 +39,20 @@ def filter(self, record: LogRecord) -> bool: def str(self) -> None: ... + + +from typing_extensions import override + + +class MyClass: + ImportError = 4 + id: int + dir = "/" + + def __init__(self): + self.float = 5 # is fine + self.id = 10 + self.dir = "." + + def str(self): + pass diff --git a/crates/ruff/src/checkers/ast/analyze/statement.rs b/crates/ruff/src/checkers/ast/analyze/statement.rs index 5765aa8193742..958a9dcd1defd 100644 --- a/crates/ruff/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff/src/checkers/ast/analyze/statement.rs @@ -332,10 +332,11 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } if let ScopeKind::Class(class_def) = checker.semantic.scope().kind { if checker.enabled(Rule::BuiltinAttributeShadowing) { - flake8_builtins::rules::builtin_attribute_shadowing( + flake8_builtins::rules::builtin_method_shadowing( checker, class_def, name, + decorator_list, name.range(), ); } diff --git a/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs b/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs index aa686b2595aa0..8e3733155cd7d 100644 --- a/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs +++ b/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs @@ -1,5 +1,6 @@ use ruff_text_size::TextRange; use rustpython_parser::ast; +use rustpython_parser::ast::Decorator; use ruff_diagnostics::Diagnostic; use ruff_diagnostics::Violation; @@ -10,7 +11,8 @@ use crate::checkers::ast::Checker; use crate::rules::flake8_builtins::helpers::shadows_builtin; /// ## What it does -/// Checks for any class attributes that use the same name as a builtin. +/// Checks for any class attributes or methods that use the same name as a +/// builtin. /// /// ## Why is this bad? /// Reusing a builtin name for the name of an attribute increases the @@ -81,12 +83,39 @@ pub(crate) fn builtin_attribute_shadowing( return; } + checker.diagnostics.push(Diagnostic::new( + BuiltinAttributeShadowing { + name: name.to_string(), + }, + 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) { // 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(), From ca52b63280f14aa7ba181ba47e09aa7cb7bd2c60 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 25 Jul 2023 12:03:52 -0400 Subject: [PATCH 2/2] Ignore explicit overrides --- .../resources/test/fixtures/flake8_builtins/A003.py | 13 ++++--------- .../rules/builtin_attribute_shadowing.rs | 4 +++- ...rules__flake8_builtins__tests__A003_A003.py.snap | 9 +++++++++ ...ns__tests__A003_A003.py_builtins_ignorelist.snap | 9 +++++++++ 4 files changed, 25 insertions(+), 10 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py b/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py index 715b95c65f88c..66d83c229d72f 100644 --- a/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py +++ b/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py @@ -45,14 +45,9 @@ def str(self) -> None: class MyClass: - ImportError = 4 - id: int - dir = "/" - - def __init__(self): - self.float = 5 # is fine - self.id = 10 - self.dir = "." - + @override def str(self): pass + + def int(self): + pass diff --git a/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs b/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs index 8e3733155cd7d..353e92f5060c8 100644 --- a/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs +++ b/crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs @@ -22,7 +22,9 @@ use crate::rules::flake8_builtins::helpers::shadows_builtin; /// /// Builtins can be marked as exceptions to this rule via the /// [`flake8-builtins.builtins-ignorelist`] configuration option, or -/// converted to the appropriate dunder method. +/// converted to the appropriate dunder method. Methods decorated with +/// `@typing.override` or `@typing_extensions.override` are also +/// ignored. /// /// ## Example /// ```python diff --git a/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py.snap b/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py.snap index 3a96ef9104e50..5da7f00304905 100644 --- a/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py.snap +++ b/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py.snap @@ -56,4 +56,13 @@ A003.py:40:9: A003 Class attribute `str` is shadowing a Python builtin 41 | ... | +A003.py:52:9: A003 Class attribute `int` is shadowing a Python builtin + | +50 | pass +51 | +52 | def int(self): + | ^^^ A003 +53 | pass + | + diff --git a/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap b/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap index ebdf4e0fdf422..7c86544588bad 100644 --- a/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap +++ b/crates/ruff/src/rules/flake8_builtins/snapshots/ruff__rules__flake8_builtins__tests__A003_A003.py_builtins_ignorelist.snap @@ -37,4 +37,13 @@ A003.py:40:9: A003 Class attribute `str` is shadowing a Python builtin 41 | ... | +A003.py:52:9: A003 Class attribute `int` is shadowing a Python builtin + | +50 | pass +51 | +52 | def int(self): + | ^^^ A003 +53 | pass + | +