From 84ae00c39512ae24423ddbd2e13b06cedc4eddeb Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 10 Aug 2023 20:54:38 -0400 Subject: [PATCH] Allow `os._exit` accesses in `SLF001` (#6490) Closes https://github.com/astral-sh/ruff/issues/6483. --- .../test/fixtures/flake8_self/SLF001.py | 4 + .../rules/private_member_access.rs | 240 +++++++++--------- crates/ruff/src/rules/pylint/rules/logging.rs | 4 +- 3 files changed, 131 insertions(+), 117 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_self/SLF001.py b/crates/ruff/resources/test/fixtures/flake8_self/SLF001.py index 5735e63751..0a5a560182 100644 --- a/crates/ruff/resources/test/fixtures/flake8_self/SLF001.py +++ b/crates/ruff/resources/test/fixtures/flake8_self/SLF001.py @@ -73,3 +73,7 @@ print(foo.__dict__) print(foo.__str__()) print(foo().__class__) print(foo._asdict()) + +import os + +os._exit() diff --git a/crates/ruff/src/rules/flake8_self/rules/private_member_access.rs b/crates/ruff/src/rules/flake8_self/rules/private_member_access.rs index 1f259e51c6..4a3c0cafc3 100644 --- a/crates/ruff/src/rules/flake8_self/rules/private_member_access.rs +++ b/crates/ruff/src/rules/flake8_self/rules/private_member_access.rs @@ -1,8 +1,7 @@ -use ruff_python_ast::{self as ast, Expr, Ranged}; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::call_path::collect_call_path; +use ruff_python_ast::{self as ast, Expr, Ranged}; use ruff_python_semantic::ScopeKind; use crate::checkers::ast::Checker; @@ -62,125 +61,136 @@ impl Violation for PrivateMemberAccess { /// SLF001 pub(crate) fn private_member_access(checker: &mut Checker, expr: &Expr) { - if let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = expr { - if (attr.starts_with("__") && !attr.ends_with("__")) - || (attr.starts_with('_') && !attr.starts_with("__")) + let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = expr else { + return; + }; + + if (attr.starts_with("__") && !attr.ends_with("__")) + || (attr.starts_with('_') && !attr.starts_with("__")) + { + if checker + .settings + .flake8_self + .ignore_names + .contains(attr.as_ref()) { - if checker - .settings - .flake8_self - .ignore_names - .contains(attr.as_ref()) - { + return; + } + + // 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__" + ) { + return; + } + } + + // Allow some documented private methods, like `os._exit()`. + if let Some(call_path) = checker.semantic().resolve_call_path(expr) { + if matches!(call_path.as_slice(), ["os", "_exit"]) { + return; + } + } + + if let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() { + // Ignore `super()` calls. + if let Some(call_path) = collect_call_path(func) { + if matches!(call_path.as_slice(), ["super"]) { + return; + } + } + } + + if let Some(call_path) = collect_call_path(value) { + // Ignore `self` and `cls` accesses. + if matches!(call_path.as_slice(), ["self" | "cls" | "mcs"]) { return; } - // 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__" - ) { - return; - } - } - - if let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() { - // Ignore `super()` calls. - if let Some(call_path) = collect_call_path(func) { - if matches!(call_path.as_slice(), ["super"]) { - return; + // Ignore accesses on class members from _within_ the class. + if checker + .semantic() + .scopes + .iter() + .rev() + .find_map(|scope| match &scope.kind { + ScopeKind::Class(ast::StmtClassDef { name, .. }) => Some(name), + _ => None, + }) + .is_some_and(|name| { + if call_path.as_slice() == [name.as_str()] { + checker + .semantic() + .find_binding(name) + .is_some_and(|binding| { + // TODO(charlie): Could the name ever be bound to a + // _different_ class here? + binding.kind.is_class_definition() + }) + } else { + false } - } - } else if let Some(call_path) = collect_call_path(value) { - // Ignore `self` and `cls` accesses. - if matches!(call_path.as_slice(), ["self" | "cls" | "mcs"]) { - return; - } - - // Ignore accesses on class members from _within_ the class. - if checker - .semantic() - .scopes - .iter() - .rev() - .find_map(|scope| match &scope.kind { - ScopeKind::Class(ast::StmtClassDef { name, .. }) => Some(name), - _ => None, - }) - .is_some_and(|name| { - if call_path.as_slice() == [name.as_str()] { - checker - .semantic() - .find_binding(name) - .is_some_and(|binding| { - // TODO(charlie): Could the name ever be bound to a - // _different_ class here? - binding.kind.is_class_definition() - }) - } else { - false - } - }) - { - return; - } + }) + { + return; } - - checker.diagnostics.push(Diagnostic::new( - PrivateMemberAccess { - access: attr.to_string(), - }, - expr.range(), - )); } + + checker.diagnostics.push(Diagnostic::new( + PrivateMemberAccess { + access: attr.to_string(), + }, + expr.range(), + )); } } diff --git a/crates/ruff/src/rules/pylint/rules/logging.rs b/crates/ruff/src/rules/pylint/rules/logging.rs index 5f778ea5d6..5bbc56b4c2 100644 --- a/crates/ruff/src/rules/pylint/rules/logging.rs +++ b/crates/ruff/src/rules/pylint/rules/logging.rs @@ -111,8 +111,8 @@ pub(crate) fn logging_call(checker: &mut Checker, call: &ast::ExprCall) { let Some(Expr::Constant(ast::ExprConstant { value: Constant::Str(value), .. - })) = call.arguments - .find_positional( 0) else { + })) = call.arguments.find_positional(0) + else { return; };