From 8091beca8977d4d412c97f9230c5abba7a5daa10 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 12 Aug 2022 17:53:31 -0400 Subject: [PATCH] Implement visitor pattern (#4) --- README.md | 2 +- src/bin/rust_python_linter.rs | 2 +- src/check.rs | 94 ------ src/checker.rs | 46 +++ src/fs.rs | 1 + src/lib.rs | 3 +- src/linter.rs | 11 +- src/message.rs | 77 +++-- src/visitor.rs | 538 ++++++++++++++++++++++++++++++++++ 9 files changed, 632 insertions(+), 142 deletions(-) delete mode 100644 src/check.rs create mode 100644 src/checker.rs create mode 100644 src/visitor.rs diff --git a/README.md b/README.md index f12cb0d33d..a474af6fe5 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ in Rust. Features: -- Python 3.10 compatibility +- Python 3.8 compatibility - [ESLint](https://eslint.org/docs/latest/user-guide/command-line-interface#caching)-inspired cache semantics - [TypeScript](https://www.typescriptlang.org/docs/handbook/configuring-watch.html) diff --git a/src/bin/rust_python_linter.rs b/src/bin/rust_python_linter.rs index 2fc9e88472..173fb024dd 100644 --- a/src/bin/rust_python_linter.rs +++ b/src/bin/rust_python_linter.rs @@ -4,13 +4,13 @@ use std::time::{Duration, Instant}; use ::rust_python_linter::fs::collect_python_files; use ::rust_python_linter::linter::check_path; -use ::rust_python_linter::message::Message; use anyhow::Result; use clap::{Parser, ValueHint}; use colored::Colorize; use log::{debug, error}; use notify::{watcher, RecursiveMode, Watcher}; use rayon::prelude::*; +use rust_python_linter::message::Message; use walkdir::DirEntry; #[derive(Debug, Parser)] diff --git a/src/check.rs b/src/check.rs deleted file mode 100644 index 811916a2ff..0000000000 --- a/src/check.rs +++ /dev/null @@ -1,94 +0,0 @@ -use std::path::Path; - -use rustpython_parser::ast::{ExprKind, Stmt, StmtKind, Suite}; - -use crate::message::Message; - -fn check_statement(path: &Path, stmt: &Stmt) -> Vec { - let mut messages: Vec = vec![]; - match &stmt.node { - StmtKind::FunctionDef { body, .. } => { - messages.extend(body.iter().flat_map(|stmt| check_statement(path, stmt))); - } - StmtKind::AsyncFunctionDef { body, .. } => { - messages.extend(body.iter().flat_map(|stmt| check_statement(path, stmt))); - } - StmtKind::ClassDef { body, .. } => { - messages.extend(body.iter().flat_map(|stmt| check_statement(path, stmt))); - } - StmtKind::Return { .. } => {} - StmtKind::Delete { .. } => {} - StmtKind::Assign { .. } => {} - StmtKind::AugAssign { .. } => {} - StmtKind::AnnAssign { .. } => {} - StmtKind::For { body, orelse, .. } => { - messages.extend(body.iter().flat_map(|stmt| check_statement(path, stmt))); - messages.extend(orelse.iter().flat_map(|stmt| check_statement(path, stmt))); - } - StmtKind::AsyncFor { body, orelse, .. } => { - messages.extend(body.iter().flat_map(|stmt| check_statement(path, stmt))); - messages.extend(orelse.iter().flat_map(|stmt| check_statement(path, stmt))); - } - StmtKind::While { body, orelse, .. } => { - messages.extend(body.iter().flat_map(|stmt| check_statement(path, stmt))); - messages.extend(orelse.iter().flat_map(|stmt| check_statement(path, stmt))); - } - StmtKind::If { test, body, orelse } => { - if let ExprKind::Tuple { .. } = test.node { - messages.push(Message::IfTuple { - filename: path.to_path_buf(), - location: stmt.location, - }); - } - messages.extend(body.iter().flat_map(|stmt| check_statement(path, stmt))); - messages.extend(orelse.iter().flat_map(|stmt| check_statement(path, stmt))); - } - StmtKind::With { body, .. } => { - messages.extend(body.iter().flat_map(|stmt| check_statement(path, stmt))); - } - StmtKind::AsyncWith { body, .. } => { - messages.extend(body.iter().flat_map(|stmt| check_statement(path, stmt))); - } - StmtKind::Raise { .. } => {} - StmtKind::Try { - body, - orelse, - finalbody, - .. - } => { - messages.extend(body.iter().flat_map(|stmt| check_statement(path, stmt))); - messages.extend(orelse.iter().flat_map(|stmt| check_statement(path, stmt))); - messages.extend( - finalbody - .iter() - .flat_map(|stmt| check_statement(path, stmt)), - ); - } - StmtKind::Assert { .. } => {} - StmtKind::Import { .. } => {} - StmtKind::ImportFrom { names, .. } => { - for alias in names { - if alias.name == "*" { - messages.push(Message::ImportStarUsage { - filename: path.to_path_buf(), - location: stmt.location, - }); - } - } - } - StmtKind::Global { .. } => {} - StmtKind::Nonlocal { .. } => {} - StmtKind::Expr { .. } => {} - StmtKind::Pass => {} - StmtKind::Break => {} - StmtKind::Continue => {} - } - messages -} - -pub fn check_ast(path: &Path, python_ast: &Suite) -> Vec { - python_ast - .iter() - .flat_map(|stmt| check_statement(path, stmt)) - .collect() -} diff --git a/src/checker.rs b/src/checker.rs new file mode 100644 index 0000000000..9f6d04d7f2 --- /dev/null +++ b/src/checker.rs @@ -0,0 +1,46 @@ +use rustpython_parser::ast::{ExprKind, Stmt, StmtKind, Suite}; + +use crate::message::{Check, CheckKind}; +use crate::visitor::{walk_stmt, Visitor}; + +struct Checker { + checks: Vec, +} + +impl Visitor for Checker { + fn visit_stmt(&mut self, stmt: &Stmt) { + match &stmt.node { + StmtKind::ImportFrom { names, .. } => { + for alias in names { + if alias.name == "*" { + self.checks.push(Check { + kind: CheckKind::ImportStarUsage, + location: stmt.location, + }); + } + } + } + StmtKind::If { test, .. } => { + if let ExprKind::Tuple { .. } = test.node { + self.checks.push(Check { + kind: CheckKind::IfTuple, + location: stmt.location, + }); + } + } + _ => {} + } + walk_stmt(self, stmt); + } +} + +pub fn check_ast(python_ast: &Suite) -> Vec { + python_ast + .iter() + .flat_map(|stmt| { + let mut checker = Checker { checks: vec![] }; + checker.visit_stmt(stmt); + checker.checks + }) + .collect() +} diff --git a/src/fs.rs b/src/fs.rs index 25d8db4f1d..4f59cdd44f 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -1,4 +1,5 @@ use std::path::PathBuf; + use walkdir::{DirEntry, WalkDir}; fn is_not_hidden(entry: &DirEntry) -> bool { diff --git a/src/lib.rs b/src/lib.rs index 050555b180..49fc92d7bd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,7 @@ mod cache; -mod check; +mod checker; pub mod fs; pub mod linter; pub mod message; mod parser; +mod visitor; diff --git a/src/linter.rs b/src/linter.rs index adfcb28e30..85c8b59cea 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -4,7 +4,7 @@ use anyhow::Result; use log::debug; use serde::{Deserialize, Serialize}; -use crate::check::check_ast; +use crate::checker::check_ast; use crate::message::Message; use crate::{cache, parser}; @@ -29,7 +29,14 @@ pub fn check_path(path: &Path) -> Result> { // Run the linter. let python_ast = parser::parse(path)?; - let messages = check_ast(path, &python_ast); + let messages: Vec = check_ast(&python_ast) + .into_iter() + .map(|check| Message { + kind: check.kind, + location: check.location, + filename: path.to_string_lossy().to_string(), + }) + .collect(); cache::set(path, &messages); Ok(messages) diff --git a/src/message.rs b/src/message.rs index 31a62709e7..af7f16d325 100644 --- a/src/message.rs +++ b/src/message.rs @@ -1,5 +1,4 @@ use std::fmt; -use std::path::PathBuf; use colored::Colorize; use rustpython_parser::ast::Location; @@ -21,62 +20,54 @@ impl From for Location { } #[derive(Serialize, Deserialize)] -pub enum Message { - ImportStarUsage { - filename: PathBuf, - #[serde(with = "LocationDef")] - location: Location, - }, - IfTuple { - filename: PathBuf, - #[serde(with = "LocationDef")] - location: Location, - }, +pub enum CheckKind { + ImportStarUsage, + IfTuple, } -impl Message { - /// A four-letter shorthand code for the message. +impl CheckKind { + /// A four-letter shorthand code for the check. pub fn code(&self) -> &'static str { match self { - Message::ImportStarUsage { .. } => "F403", - Message::IfTuple { .. } => "F634", + CheckKind::ImportStarUsage => "F403", + CheckKind::IfTuple => "F634", } } - /// The body text for the message. + /// The body text for the check. pub fn body(&self) -> &'static str { match self { - Message::ImportStarUsage { .. } => "Unable to detect undefined names", - Message::IfTuple { .. } => "If test is a tuple, which is always `True`", + CheckKind::ImportStarUsage => "Unable to detect undefined names", + CheckKind::IfTuple => "If test is a tuple, which is always `True`", } } } +pub struct Check { + pub kind: CheckKind, + pub location: Location, +} + +#[derive(Serialize, Deserialize)] +pub struct Message { + pub kind: CheckKind, + #[serde(with = "LocationDef")] + pub location: Location, + pub filename: String, +} + impl fmt::Display for Message { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - Message::ImportStarUsage { filename, location } => write!( - f, - "{}{}{}{}{}\t{}\t{}", - filename.to_string_lossy().white().bold(), - ":".cyan(), - location.column(), - ":".cyan(), - location.row(), - self.code().red().bold(), - self.body() - ), - Message::IfTuple { filename, location } => write!( - f, - "{}{}{}{}{}\t{}\t{}", - filename.to_string_lossy().white().bold(), - ":".cyan(), - location.column(), - ":".cyan(), - location.row(), - self.code().red().bold(), - self.body() - ), - } + write!( + f, + "{}{}{}{}{}\t{}\t{}", + self.filename.white().bold(), + ":".cyan(), + self.location.column(), + ":".cyan(), + self.location.row(), + self.kind.code().red().bold(), + self.kind.body() + ) } } diff --git a/src/visitor.rs b/src/visitor.rs new file mode 100644 index 0000000000..1055f0211e --- /dev/null +++ b/src/visitor.rs @@ -0,0 +1,538 @@ +use rustpython_parser::ast::{ + Alias, Arg, Arguments, Boolop, Cmpop, Comprehension, Constant, Excepthandler, + ExcepthandlerKind, Expr, ExprContext, ExprKind, Keyword, Operator, Stmt, StmtKind, Unaryop, + Withitem, +}; + +#[allow(unused_variables)] +pub trait Visitor { + fn visit_ident(&mut self, ident: &str) { + // Nothing to walk. + } + fn visit_constant(&mut self, constant: &Constant) { + walk_constant(self, constant); + } + fn visit_stmt(&mut self, stmt: &Stmt) { + walk_stmt(self, stmt); + } + fn visit_expr(&mut self, expr: &Expr) { + walk_expr(self, expr); + } + fn visit_expr_context(&mut self, expr_content: &ExprContext) { + // Nothing to walk. + } + fn visit_boolop(&mut self, boolop: &Boolop) { + // Nothing to walk. + } + fn visit_operator(&mut self, operator: &Operator) { + // Nothing to walk. + } + fn visit_unaryop(&mut self, unaryop: &Unaryop) { + // Nothing to walk. + } + fn visit_cmpop(&mut self, cmpop: &Cmpop) { + // Nothing to walk. + } + fn visit_comprehension(&mut self, comprehension: &Comprehension) { + walk_comprehension(self, comprehension); + } + fn visit_excepthandler(&mut self, excepthandler: &Excepthandler) { + walk_excepthandler(self, excepthandler); + } + fn visit_arguments(&mut self, arguments: &Arguments) { + walk_arguments(self, arguments); + } + fn visit_arg(&mut self, arg: &Arg) { + walk_arg(self, arg); + } + fn visit_keyword(&mut self, keyword: &Keyword) { + walk_keyword(self, keyword); + } + fn visit_alias(&mut self, alias: &Alias) { + // Nothing to walk. + } + fn visit_withitem(&mut self, withitem: &Withitem) { + walk_withitem(self, withitem); + } +} + +#[allow(unused_variables)] +pub fn walk_stmt(visitor: &mut V, stmt: &Stmt) { + match &stmt.node { + StmtKind::FunctionDef { + name, + args, + body, + decorator_list, + returns, + type_comment, + } => { + visitor.visit_ident(name); + 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_expr(expr); + } + } + StmtKind::AsyncFunctionDef { + name, + args, + body, + decorator_list, + returns, + type_comment, + } => { + visitor.visit_ident(name); + 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_expr(expr); + } + } + StmtKind::ClassDef { + name, + bases, + keywords, + body, + decorator_list, + } => { + visitor.visit_ident(name); + for expr in bases { + visitor.visit_expr(expr) + } + for keyword in keywords { + visitor.visit_keyword(keyword) + } + for stmt in body { + visitor.visit_stmt(stmt) + } + for expr in decorator_list { + visitor.visit_expr(expr) + } + } + StmtKind::Return { value } => { + if let Some(expr) = value { + visitor.visit_expr(expr) + } + } + StmtKind::Delete { targets } => { + for expr in targets { + visitor.visit_expr(expr) + } + } + StmtKind::Assign { + targets, + value, + type_comment, + } => { + for expr in targets { + visitor.visit_expr(expr) + } + visitor.visit_expr(value) + } + StmtKind::AugAssign { target, op, value } => { + // TODO(charlie): Implement operator. + visitor.visit_expr(target); + visitor.visit_expr(value); + } + StmtKind::AnnAssign { + target, + annotation, + value, + simple, + } => { + visitor.visit_expr(target); + visitor.visit_expr(annotation); + if let Some(expr) = value { + visitor.visit_expr(expr) + } + } + StmtKind::For { + target, + iter, + body, + orelse, + type_comment, + } => { + visitor.visit_expr(target); + visitor.visit_expr(iter); + for stmt in body { + visitor.visit_stmt(stmt) + } + for stmt in orelse { + visitor.visit_stmt(stmt) + } + } + StmtKind::AsyncFor { + target, + iter, + body, + orelse, + type_comment, + } => { + visitor.visit_expr(target); + visitor.visit_expr(iter); + for stmt in body { + visitor.visit_stmt(stmt) + } + for stmt in orelse { + visitor.visit_stmt(stmt) + } + } + StmtKind::While { test, body, orelse } => { + visitor.visit_expr(test); + for stmt in body { + visitor.visit_stmt(stmt) + } + for stmt in orelse { + visitor.visit_stmt(stmt) + } + } + StmtKind::If { test, body, orelse } => { + visitor.visit_expr(test); + for stmt in body { + visitor.visit_stmt(stmt) + } + for stmt in orelse { + visitor.visit_stmt(stmt) + } + } + StmtKind::With { + items, + body, + type_comment, + } => { + for withitem in items { + visitor.visit_withitem(withitem); + } + for stmt in body { + visitor.visit_stmt(stmt) + } + } + StmtKind::AsyncWith { + items, + body, + type_comment, + } => { + for withitem in items { + visitor.visit_withitem(withitem); + } + for stmt in body { + visitor.visit_stmt(stmt) + } + } + StmtKind::Raise { exc, cause } => { + if let Some(expr) = exc { + visitor.visit_expr(expr) + }; + if let Some(expr) = cause { + visitor.visit_expr(expr) + }; + } + StmtKind::Try { + body, + handlers, + orelse, + finalbody, + } => { + for stmt in body { + visitor.visit_stmt(stmt) + } + for excepthandler in handlers { + visitor.visit_excepthandler(excepthandler) + } + for stmt in orelse { + visitor.visit_stmt(stmt) + } + for stmt in finalbody { + visitor.visit_stmt(stmt) + } + } + StmtKind::Assert { test, msg } => { + visitor.visit_expr(test); + if let Some(expr) = msg { + visitor.visit_expr(expr) + } + } + StmtKind::Import { names } => { + for alias in names { + visitor.visit_alias(alias); + } + } + StmtKind::ImportFrom { + module, + names, + level, + } => { + for alias in names { + visitor.visit_alias(alias); + } + } + StmtKind::Global { names } => { + for ident in names { + visitor.visit_ident(ident) + } + } + StmtKind::Nonlocal { names } => { + for ident in names { + visitor.visit_ident(ident) + } + } + StmtKind::Expr { value } => visitor.visit_expr(value), + StmtKind::Pass => {} + StmtKind::Break => {} + StmtKind::Continue => {} + } +} + +#[allow(unused_variables)] +fn walk_expr(visitor: &mut V, expr: &Expr) { + match &expr.node { + ExprKind::BoolOp { op, values } => { + visitor.visit_boolop(op); + for expr in values { + visitor.visit_expr(expr) + } + } + ExprKind::NamedExpr { target, value } => { + visitor.visit_expr(target); + visitor.visit_expr(value); + } + ExprKind::BinOp { left, op, right } => { + visitor.visit_expr(left); + visitor.visit_operator(op); + visitor.visit_expr(right); + } + ExprKind::UnaryOp { op, operand } => { + visitor.visit_unaryop(op); + visitor.visit_expr(operand); + } + ExprKind::Lambda { args, body } => { + visitor.visit_arguments(args); + visitor.visit_expr(body); + } + ExprKind::IfExp { test, body, orelse } => { + visitor.visit_expr(test); + visitor.visit_expr(body); + visitor.visit_expr(orelse); + } + ExprKind::Dict { keys, values } => { + for expr in keys.iter().flatten() { + visitor.visit_expr(expr) + } + for expr in values { + visitor.visit_expr(expr) + } + } + ExprKind::Set { elts } => { + for expr in elts { + visitor.visit_expr(expr) + } + } + ExprKind::ListComp { elt, generators } => { + visitor.visit_expr(elt); + for comprehension in generators { + visitor.visit_comprehension(comprehension) + } + } + ExprKind::SetComp { elt, generators } => { + visitor.visit_expr(elt); + for comprehension in generators { + visitor.visit_comprehension(comprehension) + } + } + ExprKind::DictComp { + key, + value, + generators, + } => { + visitor.visit_expr(key); + visitor.visit_expr(value); + for comprehension in generators { + visitor.visit_comprehension(comprehension) + } + } + ExprKind::GeneratorExp { elt, generators } => { + visitor.visit_expr(elt); + for comprehension in generators { + visitor.visit_comprehension(comprehension) + } + } + ExprKind::Await { value } => visitor.visit_expr(value), + ExprKind::Yield { value } => { + if let Some(expr) = value { + visitor.visit_expr(expr) + } + } + ExprKind::YieldFrom { value } => visitor.visit_expr(value), + ExprKind::Compare { + left, + ops, + comparators, + } => { + visitor.visit_expr(left); + for cmpop in ops { + visitor.visit_cmpop(cmpop); + } + for expr in comparators { + visitor.visit_expr(expr) + } + } + ExprKind::Call { + func, + args, + keywords, + } => { + visitor.visit_expr(func); + for expr in args { + visitor.visit_expr(expr); + } + for keyword in keywords { + visitor.visit_keyword(keyword); + } + } + ExprKind::FormattedValue { + value, + conversion, + format_spec, + } => { + visitor.visit_expr(value); + if let Some(expr) = format_spec { + visitor.visit_expr(expr) + } + } + ExprKind::JoinedStr { values } => { + for expr in values { + visitor.visit_expr(expr) + } + } + ExprKind::Constant { value, kind } => visitor.visit_constant(value), + ExprKind::Attribute { value, attr, ctx } => { + visitor.visit_expr(value); + visitor.visit_expr_context(ctx); + } + ExprKind::Subscript { value, slice, ctx } => { + visitor.visit_expr(value); + visitor.visit_expr(slice); + visitor.visit_expr_context(ctx); + } + ExprKind::Starred { value, ctx } => { + visitor.visit_expr(value); + visitor.visit_expr_context(ctx); + } + ExprKind::Name { id, ctx } => { + visitor.visit_ident(id); + visitor.visit_expr_context(ctx); + } + ExprKind::List { elts, ctx } => { + for expr in elts { + visitor.visit_expr(expr); + } + visitor.visit_expr_context(ctx); + } + ExprKind::Tuple { elts, ctx } => { + for expr in elts { + visitor.visit_expr(expr); + } + visitor.visit_expr_context(ctx); + } + ExprKind::Slice { lower, upper, step } => { + if let Some(expr) = lower { + visitor.visit_expr(expr); + } + if let Some(expr) = upper { + visitor.visit_expr(expr); + } + if let Some(expr) = step { + visitor.visit_expr(expr); + } + } + } +} + +#[allow(unused_variables)] +pub fn walk_constant(visitor: &mut V, constant: &Constant) { + if let Constant::Tuple(constants) = constant { + for constant in constants { + visitor.visit_constant(constant) + } + } +} + +#[allow(unused_variables)] +pub fn walk_comprehension(visitor: &mut V, comprehension: &Comprehension) { + visitor.visit_expr(&comprehension.target); + visitor.visit_expr(&comprehension.iter); + for expr in &comprehension.ifs { + visitor.visit_expr(expr); + } +} + +#[allow(unused_variables)] +pub fn walk_excepthandler(visitor: &mut V, excepthandler: &Excepthandler) { + match &excepthandler.node { + ExcepthandlerKind::ExceptHandler { type_, name, body } => { + if let Some(expr) = type_ { + visitor.visit_expr(expr) + } + for stmt in body { + visitor.visit_stmt(stmt) + } + } + } +} + +#[allow(unused_variables)] +pub fn walk_arguments(visitor: &mut V, arguments: &Arguments) { + for arg in &arguments.posonlyargs { + visitor.visit_arg(arg); + } + for arg in &arguments.args { + visitor.visit_arg(arg); + } + if let Some(arg) = &arguments.vararg { + visitor.visit_arg(arg) + } + for arg in &arguments.kwonlyargs { + visitor.visit_arg(arg); + } + for expr in arguments.kw_defaults.iter().flatten() { + visitor.visit_expr(expr) + } + if let Some(arg) = &arguments.kwarg { + visitor.visit_arg(arg) + } + for expr in &arguments.defaults { + visitor.visit_expr(expr) + } +} + +#[allow(unused_variables)] +pub fn walk_arg(visitor: &mut V, arg: &Arg) { + if let Some(expr) = &arg.node.annotation { + visitor.visit_expr(expr) + } +} + +#[allow(unused_variables)] +pub fn walk_keyword(visitor: &mut V, keyword: &Keyword) { + visitor.visit_expr(&keyword.node.value); +} + +#[allow(unused_variables)] +pub fn walk_withitem(visitor: &mut V, withitem: &Withitem) { + visitor.visit_expr(&withitem.context_expr); + if let Some(expr) = &withitem.optional_vars { + visitor.visit_expr(expr); + } +}