diff --git a/src/autofix.rs b/src/autofix.rs index dbd37c213a..07adb3c1b1 100644 --- a/src/autofix.rs +++ b/src/autofix.rs @@ -6,6 +6,8 @@ use rustpython_parser::ast::Location; use crate::checks::Check; +// TODO(charlie): This should take Vec. +// TODO(charlie): Add tests. pub fn apply_fixes(checks: &mut Vec, contents: &str, path: &Path) -> Result<()> { if checks.iter().all(|check| check.fix.is_none()) { return Ok(()); diff --git a/src/check_ast.rs b/src/check_ast.rs index 582716aa14..99f2ec0c77 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -106,12 +106,10 @@ impl Visitor for Checker<'_> { if let Some(scope) = self.scopes.last() { match scope.kind { ScopeKind::Class | ScopeKind::Module => { - self.checks.push(Check { - kind: CheckKind::ReturnOutsideFunction, - location: stmt.location, - fix: None, - fixed: false, - }); + self.checks.push(Check::new( + CheckKind::ReturnOutsideFunction, + stmt.location, + )); } _ => {} } @@ -136,20 +134,22 @@ impl Visitor for Checker<'_> { kind: BindingKind::Builtin, .. }) => { - self.checks.push(Check { - kind: CheckKind::UselessObjectInheritance( - name.to_string(), - ), - location: expr.location, - fix: fixer::remove_object_base( - self.lines, - &stmt.location, - expr.location, - bases, - keywords, - ), - fixed: false, - }); + let mut check = Check::new( + CheckKind::UselessObjectInheritance(name.to_string()), + expr.location, + ); + // TODO(charlie): Only bother to do this if autofix is + // enabled. + if let Some(fix) = fixer::remove_object_base( + self.lines, + &stmt.location, + expr.location, + bases, + keywords, + ) { + check.amend(fix); + } + self.checks.push(check); } _ => {} } @@ -263,12 +263,8 @@ impl Visitor for Checker<'_> { .select .contains(CheckKind::ImportStarUsage.code()) { - self.checks.push(Check { - kind: CheckKind::ImportStarUsage, - location: stmt.location, - fix: None, - fixed: false, - }); + self.checks + .push(Check::new(CheckKind::ImportStarUsage, stmt.location)); } } else { let binding = Binding { @@ -287,12 +283,8 @@ impl Visitor for Checker<'_> { if self.settings.select.contains(CheckKind::IfTuple.code()) { if let ExprKind::Tuple { elts, .. } = &test.node { if !elts.is_empty() { - self.checks.push(Check { - kind: CheckKind::IfTuple, - location: stmt.location, - fix: None, - fixed: false, - }); + self.checks + .push(Check::new(CheckKind::IfTuple, stmt.location)); } } } @@ -308,23 +300,19 @@ impl Visitor for Checker<'_> { ExprKind::Call { func, .. } => { if let ExprKind::Name { id, .. } = &func.node { if id == "NotImplemented" { - self.checks.push(Check { - kind: CheckKind::RaiseNotImplemented, - location: stmt.location, - fix: None, - fixed: false, - }); + self.checks.push(Check::new( + CheckKind::RaiseNotImplemented, + stmt.location, + )); } } } ExprKind::Name { id, .. } => { if id == "NotImplemented" { - self.checks.push(Check { - kind: CheckKind::RaiseNotImplemented, - location: stmt.location, - fix: None, - fixed: false, - }); + self.checks.push(Check::new( + CheckKind::RaiseNotImplemented, + stmt.location, + )); } } _ => {} @@ -341,12 +329,8 @@ impl Visitor for Checker<'_> { if self.settings.select.contains(CheckKind::AssertTuple.code()) { if let ExprKind::Tuple { elts, .. } = &test.node { if !elts.is_empty() { - self.checks.push(Check { - kind: CheckKind::AssertTuple, - location: stmt.location, - fix: None, - fixed: false, - }); + self.checks + .push(Check::new(CheckKind::AssertTuple, stmt.location)); } } } @@ -407,12 +391,10 @@ impl Visitor for Checker<'_> { && name != "__traceback_supplement__" && matches!(binding.kind, BindingKind::Assignment) { - self.checks.push(Check { - kind: CheckKind::UnusedVariable(name.to_string()), - location: binding.location, - fix: None, - fixed: false, - }); + self.checks.push(Check::new( + CheckKind::UnusedVariable(name.to_string()), + binding.location, + )); } } @@ -463,12 +445,8 @@ impl Visitor for Checker<'_> { && matches!(scope.kind, ScopeKind::Class) || matches!(scope.kind, ScopeKind::Module) { - self.checks.push(Check { - kind: CheckKind::YieldOutsideFunction, - location: expr.location, - fix: None, - fixed: false, - }); + self.checks + .push(Check::new(CheckKind::YieldOutsideFunction, expr.location)); } } ExprKind::JoinedStr { values } => { @@ -481,12 +459,10 @@ impl Visitor for Checker<'_> { .iter() .any(|value| matches!(value.node, ExprKind::FormattedValue { .. })) { - self.checks.push(Check { - kind: CheckKind::FStringMissingPlaceholders, - location: expr.location, - fix: None, - fixed: false, - }); + self.checks.push(Check::new( + CheckKind::FStringMissingPlaceholders, + expr.location, + )); } self.in_f_string = true; } @@ -553,12 +529,10 @@ impl Visitor for Checker<'_> { if let Some(binding) = scope.values.remove(name) { if self.settings.select.contains(&CheckCode::F841) && binding.used.is_none() { - self.checks.push(Check { - kind: CheckKind::UnusedVariable(name.to_string()), - location: excepthandler.location, - fix: None, - fixed: false, - }); + self.checks.push(Check::new( + CheckKind::UnusedVariable(name.to_string()), + excepthandler.location, + )); } } @@ -596,12 +570,8 @@ impl Visitor for Checker<'_> { for arg in all_arguments { let ident = &arg.node.arg; if idents.contains(ident.as_str()) { - self.checks.push(Check { - kind: CheckKind::DuplicateArgumentName, - location: arg.location, - fix: None, - fixed: false, - }); + self.checks + .push(Check::new(CheckKind::DuplicateArgumentName, arg.location)); break; } idents.insert(ident); @@ -695,12 +665,10 @@ impl Checker<'_> { } if self.settings.select.contains(&CheckCode::F821) { - self.checks.push(Check { - kind: CheckKind::UndefinedName(id.clone()), - location: expr.location, - fix: None, - fixed: false, - }) + self.checks.push(Check::new( + CheckKind::UndefinedName(id.clone()), + expr.location, + )) } } } @@ -724,12 +692,10 @@ impl Checker<'_> { .unwrap_or_default(); if let Some(scope_id) = used { if scope_id == current.id { - self.checks.push(Check { - kind: CheckKind::UndefinedLocal(id.clone()), - location: expr.location, - fix: None, - fixed: false, - }); + self.checks.push(Check::new( + CheckKind::UndefinedLocal(id.clone()), + expr.location, + )); } } } @@ -776,12 +742,10 @@ impl Checker<'_> { if current.values.remove(id).is_none() && self.settings.select.contains(&CheckCode::F821) { - self.checks.push(Check { - kind: CheckKind::UndefinedName(id.clone()), - location: expr.location, - fix: None, - fixed: false, - }) + self.checks.push(Check::new( + CheckKind::UndefinedName(id.clone()), + expr.location, + )) } } } @@ -815,12 +779,10 @@ impl Checker<'_> { if let Some(names) = all_names { for name in names { if !scope.values.contains_key(name) { - self.checks.push(Check { - kind: CheckKind::UndefinedExport(name.to_string()), - location: binding.location, - fix: None, - fixed: false, - }); + self.checks.push(Check::new( + CheckKind::UndefinedExport(name.to_string()), + binding.location, + )); } } } @@ -838,12 +800,10 @@ impl Checker<'_> { match &binding.kind { BindingKind::Importation(full_name) | BindingKind::SubmoduleImportation(full_name) => { - self.checks.push(Check { - kind: CheckKind::UnusedImport(full_name.to_string()), - location: binding.location, - fix: None, - fixed: false, - }); + self.checks.push(Check::new( + CheckKind::UnusedImport(full_name.to_string()), + binding.location, + )); } _ => {} } diff --git a/src/check_lines.rs b/src/check_lines.rs index 1fca49f2c1..9fd3dbbb54 100644 --- a/src/check_lines.rs +++ b/src/check_lines.rs @@ -35,12 +35,10 @@ pub fn check_lines(checks: &mut Vec, contents: &str, settings: &Settings) // Enforce line length. if enforce_line_too_long && should_enforce_line_length(line, settings.line_length) { - let check = Check { - kind: CheckKind::LineTooLong, - location: Location::new(row + 1, settings.line_length + 1), - fix: None, - fixed: false, - }; + let check = Check::new( + CheckKind::LineTooLong, + Location::new(row + 1, settings.line_length + 1), + ); if !check.is_inline_ignored(line) { line_checks.push(check); } diff --git a/src/checks.rs b/src/checks.rs index 535b65b4cc..944159238f 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -246,6 +246,19 @@ static NO_QA_REGEX: Lazy = Lazy::new(|| { static SPLIT_COMMA_REGEX: Lazy = Lazy::new(|| Regex::new(r"[,\s]").expect("Invalid regex")); impl Check { + pub fn new(kind: CheckKind, location: Location) -> Self { + Self { + kind, + location, + fix: None, + fixed: false, + } + } + + pub fn amend(&mut self, fix: Fix) { + self.fix = Some(fix); + } + pub fn is_inline_ignored(&self, line: &str) -> bool { match NO_QA_REGEX.captures(line) { Some(caps) => match caps.name("codes") { diff --git a/src/main.rs b/src/main.rs index 680762e173..3a713ed2c9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -87,6 +87,8 @@ fn run_once( fn report_once(messages: &[Message]) -> Result<()> { let (fixed, outstanding): (Vec<&Message>, Vec<&Message>) = messages.iter().partition(|message| message.fixed); + + // TODO(charlie): If autofix is disabled, but some rules are fixable, tell the user. if fixed.is_empty() { println!("Found {} error(s).", messages.len()); } else {