From 5f63b8bfb83ce1fc271fa05d213b38c81818a06b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 25 Jul 2023 11:54:34 -0400 Subject: [PATCH] Ignore some common builtin overrides on standard library subclasses (#6074) ## Summary If a user subclasses `threading.Event`, e.g. with: ```python from threading import Event class CustomEvent(Event): def set(self) -> None: ... ``` They no control over the method name (`set`). This PR allows `threading.Event#set` and `logging.Filter#filter` overrides, and avoids flagging A003 in such cases. Ideally, we'd avoid flagging all overridden methods, but... that's a lot more difficult, and this is at least _better_ than what we do now. Closes https://github.com/astral-sh/ruff/issues/6057. Closes https://github.com/astral-sh/ruff/issues/5956. --- .../test/fixtures/flake8_builtins/A003.py | 22 ++++++++++++++ .../rules/builtin_attribute_shadowing.rs | 30 +++++++++++++++++++ ..._flake8_builtins__tests__A003_A003.py.snap | 18 +++++++++++ ...sts__A003_A003.py_builtins_ignorelist.snap | 18 +++++++++++ 4 files changed, 88 insertions(+) diff --git a/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py b/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py index b971974ef265e..d2eaa3e393d4e 100644 --- a/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py +++ b/crates/ruff/resources/test/fixtures/flake8_builtins/A003.py @@ -17,3 +17,25 @@ def str(self): 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: + ... 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 6f523d76f9b89..aa686b2595aa0 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 @@ -4,6 +4,7 @@ use rustpython_parser::ast; use ruff_diagnostics::Diagnostic; use ruff_diagnostics::Violation; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_semantic::SemanticModel; use crate::checkers::ast::Checker; use crate::rules::flake8_builtins::helpers::shadows_builtin; @@ -80,6 +81,12 @@ pub(crate) fn builtin_attribute_shadowing( return; } + // 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; + } + checker.diagnostics.push(Diagnostic::new( BuiltinAttributeShadowing { name: name.to_string(), @@ -88,3 +95,26 @@ pub(crate) fn builtin_attribute_shadowing( )); } } + +/// 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, + model: &SemanticModel, +) -> bool { + match name { + // Ex) `Event#set` + "set" => class_def.bases.iter().any(|base| { + model.resolve_call_path(base).map_or(false, |call_path| { + matches!(call_path.as_slice(), ["threading", "Event"]) + }) + }), + // Ex) `Filter#filter` + "filter" => class_def.bases.iter().any(|base| { + model.resolve_call_path(base).map_or(false, |call_path| { + matches!(call_path.as_slice(), ["logging", "Filter"]) + }) + }), + _ => false, + } +} 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 48c9dfa2170a7..3a96ef9104e50 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 @@ -38,4 +38,22 @@ A003.py:11:9: A003 Class attribute `str` is shadowing a Python builtin 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 | ... + | + 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 342f14a9a1cf1..ebdf4e0fdf422 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 @@ -19,4 +19,22 @@ A003.py:11:9: A003 Class attribute `str` is shadowing a Python builtin 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 | ... + | +