Make SLF diagnostics robust to non-self-named variables

Author: charliermarsh

crates/ruff_linter/resources/test/fixtures/flake8_self/SLF001_2.py

@@ -0,0 +1,45 @@
+# https://github.com/astral-sh/ruff/issues/24275
+
+class C:
+    def false_positive(this):
+        this._x = 0  # fine
+
+    @classmethod
+    def false_positive_2(that):
+        return that._x  # fine
+
+    def false_negative_1(this, self):
+        self._x = 1  # error
+
+    @classmethod
+    def false_positive_3(self):
+        return self._x  # fine
+
+    @staticmethod
+    def false_negative_3(self):
+        return self._x  # error
+
+
+def false_negative_4(self):
+    return self._x  # error
+
+
+def false_negative_5():
+    self = C()
+    return self._x  # error
+
+
+self = C()
+
+
+def false_negative_6():
+    return self._x  # error
+
+
+def false_negative_7(cls):
+    return cls._x  # error
+
+
+def false_negative_8():
+    mcs = C
+    return mcs._x  # error

crates/ruff_linter/resources/test/fixtures/flake8_self/SLF001_custom_decorators.py

@@ -0,0 +1,19 @@
+def custom_classmethod(func):
+    return classmethod(func)
+
+
+def custom_staticmethod(func):
+    return staticmethod(func)
+
+
+class C:
+    def ok(this):
+        return this._x  # fine
+
+    @custom_classmethod
+    def ok_classmethod(this):
+        return this._x  # fine
+
+    @custom_staticmethod
+    def bad_staticmethod(self):
+        return self._x  # error

crates/ruff_linter/src/rules/flake8_self/mod.rs

@@ -16,6 +16,7 @@ mod tests {
 
     #[test_case(Rule::PrivateMemberAccess, Path::new("SLF001.py"))]
     #[test_case(Rule::PrivateMemberAccess, Path::new("SLF001_1.py"))]
+    #[test_case(Rule::PrivateMemberAccess, Path::new("SLF001_2.py"))]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy());
         let diagnostics = test_path(
@@ -40,4 +41,21 @@ mod tests {
         assert_diagnostics!(diagnostics);
         Ok(())
     }
+
+    #[test]
+    fn custom_method_decorators() -> Result<()> {
+        let diagnostics = test_path(
+            Path::new("flake8_self/SLF001_custom_decorators.py"),
+            &settings::LinterSettings {
+                pep8_naming: crate::rules::pep8_naming::settings::Settings {
+                    classmethod_decorators: vec!["custom_classmethod".to_string()],
+                    staticmethod_decorators: vec!["custom_staticmethod".to_string()],
+                    ..Default::default()
+                },
+                ..settings::LinterSettings::for_rule(Rule::PrivateMemberAccess)
+            },
+        )?;
+        assert_diagnostics!(diagnostics);
+        Ok(())
+    }
 }

crates/ruff_linter/src/rules/flake8_self/rules/private_member_access.rs

@@ -2,6 +2,7 @@ use ruff_macros::{ViolationMetadata, derive_message_formats};
 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::analyze::function_type;
 use ruff_python_semantic::analyze::typing;
 use ruff_python_semantic::analyze::typing::TypeChecker;
 use ruff_python_semantic::{BindingKind, ScopeKind, SemanticModel};
@@ -117,13 +118,6 @@ pub(crate) fn private_member_access(checker: &Checker, expr: &Expr) {
         }
     }
 
-    if let Some(name) = UnqualifiedName::from_expr(value) {
-        // Ignore `self` and `cls` accesses.
-        if matches!(name.segments(), ["self" | "cls" | "mcs"]) {
-            return;
-        }
-    }
-
     if let Expr::Name(name) = value.as_ref() {
         // Ignore accesses on class members from _within_ the class.
         if semantic
@@ -140,7 +134,7 @@ pub(crate) fn private_member_access(checker: &Checker, expr: &Expr) {
             return;
         }
 
-        if is_same_class_instance(name, semantic) {
+        if is_same_class_instance(checker, name) {
             return;
         }
     }
@@ -177,7 +171,12 @@ pub(crate) fn private_member_access(checker: &Checker, expr: &Expr) {
 ///
 /// 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 {
+fn is_same_class_instance(checker: &Checker, name: &ast::ExprName) -> bool {
+    if is_method_receiver(checker, name) {
+        return true;
+    }
+
+    let semantic = checker.semantic();
     let Some(binding_id) = semantic.resolve_name(name) else {
         return false;
     };
@@ -186,6 +185,55 @@ fn is_same_class_instance(name: &ast::ExprName, semantic: &SemanticModel) -> boo
     typing::check_type::<SameClassInstanceChecker>(binding, semantic)
 }
 
+fn is_method_receiver(checker: &Checker, name: &ast::ExprName) -> bool {
+    let semantic = checker.semantic();
+
+    let Some(binding_id) = semantic.resolve_name(name) else {
+        return false;
+    };
+    let binding = semantic.binding(binding_id);
+
+    if !matches!(binding.kind, BindingKind::Argument) {
+        return false;
+    }
+
+    let Some(ast::Stmt::FunctionDef(function)) = binding.statement(semantic) else {
+        return false;
+    };
+
+    let Some(first_parameter) = function
+        .parameters
+        .posonlyargs
+        .first()
+        .or_else(|| function.parameters.args.first())
+    else {
+        return false;
+    };
+
+    if binding.range != first_parameter.parameter.name.range() {
+        return false;
+    }
+
+    let scope = &semantic.scopes[binding.scope];
+    let Some(parent_scope) = semantic.first_non_type_parent_scope(scope) else {
+        return false;
+    };
+
+    matches!(
+        function_type::classify(
+            &function.name,
+            &function.decorator_list,
+            parent_scope,
+            semantic,
+            &checker.settings().pep8_naming.classmethod_decorators,
+            &checker.settings().pep8_naming.staticmethod_decorators,
+        ),
+        function_type::FunctionType::Method
+            | function_type::FunctionType::ClassMethod
+            | function_type::FunctionType::NewMethod
+    )
+}
+
 struct SameClassInstanceChecker;
 
 impl SameClassInstanceChecker {

crates/ruff_linter/src/rules/flake8_self/snapshots/ruff_linter__rules__flake8_self__tests__custom_method_decorators.snap

@@ -0,0 +1,11 @@
+---
+source: crates/ruff_linter/src/rules/flake8_self/mod.rs
+---
+SLF001 Private member accessed: `_x`
+  --> SLF001_custom_decorators.py:19:16
+   |
+17 |     @custom_staticmethod
+18 |     def bad_staticmethod(self):
+19 |         return self._x  # error
+   |                ^^^^^^^
+   |

crates/ruff_linter/src/rules/flake8_self/snapshots/ruff_linter__rules__flake8_self__tests__private-member-access_SLF001_2.py.snap

@@ -0,0 +1,63 @@
+---
+source: crates/ruff_linter/src/rules/flake8_self/mod.rs
+---
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:12:9
+   |
+11 |     def false_negative_1(this, self):
+12 |         self._x = 1  # error
+   |         ^^^^^^^
+13 |
+14 |     @classmethod
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:20:16
+   |
+18 |     @staticmethod
+19 |     def false_negative_3(self):
+20 |         return self._x  # error
+   |                ^^^^^^^
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:24:12
+   |
+23 | def false_negative_4(self):
+24 |     return self._x  # error
+   |            ^^^^^^^
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:29:12
+   |
+27 | def false_negative_5():
+28 |     self = C()
+29 |     return self._x  # error
+   |            ^^^^^^^
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:36:12
+   |
+35 | def false_negative_6():
+36 |     return self._x  # error
+   |            ^^^^^^^
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:40:12
+   |
+39 | def false_negative_7(cls):
+40 |     return cls._x  # error
+   |            ^^^^^^
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:45:12
+   |
+43 | def false_negative_8():
+44 |     mcs = C
+45 |     return mcs._x  # error
+   |            ^^^^^^
+   |