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,43 @@
+class C:
+    def non_self_named_method_receiver(this):
+        this._x = 0  # stable: fine, preview: fine
+
+    @classmethod
+    def non_self_named_classmethod_receiver(that):
+        return that._x  # stable: fine, preview: fine
+
+    def non_receiver_named_self_parameter(this, self):
+        self._x = 1  # stable: fine, preview: error
+
+    @classmethod
+    def classmethod_named_self(self):
+        return self._x  # stable: fine, preview: fine
+
+    @staticmethod
+    def staticmethod_named_self(self):
+        return self._x  # stable: fine, preview: error
+
+
+def top_level_self_parameter(self):
+    return self._x  # stable: fine, preview: error
+
+
+def local_self_binding():
+    self = C()
+    return self._x  # stable: fine, preview: error
+
+
+self = C()
+
+
+def global_self_binding():
+    return self._x  # stable: fine, preview: error
+
+
+def top_level_cls_parameter(cls):
+    return cls._x  # stable: fine, preview: error
+
+
+def local_mcs_binding():
+    mcs = C
+    return mcs._x  # stable: fine, preview: 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/preview.rs

@@ -218,6 +218,11 @@ pub(crate) const fn is_refined_submodule_import_match_enabled(settings: &LinterS
     settings.preview.is_enabled()
 }
 
+// https://github.com/astral-sh/ruff/issues/24275
+pub(crate) const fn is_slf001_self_cls_mcs_enforcement_enabled(settings: &LinterSettings) -> bool {
+    settings.preview.is_enabled()
+}
+
 // https://github.com/astral-sh/ruff/pull/20660
 pub(crate) const fn is_type_var_default_enabled(settings: &LinterSettings) -> bool {
     settings.preview.is_enabled()

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

@@ -8,6 +8,7 @@ mod tests {
 
     use crate::registry::Rule;
     use crate::rules::flake8_self;
+    use crate::settings::types::PreviewMode;
     use crate::test::test_path;
     use crate::{assert_diagnostics, settings};
     use anyhow::Result;
@@ -16,6 +17,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(
@@ -26,6 +28,20 @@ mod tests {
         Ok(())
     }
 
+    #[test_case(Rule::PrivateMemberAccess, Path::new("SLF001_2.py"))]
+    fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
+        let snapshot = format!("preview__{}_{}", rule_code.name(), path.to_string_lossy());
+        let diagnostics = test_path(
+            Path::new("flake8_self").join(path).as_path(),
+            &settings::LinterSettings {
+                preview: PreviewMode::Enabled,
+                ..settings::LinterSettings::for_rule(rule_code)
+            },
+        )?;
+        assert_diagnostics!(snapshot, diagnostics);
+        Ok(())
+    }
+
     #[test]
     fn ignore_names() -> Result<()> {
         let diagnostics = test_path(
@@ -40,4 +56,22 @@ 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 {
+                preview: PreviewMode::Enabled,
+                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,13 +2,15 @@ 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};
 use ruff_text_size::Ranged;
 
 use crate::Violation;
 use crate::checkers::ast::Checker;
+use crate::preview::is_slf001_self_cls_mcs_enforcement_enabled;
 use crate::rules::pylint::helpers::is_dunder_operator_method;
 
 /// ## What it does
@@ -76,6 +78,7 @@ pub(crate) fn private_member_access(checker: &Checker, expr: &Expr) {
 
     let semantic = checker.semantic();
     let current_scope = semantic.current_scope();
+    let enforce_self_cls_mcs = is_slf001_self_cls_mcs_enforcement_enabled(checker.settings());
 
     if semantic.in_annotation() {
         return;
@@ -117,11 +120,11 @@ 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 !enforce_self_cls_mcs
+        && let Some(name) = UnqualifiedName::from_expr(value)
+        && matches!(name.segments(), ["self" | "cls" | "mcs"])
+    {
+        return;
     }
 
     if let Expr::Name(name) = value.as_ref() {
@@ -140,7 +143,12 @@ pub(crate) fn private_member_access(checker: &Checker, expr: &Expr) {
             return;
         }
 
-        if is_same_class_instance(name, semantic) {
+        if is_same_class_instance(
+            name,
+            semantic,
+            &checker.settings().pep8_naming.classmethod_decorators,
+            &checker.settings().pep8_naming.staticmethod_decorators,
+        ) {
             return;
         }
     }
@@ -177,7 +185,21 @@ 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(
+    name: &ast::ExprName,
+    semantic: &SemanticModel,
+    classmethod_decorators: &[String],
+    staticmethod_decorators: &[String],
+) -> bool {
+    if is_method_receiver(
+        name,
+        semantic,
+        classmethod_decorators,
+        staticmethod_decorators,
+    ) {
+        return true;
+    }
+
     let Some(binding_id) = semantic.resolve_name(name) else {
         return false;
     };
@@ -186,6 +208,60 @@ fn is_same_class_instance(name: &ast::ExprName, semantic: &SemanticModel) -> boo
     typing::check_type::<SameClassInstanceChecker>(binding, semantic)
 }
 
+/// Return `true` if `name` resolves to the first parameter of a syntactic
+/// method receiver, including class methods and `__new__`.
+fn is_method_receiver(
+    name: &ast::ExprName,
+    semantic: &SemanticModel,
+    classmethod_decorators: &[String],
+    staticmethod_decorators: &[String],
+) -> bool {
+    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,
+            classmethod_decorators,
+            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__preview__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:10:9
+   |
+ 9 |     def non_receiver_named_self_parameter(this, self):
+10 |         self._x = 1  # stable: fine, preview: error
+   |         ^^^^^^^
+11 |
+12 |     @classmethod
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:18:16
+   |
+16 |     @staticmethod
+17 |     def staticmethod_named_self(self):
+18 |         return self._x  # stable: fine, preview: error
+   |                ^^^^^^^
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:22:12
+   |
+21 | def top_level_self_parameter(self):
+22 |     return self._x  # stable: fine, preview: error
+   |            ^^^^^^^
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:27:12
+   |
+25 | def local_self_binding():
+26 |     self = C()
+27 |     return self._x  # stable: fine, preview: error
+   |            ^^^^^^^
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:34:12
+   |
+33 | def global_self_binding():
+34 |     return self._x  # stable: fine, preview: error
+   |            ^^^^^^^
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:38:12
+   |
+37 | def top_level_cls_parameter(cls):
+38 |     return cls._x  # stable: fine, preview: error
+   |            ^^^^^^
+   |
+
+SLF001 Private member accessed: `_x`
+  --> SLF001_2.py:43:12
+   |
+41 | def local_mcs_binding():
+42 |     mcs = C
+43 |     return mcs._x  # stable: fine, preview: 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,4 @@
+---
+source: crates/ruff_linter/src/rules/flake8_self/mod.rs
+---
+