Skip to content

Commit

Permalink
Allow os._exit accesses in SLF001 (astral-sh#6490)
Browse files Browse the repository at this point in the history 
Closes astral-sh#6483.
  • Loading branch information
@charliermarsh @durumu
charliermarsh authored and durumu committed Aug 12, 2023
1 parent a0534a8 commit 7630104
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 112 deletions.
4 changes: 4 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_self/SLF001.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,7 @@ def __eq__(self, other):
print(foo.__str__())
print(foo().__class__)
print(foo._asdict())

import os

os._exit()
230 changes: 120 additions & 110 deletions crates/ruff/src/rules/flake8_self/rules/private_member_access.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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;
}
}

// 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;
}
}
} else if let Some(call_path) = collect_call_path(value) {
// Ignore `self` and `cls` accesses.
if matches!(call_path.as_slice(), ["self" | "cls" | "mcs"]) {
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
}
})
{
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;
}

checker.diagnostics.push(Diagnostic::new(
PrivateMemberAccess {
access: attr.to_string(),
},
expr.range(),
));
// 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;
}
}

checker.diagnostics.push(Diagnostic::new(
PrivateMemberAccess {
access: attr.to_string(),
},
expr.range(),
));
}
}
4 changes: 2 additions & 2 deletions crates/ruff/src/rules/pylint/rules/logging.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down

0 comments on commit 7630104

Please sign in to comment.