From c814745643dbae3d5b6a7782886f173ca738996b Mon Sep 17 00:00:00 2001 From: InSync Date: Sun, 23 Feb 2025 17:00:49 +0700 Subject: [PATCH] [`flake8-self`] Ignore attribute accesses on instance-like variables (`SLF001`) (#16149) --- .../test/fixtures/flake8_self/SLF001_1.py | 45 ++++ .../ruff_linter/src/rules/flake8_self/mod.rs | 1 + .../rules/private_member_access.rs | 207 +++++++++++------- ...ts__private-member-access_SLF001_1.py.snap | 4 + .../ruff_linter/src/rules/pylint/helpers.rs | 107 ++++----- crates/ruff_linter/src/rules/pylint/mod.rs | 2 +- crates/ruff_python_ast/src/helpers.rs | 7 + .../src/analyze/typing.rs | 2 +- 8 files changed, 242 insertions(+), 133 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_self/SLF001_1.py create mode 100644 crates/ruff_linter/src/rules/flake8_self/snapshots/ruff_linter__rules__flake8_self__tests__private-member-access_SLF001_1.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_self/SLF001_1.py b/crates/ruff_linter/resources/test/fixtures/flake8_self/SLF001_1.py new file mode 100644 index 0000000000000..1a784f5bdb693 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_self/SLF001_1.py @@ -0,0 +1,45 @@ +from __future__ import annotations + +from typing import Annotated + +# https://github.com/astral-sh/ruff/issues/9022 + +class Lorem[T]: + def f(self): + lorem_1 = Lorem() + lorem_1._value = 1 # fine + + lorem_2 = Lorem[bytes]() + lorem_2._value = 1 # fine + + +class Ipsum: + def __new__(cls): + instance = super().__new__(cls) + instance._value = 1 # fine + + +class Dolor[T]: + def f( + self, + a: Dolor, + b: Dolor[int], + c: Annotated[Dolor, ...], + d: Annotated[Dolor[str], ...] + ): + a._value = 1 # fine + b._value = 1 # fine + c._value = 1 # fine + d._value = 1 # fine + + @classmethod + def m(cls): + instance = cls() + instance._value = 1 # fine + + +class M(type): + @classmethod + def f(mcs): + cls = mcs() + cls._value = 1 diff --git a/crates/ruff_linter/src/rules/flake8_self/mod.rs b/crates/ruff_linter/src/rules/flake8_self/mod.rs index d870d85f9fcb1..a445b40bafaf1 100644 --- a/crates/ruff_linter/src/rules/flake8_self/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_self/mod.rs @@ -16,6 +16,7 @@ mod tests { use test_case::test_case; #[test_case(Rule::PrivateMemberAccess, Path::new("SLF001.py"))] + #[test_case(Rule::PrivateMemberAccess, Path::new("SLF001_1.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/flake8_self/rules/private_member_access.rs b/crates/ruff_linter/src/rules/flake8_self/rules/private_member_access.rs index 7437fde65762e..60ca1c8bdf1cb 100644 --- a/crates/ruff_linter/src/rules/flake8_self/rules/private_member_access.rs +++ b/crates/ruff_linter/src/rules/flake8_self/rules/private_member_access.rs @@ -1,11 +1,15 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; +use ruff_python_ast::helpers::{is_dunder, is_sunder}; use ruff_python_ast::name::UnqualifiedName; use ruff_python_ast::{self as ast, Expr}; -use ruff_python_semantic::{BindingKind, ScopeKind}; +use ruff_python_semantic::analyze::typing; +use ruff_python_semantic::analyze::typing::TypeChecker; +use ruff_python_semantic::{BindingKind, ScopeKind, SemanticModel}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::rules::pylint::helpers::is_dunder_operator_method; /// ## What it does /// Checks for accesses on "private" class members. @@ -69,27 +73,14 @@ pub(crate) fn private_member_access(checker: &Checker, expr: &Expr) { return; }; - if checker.semantic().in_annotation() { - return; - } - - // Ignore non-private accesses. - if !attr.starts_with('_') { - return; - } + let semantic = checker.semantic(); + let current_scope = semantic.current_scope(); - // Ignore dunder accesses. - let is_dunder = attr.starts_with("__") && attr.ends_with("__"); - if is_dunder { + if semantic.in_annotation() { return; } - // Ignore sunder accesses. - let is_sunder = attr.starts_with('_') - && attr.ends_with('_') - && !attr.starts_with("__") - && !attr.ends_with("__"); - if is_sunder { + if !attr.starts_with('_') || is_dunder(attr) || is_sunder(attr) { return; } @@ -103,65 +94,14 @@ pub(crate) fn private_member_access(checker: &Checker, expr: &Expr) { } // Ignore accesses on instances within special methods (e.g., `__eq__`). - if let ScopeKind::Function(ast::StmtFunctionDef { name, .. }) = - checker.semantic().current_scope().kind - { - if matches!( - name.as_str(), - "__lt__" - | "__le__" - | "__eq__" - | "__ne__" - | "__gt__" - | "__ge__" - | "__add__" - | "__sub__" - | "__mul__" - | "__matmul__" - | "__truediv__" - | "__floordiv__" - | "__mod__" - | "__divmod__" - | "__pow__" - | "__lshift__" - | "__rshift__" - | "__and__" - | "__xor__" - | "__or__" - | "__radd__" - | "__rsub__" - | "__rmul__" - | "__rmatmul__" - | "__rtruediv__" - | "__rfloordiv__" - | "__rmod__" - | "__rdivmod__" - | "__rpow__" - | "__rlshift__" - | "__rrshift__" - | "__rand__" - | "__rxor__" - | "__ror__" - | "__iadd__" - | "__isub__" - | "__imul__" - | "__imatmul__" - | "__itruediv__" - | "__ifloordiv__" - | "__imod__" - | "__ipow__" - | "__ilshift__" - | "__irshift__" - | "__iand__" - | "__ixor__" - | "__ior__" - ) { + if let ScopeKind::Function(ast::StmtFunctionDef { name, .. }) = current_scope.kind { + if is_dunder_operator_method(name) { return; } } // Allow some documented private methods, like `os._exit()`. - if let Some(qualified_name) = checker.semantic().resolve_qualified_name(expr) { + if let Some(qualified_name) = semantic.resolve_qualified_name(expr) { if matches!(qualified_name.segments(), ["os", "_exit"]) { return; } @@ -185,25 +125,23 @@ pub(crate) fn private_member_access(checker: &Checker, expr: &Expr) { if let Expr::Name(name) = value.as_ref() { // Ignore accesses on class members from _within_ the class. - if checker - .semantic() + if semantic .resolve_name(name) .and_then(|id| { - if let BindingKind::ClassDefinition(scope) = checker.semantic().binding(id).kind { + if let BindingKind::ClassDefinition(scope) = semantic.binding(id).kind { Some(scope) } else { None } }) - .is_some_and(|scope| { - checker - .semantic() - .current_scope_ids() - .any(|parent| scope == parent) - }) + .is_some_and(|scope| semantic.current_scope_ids().any(|parent| scope == parent)) { return; } + + if is_same_class_instance(name, semantic) { + return; + } } checker.report_diagnostic(Diagnostic::new( @@ -213,3 +151,110 @@ pub(crate) fn private_member_access(checker: &Checker, expr: &Expr) { expr.range(), )); } + +/// Check for the following cases: +/// +/// * Parameter annotation: +/// +/// ```python +/// class C[T]: +/// def f(self, other: C): ... +/// def f(self, other: C[...]): ... +/// def f(self, other: Annotated[C, ...]): ... +/// ``` +/// +/// * `super().__new__`/`cls` call: +/// +/// ```python +/// class C: +/// def __new__(cls): ... +/// instance = super().__new__(cls) +/// @classmethod +/// def m(cls): +/// instance = cls() +/// ``` +/// +/// This function is intentionally naive and does not handle more complex cases. +/// It is expected to be expanded overtime, possibly when type-aware APIs are available. +fn is_same_class_instance(name: &ast::ExprName, semantic: &SemanticModel) -> bool { + let Some(binding_id) = semantic.resolve_name(name) else { + return false; + }; + + let binding = semantic.binding(binding_id); + typing::check_type::(binding, semantic) +} + +struct SameClassInstanceChecker; + +impl SameClassInstanceChecker { + /// Whether `name` resolves to a class which the semantic model is traversing. + fn is_current_class_name(name: &ast::ExprName, semantic: &SemanticModel) -> bool { + semantic.current_scopes().any(|scope| { + let ScopeKind::Class(class) = scope.kind else { + return false; + }; + + class.name.id == name.id + }) + } +} + +impl TypeChecker for SameClassInstanceChecker { + /// `C`, `C[T]`, `Annotated[C, ...]`, `Annotated[C[T], ...]` + fn match_annotation(annotation: &Expr, semantic: &SemanticModel) -> bool { + let Some(class_name) = find_class_name(annotation, semantic) else { + return false; + }; + + Self::is_current_class_name(class_name, semantic) + } + + /// `cls()`, `C()`, `C[T]()`, `super().__new__()` + fn match_initializer(initializer: &Expr, semantic: &SemanticModel) -> bool { + let Expr::Call(call) = initializer else { + return false; + }; + + match &*call.func { + Expr::Subscript(_) => Self::match_annotation(&call.func, semantic), + + Expr::Name(name) => { + matches!(&*name.id, "cls" | "mcs") || Self::is_current_class_name(name, semantic) + } + + Expr::Attribute(ast::ExprAttribute { value, attr, .. }) => { + let Expr::Call(ast::ExprCall { func, .. }) = &**value else { + return false; + }; + + let Expr::Name(ast::ExprName { id: func, .. }) = &**func else { + return false; + }; + + func == "super" && attr == "__new__" + } + + _ => false, + } + } +} + +/// Convert `Annotated[C[T], ...]` to `C` (and similar) to `C` recursively. +fn find_class_name<'a>(expr: &'a Expr, semantic: &'a SemanticModel) -> Option<&'a ast::ExprName> { + match expr { + Expr::Name(name) => Some(name), + Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { + if semantic.match_typing_expr(value, "Annotated") { + let [expr, ..] = &slice.as_tuple_expr()?.elts[..] else { + return None; + }; + + return find_class_name(expr, semantic); + } + + find_class_name(value, semantic) + } + _ => None, + } +} diff --git a/crates/ruff_linter/src/rules/flake8_self/snapshots/ruff_linter__rules__flake8_self__tests__private-member-access_SLF001_1.py.snap b/crates/ruff_linter/src/rules/flake8_self/snapshots/ruff_linter__rules__flake8_self__tests__private-member-access_SLF001_1.py.snap new file mode 100644 index 0000000000000..ad933c0e8a896 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_self/snapshots/ruff_linter__rules__flake8_self__tests__private-member-access_SLF001_1.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/flake8_self/mod.rs +--- + diff --git a/crates/ruff_linter/src/rules/pylint/helpers.rs b/crates/ruff_linter/src/rules/pylint/helpers.rs index e0bbc25980a76..d2b04e6cf4b81 100644 --- a/crates/ruff_linter/src/rules/pylint/helpers.rs +++ b/crates/ruff_linter/src/rules/pylint/helpers.rs @@ -166,16 +166,68 @@ impl Visitor<'_> for SequenceIndexVisitor<'_> { } } -/// Returns `true` if a method is a known dunder method. -pub(super) fn is_known_dunder_method(method: &str) -> bool { +pub(crate) fn is_dunder_operator_method(method: &str) -> bool { matches!( method, - "__abs__" + "__lt__" + | "__le__" + | "__eq__" + | "__ne__" + | "__gt__" + | "__ge__" | "__add__" + | "__sub__" + | "__mul__" + | "__matmul__" + | "__truediv__" + | "__floordiv__" + | "__mod__" + | "__divmod__" + | "__pow__" + | "__lshift__" + | "__rshift__" + | "__and__" + | "__xor__" + | "__or__" + | "__radd__" + | "__rsub__" + | "__rmul__" + | "__rmatmul__" + | "__rtruediv__" + | "__rfloordiv__" + | "__rmod__" + | "__rdivmod__" + | "__rpow__" + | "__rlshift__" + | "__rrshift__" + | "__rand__" + | "__rxor__" + | "__ror__" + | "__iadd__" + | "__isub__" + | "__imul__" + | "__imatmul__" + | "__itruediv__" + | "__ifloordiv__" + | "__imod__" + | "__ipow__" + | "__ilshift__" + | "__irshift__" + | "__iand__" + | "__ixor__" + | "__ior__" + ) +} + +/// Returns `true` if a method is a known dunder method. +pub(super) fn is_known_dunder_method(method: &str) -> bool { + is_dunder_operator_method(method) + || matches!( + method, + "__abs__" | "__aenter__" | "__aexit__" | "__aiter__" - | "__and__" | "__anext__" | "__attrs_init__" | "__attrs_post_init__" @@ -198,17 +250,13 @@ pub(super) fn is_known_dunder_method(method: &str) -> bool { | "__delitem__" | "__dict__" | "__dir__" - | "__divmod__" | "__doc__" | "__enter__" - | "__eq__" | "__exit__" | "__float__" | "__floor__" - | "__floordiv__" | "__format__" | "__fspath__" - | "__ge__" | "__get__" | "__getattr__" | "__getattribute__" @@ -216,71 +264,33 @@ pub(super) fn is_known_dunder_method(method: &str) -> bool { | "__getnewargs__" | "__getnewargs_ex__" | "__getstate__" - | "__gt__" | "__hash__" | "__html__" - | "__iadd__" - | "__iand__" - | "__ifloordiv__" - | "__ilshift__" - | "__imatmul__" - | "__imod__" - | "__imul__" | "__index__" | "__init__" | "__init_subclass__" | "__instancecheck__" | "__int__" | "__invert__" - | "__ior__" - | "__ipow__" - | "__irshift__" - | "__isub__" | "__iter__" - | "__itruediv__" - | "__ixor__" - | "__le__" | "__len__" | "__length_hint__" - | "__lshift__" - | "__lt__" - | "__matmul__" | "__missing__" - | "__mod__" | "__module__" | "__mro_entries__" - | "__mul__" - | "__ne__" | "__neg__" | "__new__" | "__next__" - | "__or__" | "__pos__" | "__post_init__" - | "__pow__" | "__prepare__" - | "__radd__" - | "__rand__" - | "__rdivmod__" | "__reduce__" | "__reduce_ex__" | "__release_buffer__" | "__replace__" | "__repr__" | "__reversed__" - | "__rfloordiv__" - | "__rlshift__" - | "__rmatmul__" - | "__rmod__" - | "__rmul__" - | "__ror__" | "__round__" - | "__rpow__" - | "__rrshift__" - | "__rshift__" - | "__rsub__" - | "__rtruediv__" - | "__rxor__" | "__set__" | "__set_name__" | "__setattr__" @@ -288,14 +298,11 @@ pub(super) fn is_known_dunder_method(method: &str) -> bool { | "__setstate__" | "__sizeof__" | "__str__" - | "__sub__" | "__subclasscheck__" | "__subclasses__" | "__subclasshook__" - | "__truediv__" | "__trunc__" | "__weakref__" - | "__xor__" // Overridable sunder names from the `Enum` class. // See: https://docs.python.org/3/library/enum.html#supported-sunder-names | "_add_alias_" @@ -306,5 +313,5 @@ pub(super) fn is_known_dunder_method(method: &str) -> bool { | "_ignore_" | "_order_" | "_generate_next_value_" - ) + ) } diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 8cbb73b032691..b2ea7742018a7 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -1,5 +1,5 @@ //! Rules from [Pylint](https://pypi.org/project/pylint/). -mod helpers; +pub(crate) mod helpers; pub(crate) mod rules; pub mod settings; diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 026e3e44f9919..81bd07baaeb69 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -548,6 +548,13 @@ pub fn is_dunder(id: &str) -> bool { id.starts_with("__") && id.ends_with("__") } +/// Whether a name starts and ends with a single underscore. +/// +/// `_a__` is considered neither a dunder nor a sunder name. +pub fn is_sunder(id: &str) -> bool { + id.starts_with('_') && id.ends_with('_') && !id.starts_with("__") && !id.ends_with("__") +} + /// Return `true` if the [`Stmt`] is an assignment to a dunder (like `__all__`). pub fn is_assignment_to_a_dunder(stmt: &Stmt) -> bool { // Check whether it's an assignment to a dunder, with or without a type diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index c9d1858eccac5..fc801c40ca89b 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -572,7 +572,7 @@ pub trait TypeChecker { /// NOTE: this function doesn't perform more serious type inference, so it won't be able /// to understand if the value gets initialized from a call to a function always returning /// lists. This also implies no interfile analysis. -fn check_type(binding: &Binding, semantic: &SemanticModel) -> bool { +pub fn check_type(binding: &Binding, semantic: &SemanticModel) -> bool { match binding.kind { BindingKind::Assignment => match binding.statement(semantic) { // Given: