diff --git a/resources/test/fixtures/pycodestyle/constant_literals.py b/resources/test/fixtures/pycodestyle/constant_literals.py new file mode 100644 index 0000000000..5f3e722277 --- /dev/null +++ b/resources/test/fixtures/pycodestyle/constant_literals.py @@ -0,0 +1,39 @@ +### +# Errors +### +if "abc" is "def": # F632 (fix) + pass +if "abc" is None: # F632 (fix, but leaves behind unfixable E711) + pass +if None is "abc": # F632 (fix, but leaves behind unfixable E711) + pass +if "abc" is False: # F632 (fix, but leaves behind unfixable E712) + pass +if False is "abc": # F632 (fix, but leaves behind unfixable E712) + pass +if False == None: # E711, E712 (fix) + pass +if None == False: # E711, E712 (fix) + pass + +### +# Unfixable errors +### +if "abc" == None: # E711 + pass +if None == "abc": # E711 + pass +if "abc" == False: # E712 + pass +if False == "abc": # E712 + pass + +### +# Non-errors +### +if "def" == "abc": + pass +if False is None: + pass +if None is False: + pass diff --git a/src/ast/helpers.rs b/src/ast/helpers.rs index 3866fcff13..e95cc8f258 100644 --- a/src/ast/helpers.rs +++ b/src/ast/helpers.rs @@ -3,7 +3,7 @@ use once_cell::sync::Lazy; use regex::Regex; use rustc_hash::{FxHashMap, FxHashSet}; use rustpython_ast::{ - Arguments, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Location, Stmt, StmtKind, + Arguments, Constant, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Location, Stmt, StmtKind, }; use rustpython_parser::lexer; use rustpython_parser::lexer::Tok; @@ -152,15 +152,12 @@ pub fn match_call_path( static DUNDER_REGEX: Lazy = Lazy::new(|| Regex::new(r"__[^\s]+__").unwrap()); -pub fn is_assignment_to_a_dunder(node: &StmtKind) -> bool { +/// Return `true` if the `Stmt` is an assignment to a dunder (like `__all__`). +pub fn is_assignment_to_a_dunder(stmt: &Stmt) -> bool { // Check whether it's an assignment to a dunder, with or without a type // annotation. This is what pycodestyle (as of 2.9.1) does. - match node { - StmtKind::Assign { - targets, - value: _, - type_comment: _, - } => { + match &stmt.node { + StmtKind::Assign { targets, .. } => { if targets.len() != 1 { return false; } @@ -169,12 +166,7 @@ pub fn is_assignment_to_a_dunder(node: &StmtKind) -> bool { _ => false, } } - StmtKind::AnnAssign { - target, - annotation: _, - value: _, - simple: _, - } => match &target.node { + StmtKind::AnnAssign { target, .. } => match &target.node { ExprKind::Name { id, ctx: _ } => DUNDER_REGEX.is_match(id), _ => false, }, @@ -182,6 +174,32 @@ pub fn is_assignment_to_a_dunder(node: &StmtKind) -> bool { } } +/// Return `true` if the `Expr` is a singleton (`None`, `True`, `False`, or +/// `...`). +pub fn is_singleton(expr: &Expr) -> bool { + matches!( + expr.node, + ExprKind::Constant { + value: Constant::None | Constant::Bool(_) | Constant::Ellipsis, + .. + } + ) +} + +/// Return `true` if the `Expr` is a constant or tuple of constants. +pub fn is_constant(expr: &Expr) -> bool { + match &expr.node { + ExprKind::Constant { .. } => true, + ExprKind::Tuple { elts, .. } => elts.iter().all(is_constant), + _ => false, + } +} + +/// Return `true` if the `Expr` is a non-singleton constant. +pub fn is_constant_non_singleton(expr: &Expr) -> bool { + is_constant(expr) && !is_singleton(expr) +} + /// Extract the names of all handled exceptions. pub fn extract_handler_names(handlers: &[Excepthandler]) -> Vec> { let mut handler_names = vec![]; @@ -232,7 +250,6 @@ pub fn collect_arg_names<'a>(arguments: &'a Arguments) -> FxHashSet<&'a str> { /// Returns `true` if a call is an argumented `super` invocation. pub fn is_super_call_with_arguments(func: &Expr, args: &[Expr]) -> bool { - // Check: is this a `super` call? if let ExprKind::Name { id, .. } = &func.node { id == "super" && !args.is_empty() } else { diff --git a/src/check_ast.rs b/src/check_ast.rs index 003ca4aa50..e952edf3ac 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -223,10 +223,10 @@ where StmtKind::Import { .. } => { self.futures_allowed = false; } - node => { + _ => { self.futures_allowed = false; if !self.seen_import_boundary - && !helpers::is_assignment_to_a_dunder(node) + && !helpers::is_assignment_to_a_dunder(stmt) && !operations::in_nested_block( &mut self.parents.iter().rev().map(|node| node.0), ) diff --git a/src/pycodestyle/mod.rs b/src/pycodestyle/mod.rs index 332967b123..770b67de23 100644 --- a/src/pycodestyle/mod.rs +++ b/src/pycodestyle/mod.rs @@ -44,4 +44,16 @@ mod tests { insta::assert_yaml_snapshot!(snapshot, checks); Ok(()) } + + #[test] + fn constant_literals() -> Result<()> { + let mut checks = test_path( + Path::new("./resources/test/fixtures/pycodestyle/constant_literals.py"), + &settings::Settings::for_rules(vec![CheckCode::E711, CheckCode::E712, CheckCode::F632]), + true, + )?; + checks.sort_by_key(|check| check.location); + insta::assert_yaml_snapshot!(checks); + Ok(()) + } } diff --git a/src/pycodestyle/plugins.rs b/src/pycodestyle/plugins.rs index b54d439d45..b9e4b5d855 100644 --- a/src/pycodestyle/plugins.rs +++ b/src/pycodestyle/plugins.rs @@ -5,6 +5,7 @@ use rustc_hash::FxHashMap; use rustpython_ast::{Arguments, Location, StmtKind}; use rustpython_parser::ast::{Cmpop, Constant, Expr, ExprKind, Stmt, Unaryop}; +use crate::ast::helpers; use crate::ast::helpers::{match_leading_content, match_trailing_content}; use crate::ast::types::Range; use crate::ast::whitespace::leading_space; @@ -46,9 +47,10 @@ pub fn literal_comparisons( let mut checks: Vec = vec![]; let op = ops.first().unwrap(); - let comparator = left; // Check `left`. + let mut comparator = left; + let next = &comparators[0]; if check_none_comparisons && matches!( comparator.node, @@ -63,7 +65,7 @@ pub fn literal_comparisons( CheckKind::NoneComparison(RejectedCmpop::Eq), Range::from_located(comparator), ); - if checker.patch(check.kind.code()) { + if checker.patch(check.kind.code()) && !helpers::is_constant_non_singleton(next) { bad_ops.insert(0, Cmpop::Is); } checks.push(check); @@ -73,7 +75,7 @@ pub fn literal_comparisons( CheckKind::NoneComparison(RejectedCmpop::NotEq), Range::from_located(comparator), ); - if checker.patch(check.kind.code()) { + if checker.patch(check.kind.code()) && !helpers::is_constant_non_singleton(next) { bad_ops.insert(0, Cmpop::IsNot); } checks.push(check); @@ -91,7 +93,7 @@ pub fn literal_comparisons( CheckKind::TrueFalseComparison(value, RejectedCmpop::Eq), Range::from_located(comparator), ); - if checker.patch(check.kind.code()) { + if checker.patch(check.kind.code()) && !helpers::is_constant_non_singleton(next) { bad_ops.insert(0, Cmpop::Is); } checks.push(check); @@ -101,7 +103,7 @@ pub fn literal_comparisons( CheckKind::TrueFalseComparison(value, RejectedCmpop::NotEq), Range::from_located(comparator), ); - if checker.patch(check.kind.code()) { + if checker.patch(check.kind.code()) && !helpers::is_constant_non_singleton(next) { bad_ops.insert(0, Cmpop::IsNot); } checks.push(check); @@ -110,10 +112,10 @@ pub fn literal_comparisons( } // Check each comparator in order. - for (idx, (op, comparator)) in izip!(ops, comparators).enumerate() { + for (idx, (op, next)) in izip!(ops, comparators).enumerate() { if check_none_comparisons && matches!( - comparator.node, + next.node, ExprKind::Constant { value: Constant::None, kind: None @@ -123,9 +125,11 @@ pub fn literal_comparisons( if matches!(op, Cmpop::Eq) { let check = Check::new( CheckKind::NoneComparison(RejectedCmpop::Eq), - Range::from_located(comparator), + Range::from_located(next), ); - if checker.patch(check.kind.code()) { + if checker.patch(check.kind.code()) + && !helpers::is_constant_non_singleton(comparator) + { bad_ops.insert(idx, Cmpop::Is); } checks.push(check); @@ -133,9 +137,11 @@ pub fn literal_comparisons( if matches!(op, Cmpop::NotEq) { let check = Check::new( CheckKind::NoneComparison(RejectedCmpop::NotEq), - Range::from_located(comparator), + Range::from_located(next), ); - if checker.patch(check.kind.code()) { + if checker.patch(check.kind.code()) + && !helpers::is_constant_non_singleton(comparator) + { bad_ops.insert(idx, Cmpop::IsNot); } checks.push(check); @@ -146,14 +152,16 @@ pub fn literal_comparisons( if let ExprKind::Constant { value: Constant::Bool(value), kind: None, - } = comparator.node + } = next.node { if matches!(op, Cmpop::Eq) { let check = Check::new( CheckKind::TrueFalseComparison(value, RejectedCmpop::Eq), - Range::from_located(comparator), + Range::from_located(next), ); - if checker.patch(check.kind.code()) { + if checker.patch(check.kind.code()) + && !helpers::is_constant_non_singleton(comparator) + { bad_ops.insert(idx, Cmpop::Is); } checks.push(check); @@ -161,15 +169,19 @@ pub fn literal_comparisons( if matches!(op, Cmpop::NotEq) { let check = Check::new( CheckKind::TrueFalseComparison(value, RejectedCmpop::NotEq), - Range::from_located(comparator), + Range::from_located(next), ); - if checker.patch(check.kind.code()) { + if checker.patch(check.kind.code()) + && !helpers::is_constant_non_singleton(comparator) + { bad_ops.insert(idx, Cmpop::IsNot); } checks.push(check); } } } + + comparator = next; } if !bad_ops.is_empty() { diff --git a/src/pycodestyle/snapshots/ruff__pycodestyle__tests__constant_literals.snap b/src/pycodestyle/snapshots/ruff__pycodestyle__tests__constant_literals.snap new file mode 100644 index 0000000000..fbbd3492da --- /dev/null +++ b/src/pycodestyle/snapshots/ruff__pycodestyle__tests__constant_literals.snap @@ -0,0 +1,188 @@ +--- +source: src/pycodestyle/mod.rs +expression: checks +--- +- kind: IsLiteral + location: + row: 4 + column: 3 + end_location: + row: 4 + column: 17 + fix: + content: "==" + location: + row: 4 + column: 9 + end_location: + row: 4 + column: 11 +- kind: IsLiteral + location: + row: 6 + column: 3 + end_location: + row: 6 + column: 16 + fix: + content: "==" + location: + row: 6 + column: 9 + end_location: + row: 6 + column: 11 +- kind: IsLiteral + location: + row: 8 + column: 3 + end_location: + row: 8 + column: 16 + fix: + content: "==" + location: + row: 8 + column: 8 + end_location: + row: 8 + column: 10 +- kind: IsLiteral + location: + row: 10 + column: 3 + end_location: + row: 10 + column: 17 + fix: + content: "==" + location: + row: 10 + column: 9 + end_location: + row: 10 + column: 11 +- kind: IsLiteral + location: + row: 12 + column: 3 + end_location: + row: 12 + column: 17 + fix: + content: "==" + location: + row: 12 + column: 9 + end_location: + row: 12 + column: 11 +- kind: + TrueFalseComparison: + - false + - Eq + location: + row: 14 + column: 3 + end_location: + row: 14 + column: 8 + fix: + content: False is None + location: + row: 14 + column: 3 + end_location: + row: 14 + column: 16 +- kind: + NoneComparison: Eq + location: + row: 14 + column: 12 + end_location: + row: 14 + column: 16 + fix: + content: False is None + location: + row: 14 + column: 3 + end_location: + row: 14 + column: 16 +- kind: + NoneComparison: Eq + location: + row: 16 + column: 3 + end_location: + row: 16 + column: 7 + fix: + content: None is False + location: + row: 16 + column: 3 + end_location: + row: 16 + column: 16 +- kind: + TrueFalseComparison: + - false + - Eq + location: + row: 16 + column: 11 + end_location: + row: 16 + column: 16 + fix: + content: None is False + location: + row: 16 + column: 3 + end_location: + row: 16 + column: 16 +- kind: + NoneComparison: Eq + location: + row: 22 + column: 12 + end_location: + row: 22 + column: 16 + fix: ~ +- kind: + NoneComparison: Eq + location: + row: 24 + column: 3 + end_location: + row: 24 + column: 7 + fix: ~ +- kind: + TrueFalseComparison: + - false + - Eq + location: + row: 26 + column: 12 + end_location: + row: 26 + column: 17 + fix: ~ +- kind: + TrueFalseComparison: + - false + - Eq + location: + row: 28 + column: 3 + end_location: + row: 28 + column: 8 + fix: ~ + diff --git a/src/pyflakes/plugins/invalid_literal_comparisons.rs b/src/pyflakes/plugins/invalid_literal_comparisons.rs index ab7094480f..2e80a9a373 100644 --- a/src/pyflakes/plugins/invalid_literal_comparisons.rs +++ b/src/pyflakes/plugins/invalid_literal_comparisons.rs @@ -1,6 +1,6 @@ use itertools::izip; use once_cell::unsync::Lazy; -use rustpython_ast::{Cmpop, Constant, Expr, ExprKind}; +use rustpython_ast::{Cmpop, Expr}; use crate::ast::helpers; use crate::ast::operations::locate_cmpops; @@ -9,28 +9,6 @@ use crate::autofix::Fix; use crate::check_ast::Checker; use crate::checks::{Check, CheckKind}; -fn is_singleton(expr: &Expr) -> bool { - matches!( - expr.node, - ExprKind::Constant { - value: Constant::None | Constant::Bool(_) | Constant::Ellipsis, - .. - } - ) -} - -fn is_constant(expr: &Expr) -> bool { - match &expr.node { - ExprKind::Constant { .. } => true, - ExprKind::Tuple { elts, .. } => elts.iter().all(is_constant), - _ => false, - } -} - -fn is_constant_non_singleton(expr: &Expr) -> bool { - is_constant(expr) && !is_singleton(expr) -} - /// F632 pub fn invalid_literal_comparison( checker: &mut Checker, @@ -43,7 +21,8 @@ pub fn invalid_literal_comparison( let mut left = left; for (index, (op, right)) in izip!(ops, comparators).enumerate() { if matches!(op, Cmpop::Is | Cmpop::IsNot) - && (is_constant_non_singleton(left) || is_constant_non_singleton(right)) + && (helpers::is_constant_non_singleton(left) + || helpers::is_constant_non_singleton(right)) { let mut check = Check::new(CheckKind::IsLiteral, location); if checker.patch(check.kind.code()) {