From 7318bf8b1274a62eb6fa1c5bc31bc3cdc3f5a65a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 5 Sep 2022 23:19:05 -0400 Subject: [PATCH] Implement E711 and E712 --- Cargo.lock | 1 + Cargo.toml | 1 + README.md | 2 + examples/generate_rules_table.rs | 8 +- resources/test/fixtures/E711.py | 11 +++ resources/test/fixtures/E712.py | 14 ++++ resources/test/fixtures/pyproject.toml | 2 + src/check_ast.rs | 105 ++++++++++++++++++++++++- src/checks.rs | 48 ++++++++++- src/lib.rs | 2 + src/linter.rs | 76 +++++++++++++++++- src/pyproject.rs | 2 + 12 files changed, 265 insertions(+), 7 deletions(-) create mode 100644 resources/test/fixtures/E711.py create mode 100644 resources/test/fixtures/E712.py diff --git a/Cargo.lock b/Cargo.lock index 1613b4f5de..3aadb299b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1758,6 +1758,7 @@ dependencies = [ "fern", "filetime", "glob", + "itertools", "log", "notify", "once_cell", diff --git a/Cargo.toml b/Cargo.toml index 8f70423562..fd95edea38 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,7 @@ dirs = { version = "4.0.0" } fern = { version = "0.6.1" } filetime = { version = "0.2.17" } glob = { version = "0.3.0" } +itertools = "0.10.3" log = { version = "0.4.17" } notify = { version = "4.0.17" } once_cell = { version = "1.13.1" } diff --git a/README.md b/README.md index 59c8f79aa9..50b0e5580a 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,8 @@ lint rules that are obviated by Black (e.g., stylistic rules). | ---- | ----- | ------- | | E402 | ModuleImportNotAtTopOfFile | Module level import not at top of file | | E501 | LineTooLong | Line too long | +| E711 | NoneComparison | Comparison to `None` should be `cond is None` | +| E712 | TrueFalseComparison | Comparison to `True` should be `cond is True` | | E731 | DoNotAssignLambda | Do not assign a lambda expression, use a def | | E902 | IOError | No such file or directory: `...` | | F401 | UnusedImport | `...` imported but unused | diff --git a/examples/generate_rules_table.rs b/examples/generate_rules_table.rs index c6bed78158..c7af58839a 100644 --- a/examples/generate_rules_table.rs +++ b/examples/generate_rules_table.rs @@ -1,21 +1,23 @@ /// Generate a Markdown-compatible table of supported lint rules. -use ruff::checks::CheckKind; +use ruff::checks::{CheckKind, RejectedCmpop}; fn main() { let mut check_kinds: Vec = vec![ CheckKind::AssertTuple, CheckKind::DefaultExceptNotLast, + CheckKind::DoNotAssignLambda, CheckKind::DuplicateArgumentName, CheckKind::FStringMissingPlaceholders, - CheckKind::IfTuple, CheckKind::IOError("...".to_string()), + CheckKind::IfTuple, CheckKind::ImportStarUsage, CheckKind::LineTooLong, - CheckKind::DoNotAssignLambda, CheckKind::ModuleImportNotAtTopOfFile, CheckKind::NoAssertEquals, + CheckKind::NoneComparison(RejectedCmpop::Eq), CheckKind::RaiseNotImplemented, CheckKind::ReturnOutsideFunction, + CheckKind::TrueFalseComparison(true, RejectedCmpop::Eq), CheckKind::UndefinedExport("...".to_string()), CheckKind::UndefinedLocal("...".to_string()), CheckKind::UndefinedName("...".to_string()), diff --git a/resources/test/fixtures/E711.py b/resources/test/fixtures/E711.py new file mode 100644 index 0000000000..932a7fa9d5 --- /dev/null +++ b/resources/test/fixtures/E711.py @@ -0,0 +1,11 @@ +if var == None: + pass + +if None != var: + pass + +if var is None: + pass + +if None is not var: + pass diff --git a/resources/test/fixtures/E712.py b/resources/test/fixtures/E712.py new file mode 100644 index 0000000000..bf84a8163e --- /dev/null +++ b/resources/test/fixtures/E712.py @@ -0,0 +1,14 @@ +if var == True: + pass + +if False != var: + pass + +if var != False != True: + pass + +if var is True: + pass + +if False is not var: + pass diff --git a/resources/test/fixtures/pyproject.toml b/resources/test/fixtures/pyproject.toml index 43d0ae65f4..ba44ffa78e 100644 --- a/resources/test/fixtures/pyproject.toml +++ b/resources/test/fixtures/pyproject.toml @@ -4,6 +4,8 @@ exclude = ["excluded.py", "**/migrations"] select = [ "E402", "E501", + "E711", + "E712", "E731", "E902", "F401", diff --git a/src/check_ast.rs b/src/check_ast.rs index 09bb2e5b14..d99fc19a4d 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -1,8 +1,9 @@ use std::collections::BTreeSet; use std::path::Path; +use itertools::izip; use rustpython_parser::ast::{ - Arg, Arguments, Constant, Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind, + Arg, Arguments, Cmpop, Constant, Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind, Location, Stmt, StmtKind, Suite, }; use rustpython_parser::parser; @@ -11,7 +12,7 @@ use crate::ast_ops::{ extract_all_names, Binding, BindingKind, Scope, ScopeKind, SourceCodeLocator, }; use crate::builtins::{BUILTINS, MAGIC_GLOBALS}; -use crate::checks::{Check, CheckCode, CheckKind, Fix}; +use crate::checks::{Check, CheckCode, CheckKind, Fix, RejectedCmpop}; use crate::settings::Settings; use crate::visitor::{walk_excepthandler, Visitor}; use crate::{autofix, fixer, visitor}; @@ -522,6 +523,106 @@ impl Visitor for Checker<'_> { } self.in_f_string = true; } + ExprKind::Compare { + left, + ops, + comparators, + } => { + let op = ops.first().unwrap(); + let comparator = left; + + // Check `left`. + if self.settings.select.contains(&CheckCode::E711) + && matches!( + comparator.node, + ExprKind::Constant { + value: Constant::None, + kind: None + } + ) + { + if matches!(op, Cmpop::Eq) { + self.checks.push(Check::new( + CheckKind::NoneComparison(RejectedCmpop::Eq), + comparator.location, + )); + } + if matches!(op, Cmpop::NotEq) { + self.checks.push(Check::new( + CheckKind::NoneComparison(RejectedCmpop::NotEq), + comparator.location, + )); + } + } + + if self.settings.select.contains(&CheckCode::E712) { + if let ExprKind::Constant { + value: Constant::Bool(value), + kind: None, + } = comparator.node + { + if matches!(op, Cmpop::Eq) { + self.checks.push(Check::new( + CheckKind::TrueFalseComparison(value, RejectedCmpop::Eq), + comparator.location, + )); + } + if matches!(op, Cmpop::NotEq) { + self.checks.push(Check::new( + CheckKind::TrueFalseComparison(value, RejectedCmpop::NotEq), + comparator.location, + )); + } + } + } + + // Check each comparator in order. + for (op, comparator) in izip!(ops, comparators) { + if self.settings.select.contains(&CheckCode::E711) + && matches!( + comparator.node, + ExprKind::Constant { + value: Constant::None, + kind: None + } + ) + { + if matches!(op, Cmpop::Eq) { + self.checks.push(Check::new( + CheckKind::NoneComparison(RejectedCmpop::Eq), + comparator.location, + )); + } + if matches!(op, Cmpop::NotEq) { + self.checks.push(Check::new( + CheckKind::NoneComparison(RejectedCmpop::NotEq), + comparator.location, + )); + } + } + + if self.settings.select.contains(&CheckCode::E712) { + if let ExprKind::Constant { + value: Constant::Bool(value), + kind: None, + } = comparator.node + { + if matches!(op, Cmpop::Eq) { + self.checks.push(Check::new( + CheckKind::TrueFalseComparison(value, RejectedCmpop::Eq), + comparator.location, + )); + } + if matches!(op, Cmpop::NotEq) { + self.checks.push(Check::new( + CheckKind::TrueFalseComparison(value, RejectedCmpop::NotEq), + comparator.location, + )); + } + } + } + } + } ExprKind::Constant { value: Constant::Str(value), .. diff --git a/src/checks.rs b/src/checks.rs index f4588e1fa9..d98661cd87 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -10,6 +10,8 @@ use serde::{Deserialize, Serialize}; pub enum CheckCode { E402, E501, + E711, + E712, E731, E902, F401, @@ -37,6 +39,8 @@ impl FromStr for CheckCode { match s { "E402" => Ok(CheckCode::E402), "E501" => Ok(CheckCode::E501), + "E711" => Ok(CheckCode::E711), + "E712" => Ok(CheckCode::E712), "E731" => Ok(CheckCode::E731), "E902" => Ok(CheckCode::E902), "F401" => Ok(CheckCode::F401), @@ -65,6 +69,8 @@ impl CheckCode { match self { CheckCode::E402 => "E402", CheckCode::E501 => "E501", + CheckCode::E711 => "E711", + CheckCode::E712 => "E712", CheckCode::E731 => "E731", CheckCode::E902 => "E902", CheckCode::F401 => "F401", @@ -91,6 +97,8 @@ impl CheckCode { match self { CheckCode::E402 => &LintSource::AST, CheckCode::E501 => &LintSource::Lines, + CheckCode::E711 => &LintSource::AST, + CheckCode::E712 => &LintSource::AST, CheckCode::E731 => &LintSource::AST, CheckCode::E902 => &LintSource::FileSystem, CheckCode::F401 => &LintSource::AST, @@ -120,6 +128,12 @@ pub enum LintSource { FileSystem, } +#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] +pub enum RejectedCmpop { + Eq, + NotEq, +} + #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] pub enum CheckKind { AssertTuple, @@ -133,8 +147,10 @@ pub enum CheckKind { DoNotAssignLambda, ModuleImportNotAtTopOfFile, NoAssertEquals, + NoneComparison(RejectedCmpop), RaiseNotImplemented, ReturnOutsideFunction, + TrueFalseComparison(bool, RejectedCmpop), UndefinedExport(String), UndefinedLocal(String), UndefinedName(String), @@ -159,8 +175,10 @@ impl CheckKind { CheckKind::DoNotAssignLambda => "DoNotAssignLambda", CheckKind::ModuleImportNotAtTopOfFile => "ModuleImportNotAtTopOfFile", CheckKind::NoAssertEquals => "NoAssertEquals", + CheckKind::NoneComparison(_) => "NoneComparison", CheckKind::RaiseNotImplemented => "RaiseNotImplemented", CheckKind::ReturnOutsideFunction => "ReturnOutsideFunction", + CheckKind::TrueFalseComparison(_, _) => "TrueFalseComparison", CheckKind::UndefinedExport(_) => "UndefinedExport", CheckKind::UndefinedLocal(_) => "UndefinedLocal", CheckKind::UndefinedName(_) => "UndefinedName", @@ -185,8 +203,10 @@ impl CheckKind { CheckKind::DoNotAssignLambda => &CheckCode::E731, CheckKind::ModuleImportNotAtTopOfFile => &CheckCode::E402, CheckKind::NoAssertEquals => &CheckCode::R002, + CheckKind::NoneComparison(_) => &CheckCode::E711, CheckKind::RaiseNotImplemented => &CheckCode::F901, CheckKind::ReturnOutsideFunction => &CheckCode::F706, + CheckKind::TrueFalseComparison(_, _) => &CheckCode::E712, CheckKind::UndefinedExport(_) => &CheckCode::F822, CheckKind::UndefinedLocal(_) => &CheckCode::F823, CheckKind::UndefinedName(_) => &CheckCode::F821, @@ -227,12 +247,36 @@ impl CheckKind { CheckKind::NoAssertEquals => { "`assertEquals` is deprecated, use `assertEqual` instead".to_string() } + CheckKind::NoneComparison(op) => match op { + RejectedCmpop::Eq => "Comparison to `None` should be `cond is None`".to_string(), + RejectedCmpop::NotEq => { + "Comparison to `None` should be `cond is not None`".to_string() + } + }, CheckKind::RaiseNotImplemented => { "`raise NotImplemented` should be `raise NotImplementedError`".to_string() } CheckKind::ReturnOutsideFunction => { "a `return` statement outside of a function/method".to_string() } + CheckKind::TrueFalseComparison(value, op) => match *value { + true => match op { + RejectedCmpop::Eq => { + "Comparison to `True` should be `cond is True`".to_string() + } + RejectedCmpop::NotEq => { + "Comparison to `True` should be `cond is not True`".to_string() + } + }, + false => match op { + RejectedCmpop::Eq => { + "Comparison to `False` should be `cond is False`".to_string() + } + RejectedCmpop::NotEq => { + "Comparison to `False` should be `cond is not False`".to_string() + } + }, + }, CheckKind::UndefinedExport(name) => { format!("Undefined name `{name}` in `__all__`") } @@ -262,15 +306,17 @@ impl CheckKind { CheckKind::DefaultExceptNotLast => false, CheckKind::DuplicateArgumentName => false, CheckKind::FStringMissingPlaceholders => false, - CheckKind::IfTuple => false, CheckKind::IOError(_) => false, + CheckKind::IfTuple => false, CheckKind::ImportStarUsage => false, CheckKind::DoNotAssignLambda => false, CheckKind::LineTooLong => false, CheckKind::ModuleImportNotAtTopOfFile => false, CheckKind::NoAssertEquals => true, + CheckKind::NoneComparison(_) => false, CheckKind::RaiseNotImplemented => false, CheckKind::ReturnOutsideFunction => false, + CheckKind::TrueFalseComparison(_, _) => false, CheckKind::UndefinedExport(_) => false, CheckKind::UndefinedLocal(_) => false, CheckKind::UndefinedName(_) => false, diff --git a/src/lib.rs b/src/lib.rs index 4ff941508f..3f072b9f7d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,3 +1,5 @@ +extern crate core; + mod ast_ops; mod autofix; mod builtins; diff --git a/src/linter.rs b/src/linter.rs index 34c0935952..a19c1d3ede 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -84,7 +84,7 @@ mod tests { use anyhow::Result; use rustpython_parser::ast::Location; - use crate::checks::{Check, CheckCode, CheckKind, Fix}; + use crate::checks::{Check, CheckCode, CheckKind, Fix, RejectedCmpop}; use crate::linter::check_path; use crate::{autofix, settings}; @@ -136,6 +136,79 @@ mod tests { Ok(()) } + #[test] + fn e711() -> Result<()> { + let actual = check_path( + Path::new("./resources/test/fixtures/E711.py"), + &settings::Settings { + line_length: 88, + exclude: vec![], + select: BTreeSet::from([CheckCode::E711]), + }, + &autofix::Mode::Generate, + )?; + let expected = vec![ + Check { + kind: CheckKind::NoneComparison(RejectedCmpop::Eq), + location: Location::new(1, 11), + fix: None, + }, + Check { + kind: CheckKind::NoneComparison(RejectedCmpop::NotEq), + location: Location::new(4, 4), + fix: None, + }, + ]; + assert_eq!(actual.len(), expected.len()); + for i in 0..actual.len() { + assert_eq!(actual[i], expected[i]); + } + + Ok(()) + } + + #[test] + fn e712() -> Result<()> { + let actual = check_path( + Path::new("./resources/test/fixtures/E712.py"), + &settings::Settings { + line_length: 88, + exclude: vec![], + select: BTreeSet::from([CheckCode::E712]), + }, + &autofix::Mode::Generate, + )?; + let expected = vec![ + Check { + kind: CheckKind::TrueFalseComparison(true, RejectedCmpop::Eq), + location: Location::new(1, 11), + fix: None, + }, + Check { + kind: CheckKind::TrueFalseComparison(false, RejectedCmpop::NotEq), + location: Location::new(4, 4), + fix: None, + }, + Check { + kind: CheckKind::TrueFalseComparison(false, RejectedCmpop::NotEq), + location: Location::new(7, 11), + fix: None, + }, + Check { + kind: CheckKind::TrueFalseComparison(true, RejectedCmpop::NotEq), + location: Location::new(7, 20), + fix: None, + }, + ]; + + assert_eq!(actual.len(), expected.len()); + for i in 0..actual.len() { + assert_eq!(actual[i], expected[i]); + } + + Ok(()) + } + #[test] fn e731() -> Result<()> { let actual = check_path( @@ -152,6 +225,7 @@ mod tests { location: Location::new(1, 1), fix: None, }]; + assert_eq!(actual.len(), expected.len()); for i in 0..actual.len() { assert_eq!(actual[i], expected[i]); diff --git a/src/pyproject.rs b/src/pyproject.rs index ad185e5df1..11f3a126f3 100644 --- a/src/pyproject.rs +++ b/src/pyproject.rs @@ -239,6 +239,8 @@ other-attribute = 1 select: Some(BTreeSet::from([ CheckCode::E402, CheckCode::E501, + CheckCode::E711, + CheckCode::E712, CheckCode::E731, CheckCode::E902, CheckCode::F401,