From 40c1e7e0057c4d949fd9f1a2a07843ed8da070f3 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 12 Sep 2022 11:16:08 -0400 Subject: [PATCH] Implement F701 and F702 (#169) --- README.md | 5 +- examples/generate_rules_table.rs | 4 +- resources/test/fixtures/F701.py | 23 ++++++ resources/test/fixtures/F702.py | 23 ++++++ resources/test/fixtures/pyproject.toml | 2 + src/ast/checks.rs | 76 +++++++++++++++++- src/check_ast.rs | 18 +++++ src/checks.rs | 106 +++++++++++-------------- src/linter.rs | 84 ++++++++++++++++++++ src/pyproject.rs | 2 + src/settings.rs | 2 + 11 files changed, 280 insertions(+), 65 deletions(-) create mode 100644 resources/test/fixtures/F701.py create mode 100644 resources/test/fixtures/F702.py diff --git a/README.md b/README.md index 165edcd6e1395..8164af468e27c 100644 --- a/README.md +++ b/README.md @@ -124,13 +124,12 @@ ruff's goal is to achieve feature-parity with Flake8 when used (1) without any p stylistic checks; limiting to Python 3 obviates the need for certain compatibility checks.) Under those conditions, Flake8 implements about 58 rules, give or take. At time of writing, ruff -implements 34 rules. (Note that these 34 rules likely cover a disproportionate share of errors: +implements 36 rules. (Note that these 36 rules likely cover a disproportionate share of errors: unused imports, undefined variables, etc.) Of the unimplemented rules, ruff is missing: - 15 rules related to string `.format` calls. -- 6 rules related to misplaced `yield`, `break`, and `return` statements. - 3 rules related to syntax errors in doctests and annotations. - 2 rules related to redefining unused names. @@ -169,6 +168,8 @@ Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis F | F622 | TwoStarredExpressions | two starred expressions in assignment | | F631 | AssertTuple | Assert test is a non-empty tuple, which is always `True` | | F634 | IfTuple | If test is a tuple, which is always `True` | +| F701 | BreakOutsideLoop | `break` outside loop | +| F702 | ContinueOutsideLoop | `continue` not properly in loop | | F704 | YieldOutsideFunction | a `yield` or `yield from` statement outside of a function/method | | F706 | ReturnOutsideFunction | a `return` statement outside of a function/method | | F707 | DefaultExceptNotLast | an `except:` block as not the last exception handler | diff --git a/examples/generate_rules_table.rs b/examples/generate_rules_table.rs index 1ff212d966200..4196b2f459c76 100644 --- a/examples/generate_rules_table.rs +++ b/examples/generate_rules_table.rs @@ -3,10 +3,12 @@ use ruff::checks::{CheckKind, RejectedCmpop}; fn main() { let mut check_kinds: Vec = vec![ - CheckKind::AmbiguousVariableName("...".to_string()), CheckKind::AmbiguousClassName("...".to_string()), CheckKind::AmbiguousFunctionName("...".to_string()), + CheckKind::AmbiguousVariableName("...".to_string()), CheckKind::AssertTuple, + CheckKind::BreakOutsideLoop, + CheckKind::ContinueOutsideLoop, CheckKind::DefaultExceptNotLast, CheckKind::DoNotAssignLambda, CheckKind::DoNotUseBareExcept, diff --git a/resources/test/fixtures/F701.py b/resources/test/fixtures/F701.py new file mode 100644 index 0000000000000..68f591954108e --- /dev/null +++ b/resources/test/fixtures/F701.py @@ -0,0 +1,23 @@ +for i in range(10): + break +else: + break + +i = 0 +while i < 10: + i += 1 + break + + +def f(): + for i in range(10): + break + + break + + +class Foo: + break + + +break diff --git a/resources/test/fixtures/F702.py b/resources/test/fixtures/F702.py new file mode 100644 index 0000000000000..97d7aba7218c6 --- /dev/null +++ b/resources/test/fixtures/F702.py @@ -0,0 +1,23 @@ +for i in range(10): + continue +else: + continue + +i = 0 +while i < 10: + i += 1 + continue + + +def f(): + for i in range(10): + continue + + continue + + +class Foo: + continue + + +continue diff --git a/resources/test/fixtures/pyproject.toml b/resources/test/fixtures/pyproject.toml index 605d807632282..08bf367deea4b 100644 --- a/resources/test/fixtures/pyproject.toml +++ b/resources/test/fixtures/pyproject.toml @@ -25,6 +25,8 @@ select = [ "F622", "F631", "F634", + "F701", + "F702", "F704", "F706", "F707", diff --git a/src/ast/checks.rs b/src/ast/checks.rs index 0f2122b74143a..e393abf932ec2 100644 --- a/src/ast/checks.rs +++ b/src/ast/checks.rs @@ -3,7 +3,7 @@ use std::collections::BTreeSet; use itertools::izip; use rustpython_parser::ast::{ Arg, Arguments, Cmpop, Constant, Excepthandler, ExcepthandlerKind, Expr, ExprKind, Keyword, - Location, Stmt, Unaryop, + Location, Stmt, StmtKind, Unaryop, }; use crate::ast::operations::SourceCodeLocator; @@ -468,3 +468,77 @@ pub fn check_starred_expressions( None } + +/// Check BreakOutsideLoop compliance. +pub fn check_break_outside_loop( + stmt: &Stmt, + parents: &[&Stmt], + parent_stack: &[usize], +) -> Option { + let mut allowed: bool = false; + let mut parent = stmt; + for index in parent_stack.iter().rev() { + let child = parent; + parent = parents[*index]; + match &parent.node { + StmtKind::For { orelse, .. } + | StmtKind::AsyncFor { orelse, .. } + | StmtKind::While { orelse, .. } => { + if !orelse.contains(child) { + allowed = true; + break; + } + } + + StmtKind::FunctionDef { .. } + | StmtKind::AsyncFunctionDef { .. } + | StmtKind::ClassDef { .. } => { + break; + } + _ => {} + } + } + + if !allowed { + Some(Check::new(CheckKind::BreakOutsideLoop, stmt.location)) + } else { + None + } +} + +/// Check ContinueOutsideLoop compliance. +pub fn check_continue_outside_loop( + stmt: &Stmt, + parents: &[&Stmt], + parent_stack: &[usize], +) -> Option { + let mut allowed: bool = false; + let mut parent = stmt; + for index in parent_stack.iter().rev() { + let child = parent; + parent = parents[*index]; + match &parent.node { + StmtKind::For { orelse, .. } + | StmtKind::AsyncFor { orelse, .. } + | StmtKind::While { orelse, .. } => { + if !orelse.contains(child) { + allowed = true; + break; + } + } + + StmtKind::FunctionDef { .. } + | StmtKind::AsyncFunctionDef { .. } + | StmtKind::ClassDef { .. } => { + break; + } + _ => {} + } + } + + if !allowed { + Some(Check::new(CheckKind::ContinueOutsideLoop, stmt.location)) + } else { + None + } +} diff --git a/src/check_ast.rs b/src/check_ast.rs index 6f038465bdc4b..cfad44874697d 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -194,6 +194,24 @@ where })); } } + StmtKind::Break => { + if self.settings.select.contains(&CheckCode::F701) { + if let Some(check) = + checks::check_break_outside_loop(stmt, &self.parents, &self.parent_stack) + { + self.checks.push(check); + } + } + } + StmtKind::Continue => { + if self.settings.select.contains(&CheckCode::F702) { + if let Some(check) = + checks::check_continue_outside_loop(stmt, &self.parents, &self.parent_stack) + { + self.checks.push(check); + } + } + } StmtKind::FunctionDef { name, decorator_list, diff --git a/src/checks.rs b/src/checks.rs index a792f93d8c65f..05e1132f9423b 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -31,6 +31,8 @@ pub enum CheckCode { F622, F631, F634, + F701, + F702, F704, F706, F707, @@ -72,6 +74,8 @@ impl FromStr for CheckCode { "F622" => Ok(CheckCode::F622), "F631" => Ok(CheckCode::F631), "F634" => Ok(CheckCode::F634), + "F701" => Ok(CheckCode::F701), + "F702" => Ok(CheckCode::F702), "F704" => Ok(CheckCode::F704), "F706" => Ok(CheckCode::F706), "F707" => Ok(CheckCode::F707), @@ -114,6 +118,8 @@ impl CheckCode { CheckCode::F622 => "F622", CheckCode::F631 => "F631", CheckCode::F634 => "F634", + CheckCode::F701 => "F701", + CheckCode::F702 => "F702", CheckCode::F704 => "F704", CheckCode::F706 => "F706", CheckCode::F707 => "F707", @@ -154,6 +160,8 @@ impl CheckCode { CheckCode::F622 => &LintSource::AST, CheckCode::F631 => &LintSource::AST, CheckCode::F634 => &LintSource::AST, + CheckCode::F701 => &LintSource::AST, + CheckCode::F702 => &LintSource::AST, CheckCode::F704 => &LintSource::AST, CheckCode::F706 => &LintSource::AST, CheckCode::F707 => &LintSource::AST, @@ -184,10 +192,12 @@ pub enum RejectedCmpop { #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] pub enum CheckKind { - AssertTuple, - AmbiguousVariableName(String), AmbiguousClassName(String), AmbiguousFunctionName(String), + AmbiguousVariableName(String), + AssertTuple, + BreakOutsideLoop, + ContinueOutsideLoop, DefaultExceptNotLast, DoNotAssignLambda, DoNotUseBareExcept, @@ -224,11 +234,14 @@ impl CheckKind { /// The name of the check. pub fn name(&self) -> &'static str { match self { - CheckKind::AssertTuple => "AssertTuple", - CheckKind::AmbiguousVariableName(_) => "AmbiguousVariableName", CheckKind::AmbiguousClassName(_) => "AmbiguousClassName", CheckKind::AmbiguousFunctionName(_) => "AmbiguousFunctionName", + CheckKind::AmbiguousVariableName(_) => "AmbiguousVariableName", + CheckKind::AssertTuple => "AssertTuple", + CheckKind::BreakOutsideLoop => "BreakOutsideLoop", + CheckKind::ContinueOutsideLoop => "ContinueOutsideLoop", CheckKind::DefaultExceptNotLast => "DefaultExceptNotLast", + CheckKind::DoNotAssignLambda => "DoNotAssignLambda", CheckKind::DoNotUseBareExcept => "DoNotUseBareExcept", CheckKind::DuplicateArgumentName => "DuplicateArgumentName", CheckKind::FStringMissingPlaceholders => "FStringMissingPlaceholders", @@ -238,7 +251,6 @@ impl CheckKind { CheckKind::ImportStarUsage => "ImportStarUsage", CheckKind::LateFutureImport => "LateFutureImport", CheckKind::LineTooLong(_, _) => "LineTooLong", - CheckKind::DoNotAssignLambda => "DoNotAssignLambda", CheckKind::ModuleImportNotAtTopOfFile => "ModuleImportNotAtTopOfFile", CheckKind::MultiValueRepeatedKeyLiteral => "MultiValueRepeatedKeyLiteral", CheckKind::MultiValueRepeatedKeyVariable(_) => "MultiValueRepeatedKeyVariable", @@ -266,8 +278,14 @@ impl CheckKind { /// A four-letter shorthand code for the check. pub fn code(&self) -> &'static CheckCode { match self { + CheckKind::AmbiguousClassName(_) => &CheckCode::E742, + CheckKind::AmbiguousFunctionName(_) => &CheckCode::E743, + CheckKind::AmbiguousVariableName(_) => &CheckCode::E741, CheckKind::AssertTuple => &CheckCode::F631, + CheckKind::BreakOutsideLoop => &CheckCode::F701, + CheckKind::ContinueOutsideLoop => &CheckCode::F702, CheckKind::DefaultExceptNotLast => &CheckCode::F707, + CheckKind::DoNotAssignLambda => &CheckCode::E731, CheckKind::DoNotUseBareExcept => &CheckCode::E722, CheckKind::DuplicateArgumentName => &CheckCode::F831, CheckKind::FStringMissingPlaceholders => &CheckCode::F541, @@ -277,10 +295,6 @@ impl CheckKind { CheckKind::ImportStarUsage => &CheckCode::F403, CheckKind::LateFutureImport => &CheckCode::F404, CheckKind::LineTooLong(_, _) => &CheckCode::E501, - CheckKind::DoNotAssignLambda => &CheckCode::E731, - CheckKind::AmbiguousVariableName(_) => &CheckCode::E741, - CheckKind::AmbiguousClassName(_) => &CheckCode::E742, - CheckKind::AmbiguousFunctionName(_) => &CheckCode::E743, CheckKind::ModuleImportNotAtTopOfFile => &CheckCode::E402, CheckKind::MultiValueRepeatedKeyLiteral => &CheckCode::F601, CheckKind::MultiValueRepeatedKeyVariable(_) => &CheckCode::F602, @@ -306,13 +320,27 @@ impl CheckKind { /// The body text for the check. pub fn body(&self) -> String { match self { + CheckKind::AmbiguousClassName(name) => { + format!("ambiguous class name '{}'", name) + } + CheckKind::AmbiguousFunctionName(name) => { + format!("ambiguous function name '{}'", name) + } + CheckKind::AmbiguousVariableName(name) => { + format!("ambiguous variable name '{}'", name) + } CheckKind::AssertTuple => { "Assert test is a non-empty tuple, which is always `True`".to_string() } - CheckKind::DoNotUseBareExcept => "Do not use bare `except`".to_string(), + CheckKind::BreakOutsideLoop => "`break` outside loop".to_string(), + CheckKind::ContinueOutsideLoop => "`continue` not properly in loop".to_string(), CheckKind::DefaultExceptNotLast => { "an `except:` block as not the last exception handler".to_string() } + CheckKind::DoNotAssignLambda => { + "Do not assign a lambda expression, use a def".to_string() + } + CheckKind::DoNotUseBareExcept => "Do not use bare `except`".to_string(), CheckKind::DuplicateArgumentName => { "Duplicate argument name in function definition".to_string() } @@ -333,18 +361,6 @@ impl CheckKind { CheckKind::LineTooLong(length, limit) => { format!("Line too long ({length} > {limit} characters)") } - CheckKind::DoNotAssignLambda => { - "Do not assign a lambda expression, use a def".to_string() - } - CheckKind::AmbiguousClassName(name) => { - format!("ambiguous class name '{}'", name) - } - CheckKind::AmbiguousFunctionName(name) => { - format!("ambiguous function name '{}'", name) - } - CheckKind::AmbiguousVariableName(name) => { - format!("ambiguous variable name '{}'", name) - } CheckKind::ModuleImportNotAtTopOfFile => { "Module level import not at top of file".to_string() } @@ -396,12 +412,12 @@ impl CheckKind { CheckKind::UndefinedExport(name) => { format!("Undefined name `{name}` in `__all__`") } - CheckKind::UndefinedName(name) => { - format!("Undefined name `{name}`") - } CheckKind::UndefinedLocal(name) => { format!("Local variable `{name}` referenced before assignment") } + CheckKind::UndefinedName(name) => { + format!("Undefined name `{name}`") + } CheckKind::UnusedImport(name) => format!("`{name}` imported but unused"), CheckKind::UnusedVariable(name) => { format!("Local variable `{name}` is assigned to but never used") @@ -417,42 +433,10 @@ impl CheckKind { /// Whether the check kind is (potentially) fixable. pub fn fixable(&self) -> bool { - match self { - CheckKind::AmbiguousClassName(_) => true, - CheckKind::AmbiguousFunctionName(_) => true, - CheckKind::AmbiguousVariableName(_) => false, - CheckKind::AssertTuple => false, - CheckKind::DefaultExceptNotLast => false, - CheckKind::DoNotAssignLambda => false, - CheckKind::DoNotUseBareExcept => false, - CheckKind::DuplicateArgumentName => false, - CheckKind::FStringMissingPlaceholders => false, - CheckKind::FutureFeatureNotDefined(_) => false, - CheckKind::IOError(_) => false, - CheckKind::IfTuple => false, - CheckKind::ImportStarUsage => false, - CheckKind::LateFutureImport => false, - CheckKind::LineTooLong(_, _) => false, - CheckKind::ModuleImportNotAtTopOfFile => false, - CheckKind::MultiValueRepeatedKeyLiteral => false, - CheckKind::MultiValueRepeatedKeyVariable(_) => false, - CheckKind::NoAssertEquals => true, - CheckKind::NoneComparison(_) => false, - CheckKind::NotInTest => false, - CheckKind::NotIsTest => false, - CheckKind::RaiseNotImplemented => false, - CheckKind::ReturnOutsideFunction => false, - CheckKind::TooManyExpressionsInStarredAssignment => false, - CheckKind::TrueFalseComparison(_, _) => false, - CheckKind::TwoStarredExpressions => false, - CheckKind::UndefinedExport(_) => false, - CheckKind::UndefinedLocal(_) => false, - CheckKind::UndefinedName(_) => false, - CheckKind::UnusedImport(_) => false, - CheckKind::UnusedVariable(_) => false, - CheckKind::UselessObjectInheritance(_) => true, - CheckKind::YieldOutsideFunction => false, - } + matches!( + self, + CheckKind::NoAssertEquals | CheckKind::UselessObjectInheritance(_) + ) } } diff --git a/src/linter.rs b/src/linter.rs index d6907567bb6c5..8928282709ff8 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -851,6 +851,90 @@ mod tests { Ok(()) } + #[test] + fn f701() -> Result<()> { + let mut actual = check_path( + Path::new("./resources/test/fixtures/F701.py"), + &settings::Settings { + line_length: 88, + exclude: vec![], + select: BTreeSet::from([CheckCode::F701]), + }, + &fixer::Mode::Generate, + )?; + actual.sort_by_key(|check| check.location); + let expected = vec![ + Check { + kind: CheckKind::BreakOutsideLoop, + location: Location::new(4, 5), + fix: None, + }, + Check { + kind: CheckKind::BreakOutsideLoop, + location: Location::new(16, 5), + fix: None, + }, + Check { + kind: CheckKind::BreakOutsideLoop, + location: Location::new(20, 5), + fix: None, + }, + Check { + kind: CheckKind::BreakOutsideLoop, + location: Location::new(23, 1), + fix: None, + }, + ]; + assert_eq!(actual.len(), expected.len()); + for i in 0..actual.len() { + assert_eq!(actual[i], expected[i]); + } + + Ok(()) + } + + #[test] + fn f702() -> Result<()> { + let mut actual = check_path( + Path::new("./resources/test/fixtures/F702.py"), + &settings::Settings { + line_length: 88, + exclude: vec![], + select: BTreeSet::from([CheckCode::F702]), + }, + &fixer::Mode::Generate, + )?; + actual.sort_by_key(|check| check.location); + let expected = vec![ + Check { + kind: CheckKind::ContinueOutsideLoop, + location: Location::new(4, 5), + fix: None, + }, + Check { + kind: CheckKind::ContinueOutsideLoop, + location: Location::new(16, 5), + fix: None, + }, + Check { + kind: CheckKind::ContinueOutsideLoop, + location: Location::new(20, 5), + fix: None, + }, + Check { + kind: CheckKind::ContinueOutsideLoop, + location: Location::new(23, 1), + fix: None, + }, + ]; + assert_eq!(actual.len(), expected.len()); + for i in 0..actual.len() { + assert_eq!(actual[i], expected[i]); + } + + Ok(()) + } + #[test] fn f704() -> Result<()> { let mut actual = check_path( diff --git a/src/pyproject.rs b/src/pyproject.rs index c91313fc5f069..976eee42c9dcd 100644 --- a/src/pyproject.rs +++ b/src/pyproject.rs @@ -282,6 +282,8 @@ other-attribute = 1 CheckCode::F622, CheckCode::F631, CheckCode::F634, + CheckCode::F701, + CheckCode::F702, CheckCode::F704, CheckCode::F706, CheckCode::F707, diff --git a/src/settings.rs b/src/settings.rs index 405cb7eb4b613..49abe6763c5db 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -66,6 +66,8 @@ impl Settings { CheckCode::F622, CheckCode::F631, CheckCode::F634, + CheckCode::F701, + CheckCode::F702, CheckCode::F704, CheckCode::F706, CheckCode::F707,