diff --git a/resources/test/fixtures/flake8_simplify/SIM108.py b/resources/test/fixtures/flake8_simplify/SIM108.py index b71124c52d..d0cb3fc151 100644 --- a/resources/test/fixtures/flake8_simplify/SIM108.py +++ b/resources/test/fixtures/flake8_simplify/SIM108.py @@ -29,3 +29,20 @@ else: b = 1 else: b = 2 + +import sys + +if sys.version_info >= (3, 9): + randbytes = random.randbytes +else: + randbytes = _get_random_bytes + +if sys.platform == "darwin": + randbytes = random.randbytes +else: + randbytes = _get_random_bytes + +if sys.platform.startswith("linux"): + randbytes = random.randbytes +else: + randbytes = _get_random_bytes diff --git a/src/ast/helpers.rs b/src/ast/helpers.rs index 3c46b7c419..801ca8987a 100644 --- a/src/ast/helpers.rs +++ b/src/ast/helpers.rs @@ -179,6 +179,221 @@ pub fn match_call_path( } } +/// Return `true` if the `Expr` contains a reference to `${module}.${target}`. +pub fn contains_call_path( + expr: &Expr, + module: &str, + member: &str, + import_aliases: &FxHashMap<&str, &str>, + from_imports: &FxHashMap<&str, FxHashSet<&str>>, +) -> bool { + let call_path = collect_call_paths(expr); + if !call_path.is_empty() { + if match_call_path( + &dealias_call_path(call_path, import_aliases), + module, + member, + from_imports, + ) { + return true; + } + } + + match &expr.node { + ExprKind::BoolOp { values, .. } => values + .iter() + .any(|expr| contains_call_path(expr, module, member, import_aliases, from_imports)), + ExprKind::NamedExpr { target, value } => { + contains_call_path(target, module, member, import_aliases, from_imports) + || contains_call_path(value, module, member, import_aliases, from_imports) + } + ExprKind::BinOp { left, right, .. } => { + contains_call_path(left, module, member, import_aliases, from_imports) + || contains_call_path(right, module, member, import_aliases, from_imports) + } + ExprKind::UnaryOp { operand, .. } => { + contains_call_path(operand, module, member, import_aliases, from_imports) + } + ExprKind::Lambda { body, .. } => { + contains_call_path(body, module, member, import_aliases, from_imports) + } + ExprKind::IfExp { test, body, orelse } => { + contains_call_path(test, module, member, import_aliases, from_imports) + || contains_call_path(body, module, member, import_aliases, from_imports) + || contains_call_path(orelse, module, member, import_aliases, from_imports) + } + ExprKind::Dict { keys, values } => values + .iter() + .chain(keys.iter()) + .any(|expr| contains_call_path(expr, module, member, import_aliases, from_imports)), + ExprKind::Set { elts } => elts + .iter() + .any(|expr| contains_call_path(expr, module, member, import_aliases, from_imports)), + ExprKind::ListComp { elt, generators } => { + contains_call_path(elt, module, member, import_aliases, from_imports) + || generators.iter().any(|generator| { + contains_call_path( + &generator.target, + module, + member, + import_aliases, + from_imports, + ) || contains_call_path( + &generator.iter, + module, + member, + import_aliases, + from_imports, + ) || generator.ifs.iter().any(|expr| { + contains_call_path(expr, module, member, import_aliases, from_imports) + }) + }) + } + ExprKind::SetComp { elt, generators } => { + contains_call_path(elt, module, member, import_aliases, from_imports) + || generators.iter().any(|generator| { + contains_call_path( + &generator.target, + module, + member, + import_aliases, + from_imports, + ) || contains_call_path( + &generator.iter, + module, + member, + import_aliases, + from_imports, + ) || generator.ifs.iter().any(|expr| { + contains_call_path(expr, module, member, import_aliases, from_imports) + }) + }) + } + ExprKind::DictComp { + key, + value, + generators, + } => { + contains_call_path(key, module, member, import_aliases, from_imports) + || contains_call_path(value, module, member, import_aliases, from_imports) + || generators.iter().any(|generator| { + contains_call_path( + &generator.target, + module, + member, + import_aliases, + from_imports, + ) || contains_call_path( + &generator.iter, + module, + member, + import_aliases, + from_imports, + ) || generator.ifs.iter().any(|expr| { + contains_call_path(expr, module, member, import_aliases, from_imports) + }) + }) + } + ExprKind::GeneratorExp { elt, generators } => { + contains_call_path(elt, module, member, import_aliases, from_imports) || { + generators.iter().any(|generator| { + contains_call_path( + &generator.target, + module, + member, + import_aliases, + from_imports, + ) || contains_call_path( + &generator.iter, + module, + member, + import_aliases, + from_imports, + ) || generator.ifs.iter().any(|expr| { + contains_call_path(expr, module, member, import_aliases, from_imports) + }) + }) + } + } + ExprKind::Await { value } => { + contains_call_path(value, module, member, import_aliases, from_imports) + } + ExprKind::Yield { value } => value.as_ref().map_or(false, |value| { + contains_call_path(value, module, member, import_aliases, from_imports) + }), + ExprKind::YieldFrom { value } => { + contains_call_path(value, module, member, import_aliases, from_imports) + } + ExprKind::Compare { + left, + ops: _, + comparators, + } => { + contains_call_path(left, module, member, import_aliases, from_imports) + || comparators.iter().any(|expr| { + contains_call_path(expr, module, member, import_aliases, from_imports) + }) + } + ExprKind::Call { + func, + args, + keywords, + } => { + contains_call_path(func, module, member, import_aliases, from_imports) + || args.iter().any(|expr| { + contains_call_path(expr, module, member, import_aliases, from_imports) + }) + || keywords.iter().any(|keyword| { + contains_call_path( + &keyword.node.value, + module, + member, + import_aliases, + from_imports, + ) + }) + } + ExprKind::FormattedValue { + value, format_spec, .. + } => { + contains_call_path(value, module, member, import_aliases, from_imports) + || format_spec.as_ref().map_or(false, |value| { + contains_call_path(value, module, member, import_aliases, from_imports) + }) + } + ExprKind::JoinedStr { values } => values + .iter() + .any(|expr| contains_call_path(expr, module, member, import_aliases, from_imports)), + ExprKind::Constant { .. } => false, + ExprKind::Attribute { value, .. } => { + contains_call_path(value, module, member, import_aliases, from_imports) + } + ExprKind::Subscript { value, slice, .. } => { + contains_call_path(value, module, member, import_aliases, from_imports) + || contains_call_path(slice, module, member, import_aliases, from_imports) + } + ExprKind::Starred { value, ctx: _ } => { + contains_call_path(value, module, member, import_aliases, from_imports) + } + ExprKind::Name { .. } => false, + ExprKind::List { elts, .. } => elts + .iter() + .any(|expr| contains_call_path(expr, module, member, import_aliases, from_imports)), + ExprKind::Tuple { elts, .. } => elts + .iter() + .any(|expr| contains_call_path(expr, module, member, import_aliases, from_imports)), + ExprKind::Slice { lower, upper, step } => { + lower.as_ref().map_or(false, |value| { + contains_call_path(value, module, member, import_aliases, from_imports) + }) || upper.as_ref().map_or(false, |value| { + contains_call_path(value, module, member, import_aliases, from_imports) + }) || step.as_ref().map_or(false, |value| { + contains_call_path(value, module, member, import_aliases, from_imports) + }) + } + } +} + static DUNDER_REGEX: Lazy = Lazy::new(|| Regex::new(r"__[^\s]+__").unwrap()); /// Return `true` if the `Stmt` is an assignment to a dunder (like `__all__`). diff --git a/src/flake8_simplify/rules/ast_if.rs b/src/flake8_simplify/rules/ast_if.rs index 529e51d178..ebf31729c2 100644 --- a/src/flake8_simplify/rules/ast_if.rs +++ b/src/flake8_simplify/rules/ast_if.rs @@ -1,6 +1,8 @@ use rustpython_ast::{Constant, Expr, ExprKind, Stmt, StmtKind}; -use crate::ast::helpers::{create_expr, create_stmt, unparse_expr, unparse_stmt}; +use crate::ast::helpers::{ + contains_call_path, create_expr, create_stmt, unparse_expr, unparse_stmt, +}; use crate::ast::types::Range; use crate::autofix::Fix; use crate::checkers::ast::Checker; @@ -144,7 +146,28 @@ pub fn use_ternary_operator(checker: &mut Checker, stmt: &Stmt, parent: Option<& return; } - let target_var = &body_targets[0]; + // Avoid suggesting ternary for `if sys.version_info >= ...`-style checks. + if contains_call_path( + test, + "sys", + "version_info", + &checker.import_aliases, + &checker.from_imports, + ) { + return; + } + + // Avoid suggesting ternary for `if sys.platform.startswith("...")`-style + // checks. + if contains_call_path( + test, + "sys", + "platform", + &checker.import_aliases, + &checker.from_imports, + ) { + return; + } // It's part of a bigger if-elif block: // https://github.com/MartinThoma/flake8-simplify/issues/115 @@ -176,6 +199,7 @@ pub fn use_ternary_operator(checker: &mut Checker, stmt: &Stmt, parent: Option<& } } + let target_var = &body_targets[0]; let ternary = ternary(target_var, body_value, test, orelse_value); let content = unparse_stmt(&ternary, checker.style); let mut diagnostic = Diagnostic::new(