From 3c41b33023a42a46a164f15142620ad668927300 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 7 Sep 2022 20:43:55 -0400 Subject: [PATCH] Try to fix --- resources/test/fixtures/F821.py | 90 ++++---- src/check_ast.rs | 350 ++++++++++++++++++++------------ src/linter.rs | 92 +++++---- src/visitor.rs | 199 ++++++++++-------- 4 files changed, 433 insertions(+), 298 deletions(-) diff --git a/resources/test/fixtures/F821.py b/resources/test/fixtures/F821.py index 0b47c98897..a793ce9a32 100644 --- a/resources/test/fixtures/F821.py +++ b/resources/test/fixtures/F821.py @@ -1,50 +1,50 @@ -# def get_name(): -# return self.name -# -# -# def get_name(): -# return (self.name,) -# -# -# def get_name(): -# del self.name -# -# -# def get_name(self): -# return self.name -# -# -# x = list() -# -# -# def randdec(maxprec, maxexp): -# return numeric_string(maxprec, maxexp) -# -# -# def ternary_optarg(prec, exp_range, itr): -# for _ in range(100): -# a = randdec(prec, 2 * exp_range) -# b = randdec(prec, 2 * exp_range) -# c = randdec(prec, 2 * exp_range) -# yield a, b, c, None -# yield a, b, c, None, None -# -# -# class Foo: -# CLASS_VAR = 1 -# REFERENCES_CLASS_VAR = {"CLASS_VAR": CLASS_VAR} -# ANNOTATED_CLASS_VAR: int = 2 -# +def get_name(): + return self.name + + +def get_name(): + return (self.name,) + + +def get_name(): + del self.name + + +def get_name(self): + return self.name + + +x = list() + + +def randdec(maxprec, maxexp): + return numeric_string(maxprec, maxexp) + + +def ternary_optarg(prec, exp_range, itr): + for _ in range(100): + a = randdec(prec, 2 * exp_range) + b = randdec(prec, 2 * exp_range) + c = randdec(prec, 2 * exp_range) + yield a, b, c, None + yield a, b, c, None, None + + +class Foo: + CLASS_VAR = 1 + REFERENCES_CLASS_VAR = {"CLASS_VAR": CLASS_VAR} + ANNOTATED_CLASS_VAR: int = 2 + class Class: def __init__(self): Class -# -# try: -# x = 1 / 0 -# except Exception as e: -# print(e) -# -# -# y: int = 1 + +try: + x = 1 / 0 +except Exception as e: + print(e) + + +y: int = 1 diff --git a/src/check_ast.rs b/src/check_ast.rs index 5a1e446c6a..868966d739 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -17,20 +17,26 @@ use crate::settings::Settings; use crate::visitor::{walk_excepthandler, Visitor}; use crate::{autofix, fixer, visitor}; +pub const GLOBAL_SCOPE_INDEX: usize = 0; + struct Checker<'a> { + // Input data. locator: SourceCodeLocator<'a>, settings: &'a Settings, autofix: &'a autofix::Mode, path: &'a str, + // Computed checks. checks: Vec, + // Scope tracking: retain all scopes, along with a stack of indexes to track which scopes are + // active. scopes: Vec, - dead_scopes: Vec, + scope_stack: Vec, + parents: Vec<&'a Stmt>, + dead_scopes: Vec, deferred_annotations: Vec<&'a str>, - // I think I need to track the list of scope indexes... - // And then store all scopes. - // And track the current scope stack by index? - deferred_functions: Vec<(&'a Stmt, &'a [Scope])>, - stack: Vec<&'a Stmt>, + deferred_functions: Vec<(&'a Stmt, Vec)>, + deferred_lambdas: Vec<(&'a Expr, Vec)>, + // Derivative state. in_f_string: bool, in_annotation: bool, seen_non_import: bool, @@ -50,11 +56,13 @@ impl<'a> Checker<'a> { path, locator: SourceCodeLocator::new(content), checks: vec![], + scope_stack: vec![], scopes: vec![], dead_scopes: vec![], deferred_annotations: vec![], deferred_functions: vec![], - stack: vec![], + deferred_lambdas: vec![], + parents: vec![], in_f_string: false, in_annotation: false, seen_non_import: false, @@ -82,17 +90,20 @@ where 'b: 'a, { fn visit_stmt(&mut self, stmt: &'b Stmt) { - self.stack.push(stmt); + self.parents.push(stmt); + // Pre-visit. match &stmt.node { StmtKind::Global { names } | StmtKind::Nonlocal { names } => { // TODO(charlie): Handle doctests. - let global_scope_index = 0; - let global_scope_id = self.scopes[global_scope_index].id; - let current_scope_id = self.scopes.last().expect("No current scope found.").id; + let global_scope_id = self.scopes[GLOBAL_SCOPE_INDEX].id; + + let current_scope = + &self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; + let current_scope_id = current_scope.id; if current_scope_id != global_scope_id { for name in names { - for scope in self.scopes.iter_mut().skip(global_scope_index + 1) { + for scope in self.scopes.iter_mut().skip(GLOBAL_SCOPE_INDEX + 1) { scope.values.insert( name.to_string(), Binding { @@ -118,7 +129,7 @@ where .. } => { for expr in decorator_list { - self.visit_expr(expr, Some(stmt)); + self.visit_expr(expr); } for expr in returns { self.visit_annotation(expr); @@ -131,10 +142,6 @@ where location: stmt.location, }, ); - - self.deferred_functions.push((stmt, &self.scopes)); - - // self.push_scope(Scope::new(ScopeKind::Function)); } StmtKind::Return { .. } => { if self @@ -142,8 +149,8 @@ where .select .contains(CheckKind::ReturnOutsideFunction.code()) { - if let Some(scope) = self.scopes.last() { - match scope.kind { + if let Some(scope_index) = self.scope_stack.last().cloned() { + match self.scopes[scope_index].kind { ScopeKind::Class | ScopeKind::Module => { self.checks.push(Check::new( CheckKind::ReturnOutsideFunction, @@ -166,7 +173,8 @@ where for expr in bases { if let ExprKind::Name { id, .. } = &expr.node { if id == "object" { - let scope = self.scopes.last().expect("No current scope found."); + let scope = &self.scopes + [*(self.scope_stack.last().expect("No current scope found."))]; match scope.values.get(id) { None | Some(Binding { @@ -201,13 +209,13 @@ where } for expr in bases { - self.visit_expr(expr, Some(stmt)) + self.visit_expr(expr) } for keyword in keywords { self.visit_keyword(keyword) } for expr in decorator_list { - self.visit_expr(expr, Some(stmt)) + self.visit_expr(expr) } self.push_scope(Scope::new(ScopeKind::Class)) } @@ -286,7 +294,13 @@ where name, Binding { kind: BindingKind::FutureImportation, - used: Some(self.scopes.last().expect("No current scope found.").id), + used: Some( + self.scopes[*(self + .scope_stack + .last() + .expect("No current scope found."))] + .id, + ), location: stmt.location, }, ); @@ -441,41 +455,23 @@ where _ => {} } - visitor::walk_stmt(self, stmt); - + // Recurse. match &stmt.node { - StmtKind::ClassDef { .. } => { - if let Some(scope) = self.scopes.pop() { - self.dead_scopes.push(scope); + StmtKind::FunctionDef { .. } | StmtKind::AsyncFunctionDef { .. } => { + self.deferred_functions + .push((stmt, self.scope_stack.clone())); + } + StmtKind::ClassDef { body, .. } => { + for stmt in body { + self.visit_stmt(stmt); } } - // StmtKind::FunctionDef { .. } | StmtKind::AsyncFunctionDef { .. } => { - // let scope = self.scopes.last().expect("No current scope found."); - // for (name, binding) in scope.values.iter() { - // // TODO(charlie): Ignore if using `locals`. - // if self.settings.select.contains(&CheckCode::F841) - // && binding.used.is_none() - // && name != "_" - // && name != "__tracebackhide__" - // && name != "__traceback_info__" - // && name != "__traceback_supplement__" - // && matches!(binding.kind, BindingKind::Assignment) - // { - // self.checks.push(Check::new( - // CheckKind::UnusedVariable(name.to_string()), - // binding.location, - // )); - // } - // } - // - // if let Some(scope) = self.scopes.pop() { - // self.dead_scopes.push(scope); - // } - // } - _ => {} + _ => visitor::walk_stmt(self, stmt), }; + // Post-visit. if let StmtKind::ClassDef { name, .. } = &stmt.node { + self.pop_scope(); self.add_binding( name.to_string(), Binding { @@ -484,22 +480,30 @@ where location: stmt.location, }, ); - } + }; + + self.parents.pop(); } fn visit_annotation(&mut self, expr: &'b Expr) { let initial = self.in_annotation; self.in_annotation = true; - self.visit_expr(expr, None); + self.visit_expr(expr); self.in_annotation = initial; } - fn visit_expr(&mut self, expr: &'b Expr, parent: Option<&Stmt>) { + fn visit_expr(&mut self, expr: &'b Expr) { let initial = self.in_f_string; + + // Pre-visit. match &expr.node { ExprKind::Name { ctx, .. } => match ctx { ExprContext::Load => self.handle_node_load(expr), - ExprContext::Store => self.handle_node_store(expr, parent), + ExprContext::Store => { + let parent = self.parents.pop().expect("No parnet statement found."); + self.handle_node_store(expr, Some(parent)); + self.parents.push(parent); + } ExprContext::Del => self.handle_node_delete(expr), }, ExprKind::Call { func, .. } => { @@ -579,9 +583,9 @@ where | ExprKind::ListComp { .. } | ExprKind::DictComp { .. } | ExprKind::SetComp { .. } => self.push_scope(Scope::new(ScopeKind::Generator)), - ExprKind::Lambda { .. } => self.push_scope(Scope::new(ScopeKind::Function)), ExprKind::Yield { .. } | ExprKind::YieldFrom { .. } => { - let scope = self.scopes.last().expect("No current scope found."); + let scope = + &self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; if self .settings .select @@ -738,17 +742,21 @@ where _ => {} }; - visitor::walk_expr(self, expr); + // Recurse. + match &expr.node { + ExprKind::Lambda { .. } => { + self.deferred_lambdas.push((expr, self.scope_stack.clone())); + } + _ => visitor::walk_expr(self, expr), + } + // Post-visit. match &expr.node { ExprKind::GeneratorExp { .. } | ExprKind::ListComp { .. } | ExprKind::DictComp { .. } - | ExprKind::SetComp { .. } - | ExprKind::Lambda { .. } => { - if let Some(scope) = self.scopes.pop() { - self.dead_scopes.push(scope); - } + | ExprKind::SetComp { .. } => { + self.pop_scope(); } ExprKind::JoinedStr { .. } => { self.in_f_string = initial; @@ -761,8 +769,10 @@ where match &excepthandler.node { ExcepthandlerKind::ExceptHandler { name, .. } => match name { Some(name) => { - let scope = self.scopes.last().expect("No current scope found."); + let scope = + &self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; if scope.values.contains_key(name) { + let parent = self.parents.pop().expect("No parnet statement found."); self.handle_node_store( &Expr::new( excepthandler.location, @@ -771,12 +781,15 @@ where ctx: ExprContext::Store, }, ), - None, + Some(parent), ); + self.parents.push(parent); } - let scope = self.scopes.last().expect("No current scope found."); - let prev_definition = scope.values.get(name).cloned(); + let parent = self.parents.pop().expect("No parnet statement found."); + let scope = + &self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; + let definition = scope.values.get(name).cloned(); self.handle_node_store( &Expr::new( excepthandler.location, @@ -785,13 +798,15 @@ where ctx: ExprContext::Store, }, ), - None, + Some(parent), ); + self.parents.push(parent); walk_excepthandler(self, excepthandler); - let scope = self.scopes.last_mut().expect("No current scope found."); - if let Some(binding) = scope.values.remove(name) { + let scope = &mut self.scopes + [*(self.scope_stack.last().expect("No current scope found."))]; + if let Some(binding) = &scope.values.remove(name) { if self.settings.select.contains(&CheckCode::F841) && binding.used.is_none() { self.checks.push(Check::new( @@ -801,7 +816,7 @@ where } } - if let Some(binding) = prev_definition { + if let Some(binding) = definition { scope.values.insert(name.to_string(), binding); } } @@ -859,41 +874,47 @@ where } } -impl Checker<'_> { +impl<'a> Checker<'a> { fn push_scope(&mut self, scope: Scope) { + self.scope_stack.push(self.scopes.len()); self.scopes.push(scope); } fn pop_scope(&mut self) { - self.dead_scopes - .push(self.scopes.pop().expect("Attempted to pop without scope.")); + self.dead_scopes.push( + self.scope_stack + .pop() + .expect("Attempted to pop without scope."), + ); } fn bind_builtins(&mut self) { + let scope = &mut self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; + for builtin in BUILTINS { - self.add_binding( + scope.values.insert( builtin.to_string(), Binding { kind: BindingKind::Builtin, location: Default::default(), used: None, }, - ) + ); } for builtin in MAGIC_GLOBALS { - self.add_binding( + scope.values.insert( builtin.to_string(), Binding { kind: BindingKind::Builtin, location: Default::default(), used: None, }, - ) + ); } } fn add_binding(&mut self, name: String, binding: Binding) { - let scope = self.scopes.last_mut().expect("No current scope found."); + let scope = &mut self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; // TODO(charlie): Don't treat annotations as assignments if there is an existing value. let binding = match scope.values.get(&name) { @@ -909,14 +930,17 @@ impl Checker<'_> { fn handle_node_load(&mut self, expr: &Expr) { if let ExprKind::Name { id, .. } = &expr.node { - let scope_id = self.scopes.last_mut().expect("No current scope found.").id; + let scope_id = + self.scopes[*(self.scope_stack.last().expect("No current scope found."))].id; + let mut first_iter = true; - let mut in_generators = false; - for scope in self.scopes.iter_mut().rev() { + let mut in_generator = false; + for scope_index in self.scope_stack.iter().rev() { + let scope = &mut self.scopes[*scope_index]; if matches!(scope.kind, ScopeKind::Class) { if id == "__class__" { return; - } else if !first_iter && !in_generators { + } else if !first_iter && !in_generator { continue; } } @@ -926,7 +950,7 @@ impl Checker<'_> { } first_iter = false; - in_generators = matches!(scope.kind, ScopeKind::Generator); + in_generator = matches!(scope.kind, ScopeKind::Generator); } if self.settings.select.contains(&CheckCode::F821) { @@ -940,7 +964,8 @@ impl Checker<'_> { fn handle_node_store(&mut self, expr: &Expr, parent: Option<&Stmt>) { if let ExprKind::Name { id, .. } = &expr.node { - let current = self.scopes.last().expect("No current scope found."); + let current = + &self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; if self.settings.select.contains(&CheckCode::F823) && matches!(current.kind, ScopeKind::Function) @@ -970,14 +995,13 @@ impl Checker<'_> { // TODO(charlie): Handle alternate binding types (like `Annotation`). if id == "__all__" && matches!(current.kind, ScopeKind::Module) - && match parent { - None => false, - Some(stmt) => { + && parent + .map(|stmt| { matches!(stmt.node, StmtKind::Assign { .. }) || matches!(stmt.node, StmtKind::AugAssign { .. }) || matches!(stmt.node, StmtKind::AnnAssign { .. }) - } - } + }) + .unwrap_or_default() { // Really need parent here. self.add_binding( @@ -1003,9 +1027,9 @@ impl Checker<'_> { fn handle_node_delete(&mut self, expr: &Expr) { if let ExprKind::Name { id, .. } = &expr.node { - let current = self.scopes.last_mut().expect("No current scope found."); - if current.values.remove(id).is_none() - && self.settings.select.contains(&CheckCode::F821) + let scope = + &mut self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; + if scope.values.remove(id).is_none() && self.settings.select.contains(&CheckCode::F821) { self.checks.push(Check::new( CheckKind::UndefinedName(id.clone()), @@ -1015,13 +1039,93 @@ impl Checker<'_> { } } - fn check_deferred(&mut self, path: &str) { - // while !self.deferred.is_empty() { - // let value = self.deferred.pop().unwrap(); - // if let Ok(expr) = parser::parse_expression(&value, path) { - // self.visit_expr(&expr, None); - // } - // } + fn check_deferred_annotations<'b>(&mut self, path: &str, allocator: &'b mut Vec) + where + 'b: 'a, + { + while !self.deferred_annotations.is_empty() { + let value = self.deferred_annotations.pop().unwrap(); + if let Ok(expr) = parser::parse_expression(value, path) { + allocator.push(expr); + } + } + for expr in allocator { + self.visit_expr(expr); + } + } + + fn check_deferred_functions(&mut self) { + while !self.deferred_functions.is_empty() { + let (stmt, scopes) = self.deferred_functions.pop().unwrap(); + + self.scope_stack = scopes; + self.push_scope(Scope::new(ScopeKind::Function)); + + match &stmt.node { + StmtKind::FunctionDef { body, args, .. } + | StmtKind::AsyncFunctionDef { body, args, .. } => { + self.visit_arguments(args); + for stmt in body { + self.visit_stmt(stmt); + } + } + _ => {} + } + + let scope = &self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; + for (name, binding) in scope.values.iter() { + // TODO(charlie): Ignore if using `locals`. + if self.settings.select.contains(&CheckCode::F841) + && binding.used.is_none() + && name != "_" + && name != "__tracebackhide__" + && name != "__traceback_info__" + && name != "__traceback_supplement__" + && matches!(binding.kind, BindingKind::Assignment) + { + self.checks.push(Check::new( + CheckKind::UnusedVariable(name.to_string()), + binding.location, + )); + } + } + + self.pop_scope(); + } + } + + fn check_deferred_lambdas(&mut self) { + while !self.deferred_lambdas.is_empty() { + let (expr, scopes) = self.deferred_lambdas.pop().unwrap(); + + self.scope_stack = scopes; + self.push_scope(Scope::new(ScopeKind::Function)); + + if let ExprKind::Lambda { args, body } = &expr.node { + self.visit_arguments(args); + self.visit_expr(body); + } + + let scope = &self.scopes[*(self.scope_stack.last().expect("No current scope found."))]; + for (name, binding) in scope.values.iter() { + // TODO(charlie): Ignore if using `locals`. + if self.settings.select.contains(&CheckCode::F841) + && binding.used.is_none() + && name != "_" + && name != "__tracebackhide__" + && name != "__traceback_info__" + && name != "__traceback_supplement__" + && matches!(binding.kind, BindingKind::Assignment) + { + self.checks.push(Check::new( + CheckKind::UnusedVariable(name.to_string()), + binding.location, + )); + } + } + + self.pop_scope(); + } } fn check_dead_scopes(&mut self) { @@ -1031,7 +1135,9 @@ impl Checker<'_> { return; } - for scope in &self.dead_scopes { + for index in self.dead_scopes.clone() { + let scope = &self.scopes[index]; + let all_binding = scope.values.get("__all__"); let all_names = all_binding.and_then(|binding| match &binding.kind { BindingKind::Export(names) => Some(names), @@ -1086,48 +1192,26 @@ pub fn check_ast( settings: &Settings, autofix: &autofix::Mode, path: &str, - stmt_allocator: &mut Vec, - expr_allocator: &mut Vec, ) -> Vec { let mut checker = Checker::new(settings, autofix, path, content); checker.push_scope(Scope::new(ScopeKind::Module)); checker.bind_builtins(); + // Iterate over the AST. for stmt in python_ast { checker.visit_stmt(stmt); } - // Check deferred annotations. - while !checker.deferred_annotations.is_empty() { - let value = checker.deferred_annotations.pop().unwrap(); - if let Ok(expr) = parser::parse_expression(&value, path) { - expr_allocator.push(expr); - } - } - for expr in expr_allocator { - checker.visit_expr(expr, None); - } - - // Check deferred annotations. - while !checker.deferred_functions.is_empty() { - let (stmt, scopes) = checker.deferred_functions.pop().unwrap(); - - checker.scopes = scopes.to_vec(); - checker.scopes.push(Scope::new(ScopeKind::Function)); - match &stmt.node { - StmtKind::FunctionDef { body, .. } | StmtKind::AsyncFunctionDef { body, .. } => { - for stmt in body { - checker.visit_stmt(stmt); - } - } - _ => {} - } - if let Some(scope) = checker.scopes.pop() { - checker.dead_scopes.push(scope); - } - } + // Check any deferred statements. + let mut allocator = vec![]; + checker.check_deferred_annotations(path, &mut allocator); + checker.check_deferred_functions(); + checker.check_deferred_lambdas(); + // Reset the scope to module-level, and check all consumed scopes. + checker.scope_stack = vec![GLOBAL_SCOPE_INDEX]; checker.pop_scope(); checker.check_dead_scopes(); + checker.checks } diff --git a/src/linter.rs b/src/linter.rs index 774b4f60a4..d4aa7d7f7c 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -2,7 +2,6 @@ use std::path::Path; use anyhow::Result; use log::debug; -use rustpython_parser::ast::{Expr, Stmt}; use rustpython_parser::parser; use crate::autofix::fix_file; @@ -28,17 +27,7 @@ fn check_path(path: &Path, settings: &Settings, autofix: &autofix::Mode) -> Resu { let path = path.to_string_lossy(); let python_ast = parser::parse_program(&contents, &path)?; - let mut stmt_allocator: Vec = vec![]; - let mut expr_allocator: Vec = vec![]; - checks.extend(check_ast( - &python_ast, - &contents, - settings, - autofix, - &path, - &mut stmt_allocator, - &mut expr_allocator, - )); + checks.extend(check_ast(&python_ast, &contents, settings, autofix, &path)); } // Run the lines-based checks. @@ -101,7 +90,7 @@ mod tests { #[test] fn e402() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/E402.py"), &settings::Settings { line_length: 88, @@ -110,6 +99,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![Check { kind: CheckKind::ModuleImportNotAtTopOfFile, location: Location::new(20, 1), @@ -125,7 +115,7 @@ mod tests { #[test] fn e501() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/E501.py"), &settings::Settings { line_length: 88, @@ -134,6 +124,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![Check { kind: CheckKind::LineTooLong, location: Location::new(5, 89), @@ -149,7 +140,7 @@ mod tests { #[test] fn e711() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/E711.py"), &settings::Settings { line_length: 88, @@ -158,6 +149,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::NoneComparison(RejectedCmpop::Eq), @@ -180,7 +172,7 @@ mod tests { #[test] fn e712() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/E712.py"), &settings::Settings { line_length: 88, @@ -189,6 +181,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::TrueFalseComparison(true, RejectedCmpop::Eq), @@ -222,7 +215,7 @@ mod tests { #[test] fn e713() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/E713.py"), &settings::Settings { line_length: 88, @@ -231,6 +224,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![Check { kind: CheckKind::NotInTest, location: Location::new(2, 12), @@ -246,7 +240,7 @@ mod tests { #[test] fn e714() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/E714.py"), &settings::Settings { line_length: 88, @@ -255,6 +249,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![Check { kind: CheckKind::NotIsTest, location: Location::new(1, 13), @@ -270,7 +265,7 @@ mod tests { #[test] fn e731() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/E731.py"), &settings::Settings { line_length: 88, @@ -279,6 +274,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::DoNotAssignLambda, @@ -302,7 +298,7 @@ mod tests { #[test] fn f401() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F401.py"), &settings::Settings { line_length: 88, @@ -311,12 +307,8 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ - Check { - kind: CheckKind::UnusedImport("logging.handlers".to_string()), - location: Location::new(12, 1), - fix: None, - }, Check { kind: CheckKind::UnusedImport("functools".to_string()), location: Location::new(3, 1), @@ -327,6 +319,11 @@ mod tests { location: Location::new(4, 1), fix: None, }, + Check { + kind: CheckKind::UnusedImport("logging.handlers".to_string()), + location: Location::new(12, 1), + fix: None, + }, ]; assert_eq!(actual.len(), expected.len()); for i in 0..actual.len() { @@ -338,7 +335,7 @@ mod tests { #[test] fn f403() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F403.py"), &settings::Settings { line_length: 88, @@ -347,6 +344,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::ImportStarUsage, @@ -368,7 +366,7 @@ mod tests { } #[test] fn f541() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F541.py"), &settings::Settings { line_length: 88, @@ -377,6 +375,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::FStringMissingPlaceholders, @@ -464,7 +463,7 @@ mod tests { #[test] fn f631() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F631.py"), &settings::Settings { line_length: 88, @@ -473,6 +472,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::AssertTuple, @@ -495,7 +495,7 @@ mod tests { #[test] fn f634() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F634.py"), &settings::Settings { line_length: 88, @@ -504,6 +504,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::IfTuple, @@ -526,7 +527,7 @@ mod tests { #[test] fn f704() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F704.py"), &settings::Settings { line_length: 88, @@ -535,6 +536,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::YieldOutsideFunction, @@ -562,7 +564,7 @@ mod tests { #[test] fn f706() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F706.py"), &settings::Settings { line_length: 88, @@ -571,6 +573,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::ReturnOutsideFunction, @@ -593,7 +596,7 @@ mod tests { #[test] fn f707() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F707.py"), &settings::Settings { line_length: 88, @@ -602,6 +605,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::DefaultExceptNotLast, @@ -629,7 +633,7 @@ mod tests { #[test] fn f821() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F821.py"), &settings::Settings { line_length: 88, @@ -638,6 +642,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::UndefinedName("self".to_string()), @@ -670,7 +675,7 @@ mod tests { #[test] fn f822() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F822.py"), &settings::Settings { line_length: 88, @@ -679,6 +684,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![Check { kind: CheckKind::UndefinedExport("b".to_string()), location: Location::new(3, 1), @@ -694,7 +700,7 @@ mod tests { #[test] fn f823() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F823.py"), &settings::Settings { line_length: 88, @@ -703,6 +709,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![Check { kind: CheckKind::UndefinedLocal("my_var".to_string()), location: Location::new(6, 5), @@ -718,7 +725,7 @@ mod tests { #[test] fn f831() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F831.py"), &settings::Settings { line_length: 88, @@ -727,6 +734,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::DuplicateArgumentName, @@ -754,7 +762,7 @@ mod tests { #[test] fn f841() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F841.py"), &settings::Settings { line_length: 88, @@ -763,6 +771,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::UnusedVariable("e".to_string()), @@ -785,7 +794,7 @@ mod tests { #[test] fn f901() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/F901.py"), &settings::Settings { line_length: 88, @@ -794,6 +803,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::RaiseNotImplemented, @@ -816,7 +826,7 @@ mod tests { #[test] fn r001() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/R001.py"), &settings::Settings { line_length: 88, @@ -825,6 +835,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::UselessObjectInheritance("A".to_string()), @@ -1037,7 +1048,7 @@ mod tests { #[test] fn r002() -> Result<()> { - let actual = check_path( + let mut actual = check_path( Path::new("./resources/test/fixtures/R002.py"), &settings::Settings { line_length: 88, @@ -1046,6 +1057,7 @@ mod tests { }, &autofix::Mode::Generate, )?; + actual.sort_by_key(|check| check.location); let expected = vec![ Check { kind: CheckKind::NoAssertEquals, diff --git a/src/visitor.rs b/src/visitor.rs index d9780506b6..7fb41b99ac 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -11,7 +11,7 @@ pub trait Visitor<'a> { fn visit_annotation(&mut self, expr: &'a Expr) { walk_expr(self, expr); } - fn visit_expr(&mut self, expr: &'a Expr, _parent: Option<&Stmt>) { + fn visit_expr(&mut self, expr: &'a Expr) { walk_expr(self, expr); } fn visit_constant(&mut self, constant: &'a Constant) { @@ -63,43 +63,82 @@ pub trait Visitor<'a> { pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { match &stmt.node { - StmtKind::FunctionDef { args, body, .. } => { + StmtKind::FunctionDef { + args, + body, + decorator_list, + returns, + .. + } => { visitor.visit_arguments(args); - // for stmt in body { - // visitor.visit_stmt(stmt) - // } + for expr in decorator_list { + visitor.visit_expr(expr) + } + for expr in returns { + visitor.visit_annotation(expr); + } + for stmt in body { + visitor.visit_stmt(stmt) + } } - StmtKind::AsyncFunctionDef { args, body, .. } => { + StmtKind::AsyncFunctionDef { + args, + body, + decorator_list, + returns, + .. + } => { visitor.visit_arguments(args); - // for stmt in body { - // visitor.visit_stmt(stmt) - // } + for expr in decorator_list { + visitor.visit_expr(expr) + } + for expr in returns { + visitor.visit_annotation(expr); + } + for stmt in body { + visitor.visit_stmt(stmt) + } } - StmtKind::ClassDef { body, .. } => { + StmtKind::ClassDef { + bases, + keywords, + body, + decorator_list, + .. + } => { + for expr in bases { + visitor.visit_expr(expr) + } + for keyword in keywords { + visitor.visit_keyword(keyword) + } + for expr in decorator_list { + visitor.visit_expr(expr) + } for stmt in body { visitor.visit_stmt(stmt) } } StmtKind::Return { value } => { if let Some(expr) = value { - visitor.visit_expr(expr, Some(stmt)) + visitor.visit_expr(expr) } } StmtKind::Delete { targets } => { for expr in targets { - visitor.visit_expr(expr, Some(stmt)) + visitor.visit_expr(expr) } } StmtKind::Assign { targets, value, .. } => { for expr in targets { - visitor.visit_expr(expr, Some(stmt)) + visitor.visit_expr(expr) } - visitor.visit_expr(value, Some(stmt)) + visitor.visit_expr(value) } StmtKind::AugAssign { target, op, value } => { - visitor.visit_expr(target, Some(stmt)); + visitor.visit_expr(target); visitor.visit_operator(op); - visitor.visit_expr(value, Some(stmt)); + visitor.visit_expr(value); } StmtKind::AnnAssign { target, @@ -107,10 +146,10 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { value, .. } => { - visitor.visit_expr(target, Some(stmt)); + visitor.visit_expr(target); visitor.visit_annotation(annotation); if let Some(expr) = value { - visitor.visit_expr(expr, Some(stmt)) + visitor.visit_expr(expr) } } StmtKind::For { @@ -120,8 +159,8 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { orelse, .. } => { - visitor.visit_expr(target, Some(stmt)); - visitor.visit_expr(iter, Some(stmt)); + visitor.visit_expr(target); + visitor.visit_expr(iter); for stmt in body { visitor.visit_stmt(stmt) } @@ -136,8 +175,8 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { orelse, .. } => { - visitor.visit_expr(target, Some(stmt)); - visitor.visit_expr(iter, Some(stmt)); + visitor.visit_expr(target); + visitor.visit_expr(iter); for stmt in body { visitor.visit_stmt(stmt) } @@ -146,7 +185,7 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { } } StmtKind::While { test, body, orelse } => { - visitor.visit_expr(test, Some(stmt)); + visitor.visit_expr(test); for stmt in body { visitor.visit_stmt(stmt) } @@ -155,7 +194,7 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { } } StmtKind::If { test, body, orelse } => { - visitor.visit_expr(test, Some(stmt)); + visitor.visit_expr(test); for stmt in body { visitor.visit_stmt(stmt) } @@ -181,17 +220,17 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { } StmtKind::Match { subject, cases } => { // TODO(charlie): Handle `cases`. - visitor.visit_expr(subject, Some(stmt)); + visitor.visit_expr(subject); for match_case in cases { visitor.visit_match_case(match_case); } } StmtKind::Raise { exc, cause } => { if let Some(expr) = exc { - visitor.visit_expr(expr, Some(stmt)) + visitor.visit_expr(expr) }; if let Some(expr) = cause { - visitor.visit_expr(expr, Some(stmt)) + visitor.visit_expr(expr) }; } StmtKind::Try { @@ -214,9 +253,9 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { } } StmtKind::Assert { test, msg } => { - visitor.visit_expr(test, None); + visitor.visit_expr(test); if let Some(expr) = msg { - visitor.visit_expr(expr, Some(stmt)) + visitor.visit_expr(expr) } } StmtKind::Import { names } => { @@ -231,7 +270,7 @@ pub fn walk_stmt<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, stmt: &'a Stmt) { } StmtKind::Global { .. } => {} StmtKind::Nonlocal { .. } => {} - StmtKind::Expr { value } => visitor.visit_expr(value, Some(stmt)), + StmtKind::Expr { value } => visitor.visit_expr(value), StmtKind::Pass => {} StmtKind::Break => {} StmtKind::Continue => {} @@ -243,55 +282,55 @@ pub fn walk_expr<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, expr: &'a Expr) { ExprKind::BoolOp { op, values } => { visitor.visit_boolop(op); for expr in values { - visitor.visit_expr(expr, None) + visitor.visit_expr(expr) } } ExprKind::NamedExpr { target, value } => { - visitor.visit_expr(target, None); - visitor.visit_expr(value, None); + visitor.visit_expr(target); + visitor.visit_expr(value); } ExprKind::BinOp { left, op, right } => { - visitor.visit_expr(left, None); + visitor.visit_expr(left); visitor.visit_operator(op); - visitor.visit_expr(right, None); + visitor.visit_expr(right); } ExprKind::UnaryOp { op, operand } => { visitor.visit_unaryop(op); - visitor.visit_expr(operand, None); + visitor.visit_expr(operand); } ExprKind::Lambda { args, body } => { visitor.visit_arguments(args); - visitor.visit_expr(body, None); + visitor.visit_expr(body); } ExprKind::IfExp { test, body, orelse } => { - visitor.visit_expr(test, None); - visitor.visit_expr(body, None); - visitor.visit_expr(orelse, None); + visitor.visit_expr(test); + visitor.visit_expr(body); + visitor.visit_expr(orelse); } ExprKind::Dict { keys, values } => { for expr in keys { - visitor.visit_expr(expr, None) + visitor.visit_expr(expr) } for expr in values { - visitor.visit_expr(expr, None) + visitor.visit_expr(expr) } } ExprKind::Set { elts } => { for expr in elts { - visitor.visit_expr(expr, None) + visitor.visit_expr(expr) } } ExprKind::ListComp { elt, generators } => { for comprehension in generators { visitor.visit_comprehension(comprehension) } - visitor.visit_expr(elt, None); + visitor.visit_expr(elt); } ExprKind::SetComp { elt, generators } => { for comprehension in generators { visitor.visit_comprehension(comprehension) } - visitor.visit_expr(elt, None); + visitor.visit_expr(elt); } ExprKind::DictComp { key, @@ -301,33 +340,33 @@ pub fn walk_expr<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, expr: &'a Expr) { for comprehension in generators { visitor.visit_comprehension(comprehension) } - visitor.visit_expr(key, None); - visitor.visit_expr(value, None); + visitor.visit_expr(key); + visitor.visit_expr(value); } ExprKind::GeneratorExp { elt, generators } => { for comprehension in generators { visitor.visit_comprehension(comprehension) } - visitor.visit_expr(elt, None); + visitor.visit_expr(elt); } - ExprKind::Await { value } => visitor.visit_expr(value, None), + ExprKind::Await { value } => visitor.visit_expr(value), ExprKind::Yield { value } => { if let Some(expr) = value { - visitor.visit_expr(expr, None) + visitor.visit_expr(expr) } } - ExprKind::YieldFrom { value } => visitor.visit_expr(value, None), + ExprKind::YieldFrom { value } => visitor.visit_expr(value), ExprKind::Compare { left, ops, comparators, } => { - visitor.visit_expr(left, None); + visitor.visit_expr(left); for cmpop in ops { visitor.visit_cmpop(cmpop); } for expr in comparators { - visitor.visit_expr(expr, None) + visitor.visit_expr(expr) } } ExprKind::Call { @@ -335,9 +374,9 @@ pub fn walk_expr<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, expr: &'a Expr) { args, keywords, } => { - visitor.visit_expr(func, None); + visitor.visit_expr(func); for expr in args { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } for keyword in keywords { visitor.visit_keyword(keyword); @@ -346,28 +385,28 @@ pub fn walk_expr<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, expr: &'a Expr) { ExprKind::FormattedValue { value, format_spec, .. } => { - visitor.visit_expr(value, None); + visitor.visit_expr(value); if let Some(expr) = format_spec { - visitor.visit_expr(expr, None) + visitor.visit_expr(expr) } } ExprKind::JoinedStr { values } => { for expr in values { - visitor.visit_expr(expr, None) + visitor.visit_expr(expr) } } ExprKind::Constant { value, .. } => visitor.visit_constant(value), ExprKind::Attribute { value, ctx, .. } => { - visitor.visit_expr(value, None); + visitor.visit_expr(value); visitor.visit_expr_context(ctx); } ExprKind::Subscript { value, slice, ctx } => { - visitor.visit_expr(value, None); - visitor.visit_expr(slice, None); + visitor.visit_expr(value); + visitor.visit_expr(slice); visitor.visit_expr_context(ctx); } ExprKind::Starred { value, ctx } => { - visitor.visit_expr(value, None); + visitor.visit_expr(value); visitor.visit_expr_context(ctx); } ExprKind::Name { ctx, .. } => { @@ -375,25 +414,25 @@ pub fn walk_expr<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, expr: &'a Expr) { } ExprKind::List { elts, ctx } => { for expr in elts { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } visitor.visit_expr_context(ctx); } ExprKind::Tuple { elts, ctx } => { for expr in elts { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } visitor.visit_expr_context(ctx); } ExprKind::Slice { lower, upper, step } => { if let Some(expr) = lower { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } if let Some(expr) = upper { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } if let Some(expr) = step { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } } } @@ -411,10 +450,10 @@ pub fn walk_comprehension<'a, V: Visitor<'a> + ?Sized>( visitor: &mut V, comprehension: &'a Comprehension, ) { - visitor.visit_expr(&comprehension.target, None); - visitor.visit_expr(&comprehension.iter, None); + visitor.visit_expr(&comprehension.target); + visitor.visit_expr(&comprehension.iter); for expr in &comprehension.ifs { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } } @@ -425,7 +464,7 @@ pub fn walk_excepthandler<'a, V: Visitor<'a> + ?Sized>( match &excepthandler.node { ExcepthandlerKind::ExceptHandler { type_, body, .. } => { if let Some(expr) = type_ { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } for stmt in body { visitor.visit_stmt(stmt); @@ -448,13 +487,13 @@ pub fn walk_arguments<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, arguments: & visitor.visit_arg(arg); } for expr in &arguments.kw_defaults { - visitor.visit_expr(expr, None) + visitor.visit_expr(expr) } if let Some(arg) = &arguments.kwarg { visitor.visit_arg(arg) } for expr in &arguments.defaults { - visitor.visit_expr(expr, None) + visitor.visit_expr(expr) } } @@ -465,20 +504,20 @@ pub fn walk_arg<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, arg: &'a Arg) { } pub fn walk_keyword<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, keyword: &'a Keyword) { - visitor.visit_expr(&keyword.node.value, None); + visitor.visit_expr(&keyword.node.value); } pub fn walk_withitem<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, withitem: &'a Withitem) { - visitor.visit_expr(&withitem.context_expr, None); + visitor.visit_expr(&withitem.context_expr); if let Some(expr) = &withitem.optional_vars { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } } pub fn walk_match_case<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, match_case: &'a MatchCase) { visitor.visit_pattern(&match_case.pattern); if let Some(expr) = &match_case.guard { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } for stmt in &match_case.body { visitor.visit_stmt(stmt); @@ -487,7 +526,7 @@ pub fn walk_match_case<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, match_case: pub fn walk_pattern<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, pattern: &'a Pattern) { match &pattern.node { - PatternKind::MatchValue { value } => visitor.visit_expr(value, None), + PatternKind::MatchValue { value } => visitor.visit_expr(value), PatternKind::MatchSingleton { value } => visitor.visit_constant(value), PatternKind::MatchSequence { patterns } => { for pattern in patterns { @@ -496,7 +535,7 @@ pub fn walk_pattern<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, pattern: &'a P } PatternKind::MatchMapping { keys, patterns, .. } => { for expr in keys { - visitor.visit_expr(expr, None); + visitor.visit_expr(expr); } for pattern in patterns { visitor.visit_pattern(pattern); @@ -508,7 +547,7 @@ pub fn walk_pattern<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, pattern: &'a P kwd_patterns, .. } => { - visitor.visit_expr(cls, None); + visitor.visit_expr(cls); for pattern in patterns { visitor.visit_pattern(pattern); }