Iterate on CST traversal

src/ast_visitor.rs

@@ -4,7 +4,7 @@ use rustpython_parser::ast::{
     PatternKind, Stmt, StmtKind, Unaryop, Withitem,
 };
 
-pub trait Visitor {
+pub trait ASTVisitor {
     fn visit_stmt(&mut self, stmt: &Stmt) {
         walk_stmt(self, stmt);
     }
@@ -61,7 +61,7 @@ pub trait Visitor {
     }
 }
 
-pub fn walk_stmt<V: Visitor + ?Sized>(visitor: &mut V, stmt: &Stmt) {
+pub fn walk_stmt<V: ASTVisitor + ?Sized>(visitor: &mut V, stmt: &Stmt) {
     match &stmt.node {
         StmtKind::FunctionDef { args, body, .. } => {
             visitor.visit_arguments(args);
@@ -238,7 +238,7 @@ pub fn walk_stmt<V: Visitor + ?Sized>(visitor: &mut V, stmt: &Stmt) {
     }
 }
 
-pub fn walk_expr<V: Visitor + ?Sized>(visitor: &mut V, expr: &Expr) {
+pub fn walk_expr<V: ASTVisitor + ?Sized>(visitor: &mut V, expr: &Expr) {
     match &expr.node {
         ExprKind::BoolOp { op, values } => {
             visitor.visit_boolop(op);
@@ -399,7 +399,7 @@ pub fn walk_expr<V: Visitor + ?Sized>(visitor: &mut V, expr: &Expr) {
     }
 }
 
-pub fn walk_constant<V: Visitor + ?Sized>(visitor: &mut V, constant: &Constant) {
+pub fn walk_constant<V: ASTVisitor + ?Sized>(visitor: &mut V, constant: &Constant) {
     if let Constant::Tuple(constants) = constant {
         for constant in constants {
             visitor.visit_constant(constant)
@@ -407,7 +407,7 @@ pub fn walk_constant<V: Visitor + ?Sized>(visitor: &mut V, constant: &Constant)
     }
 }
 
-pub fn walk_comprehension<V: Visitor + ?Sized>(visitor: &mut V, comprehension: &Comprehension) {
+pub fn walk_comprehension<V: ASTVisitor + ?Sized>(visitor: &mut V, comprehension: &Comprehension) {
     visitor.visit_expr(&comprehension.target, None);
     visitor.visit_expr(&comprehension.iter, None);
     for expr in &comprehension.ifs {
@@ -415,7 +415,7 @@ pub fn walk_comprehension<V: Visitor + ?Sized>(visitor: &mut V, comprehension: &
     }
 }
 
-pub fn walk_excepthandler<V: Visitor + ?Sized>(visitor: &mut V, excepthandler: &Excepthandler) {
+pub fn walk_excepthandler<V: ASTVisitor + ?Sized>(visitor: &mut V, excepthandler: &Excepthandler) {
     match &excepthandler.node {
         ExcepthandlerKind::ExceptHandler { type_, body, .. } => {
             if let Some(expr) = type_ {
@@ -428,7 +428,7 @@ pub fn walk_excepthandler<V: Visitor + ?Sized>(visitor: &mut V, excepthandler: &
     }
 }
 
-pub fn walk_arguments<V: Visitor + ?Sized>(visitor: &mut V, arguments: &Arguments) {
+pub fn walk_arguments<V: ASTVisitor + ?Sized>(visitor: &mut V, arguments: &Arguments) {
     for arg in &arguments.posonlyargs {
         visitor.visit_arg(arg);
     }
@@ -452,24 +452,24 @@ pub fn walk_arguments<V: Visitor + ?Sized>(visitor: &mut V, arguments: &Argument
     }
 }
 
-pub fn walk_arg<V: Visitor + ?Sized>(visitor: &mut V, arg: &Arg) {
+pub fn walk_arg<V: ASTVisitor + ?Sized>(visitor: &mut V, arg: &Arg) {
     if let Some(expr) = &arg.node.annotation {
         visitor.visit_annotation(expr)
     }
 }
 
-pub fn walk_keyword<V: Visitor + ?Sized>(visitor: &mut V, keyword: &Keyword) {
+pub fn walk_keyword<V: ASTVisitor + ?Sized>(visitor: &mut V, keyword: &Keyword) {
     visitor.visit_expr(&keyword.node.value, None);
 }
 
-pub fn walk_withitem<V: Visitor + ?Sized>(visitor: &mut V, withitem: &Withitem) {
+pub fn walk_withitem<V: ASTVisitor + ?Sized>(visitor: &mut V, withitem: &Withitem) {
     visitor.visit_expr(&withitem.context_expr, None);
     if let Some(expr) = &withitem.optional_vars {
         visitor.visit_expr(expr, None);
     }
 }
 
-pub fn walk_match_case<V: Visitor + ?Sized>(visitor: &mut V, match_case: &MatchCase) {
+pub fn walk_match_case<V: ASTVisitor + ?Sized>(visitor: &mut V, match_case: &MatchCase) {
     visitor.visit_pattern(&match_case.pattern);
     if let Some(expr) = &match_case.guard {
         visitor.visit_expr(expr, None);
@@ -479,7 +479,7 @@ pub fn walk_match_case<V: Visitor + ?Sized>(visitor: &mut V, match_case: &MatchC
     }
 }
 
-pub fn walk_pattern<V: Visitor + ?Sized>(visitor: &mut V, pattern: &Pattern) {
+pub fn walk_pattern<V: ASTVisitor + ?Sized>(visitor: &mut V, pattern: &Pattern) {
     match &pattern.node {
         PatternKind::MatchValue { value } => visitor.visit_expr(value, None),
         PatternKind::MatchSingleton { value } => visitor.visit_constant(value),
@@ -527,24 +527,24 @@ pub fn walk_pattern<V: Visitor + ?Sized>(visitor: &mut V, pattern: &Pattern) {
 
 #[allow(unused_variables)]
 #[inline(always)]
-pub fn walk_expr_context<V: Visitor + ?Sized>(visitor: &mut V, expr_context: &ExprContext) {}
+pub fn walk_expr_context<V: ASTVisitor + ?Sized>(visitor: &mut V, expr_context: &ExprContext) {}
 
 #[allow(unused_variables)]
 #[inline(always)]
-pub fn walk_boolop<V: Visitor + ?Sized>(visitor: &mut V, boolop: &Boolop) {}
+pub fn walk_boolop<V: ASTVisitor + ?Sized>(visitor: &mut V, boolop: &Boolop) {}
 
 #[allow(unused_variables)]
 #[inline(always)]
-pub fn walk_operator<V: Visitor + ?Sized>(visitor: &mut V, operator: &Operator) {}
+pub fn walk_operator<V: ASTVisitor + ?Sized>(visitor: &mut V, operator: &Operator) {}
 
 #[allow(unused_variables)]
 #[inline(always)]
-pub fn walk_unaryop<V: Visitor + ?Sized>(visitor: &mut V, unaryop: &Unaryop) {}
+pub fn walk_unaryop<V: ASTVisitor + ?Sized>(visitor: &mut V, unaryop: &Unaryop) {}
 
 #[allow(unused_variables)]
 #[inline(always)]
-pub fn walk_cmpop<V: Visitor + ?Sized>(visitor: &mut V, cmpop: &Cmpop) {}
+pub fn walk_cmpop<V: ASTVisitor + ?Sized>(visitor: &mut V, cmpop: &Cmpop) {}
 
 #[allow(unused_variables)]
 #[inline(always)]
-pub fn walk_alias<V: Visitor + ?Sized>(visitor: &mut V, alias: &Alias) {}
+pub fn walk_alias<V: ASTVisitor + ?Sized>(visitor: &mut V, alias: &Alias) {}

src/autofix.rs

@@ -1,14 +0,0 @@
-use crate::message::Message;
-use crate::settings::Settings;
-use crate::{cache, fs};
-use libcst_native::parse_module;
-
-pub fn autofix(contents: &str, messages: &[Message]) {
-    // Parse the module.
-    let mut m = match parse_module(&contents, None) {
-        Ok(m) => m,
-        Err(e) => panic!("foo"),
-    };
-
-    m.body
-}

src/check_ast.rs

@@ -7,11 +7,11 @@ use rustpython_parser::ast::{
 use rustpython_parser::parser;
 
 use crate::ast_ops::{extract_all_names, Binding, BindingKind, Scope, ScopeKind};
+use crate::ast_visitor;
+use crate::ast_visitor::{walk_excepthandler, ASTVisitor};
 use crate::builtins::{BUILTINS, MAGIC_GLOBALS};
 use crate::checks::{Check, CheckCode, CheckKind};
 use crate::settings::Settings;
-use crate::visitor;
-use crate::visitor::{walk_excepthandler, Visitor};
 
 struct Checker<'a> {
     settings: &'a Settings,
@@ -37,7 +37,7 @@ impl Checker<'_> {
     }
 }
 
-impl Visitor for Checker<'_> {
+impl ASTVisitor for Checker<'_> {
     fn visit_stmt(&mut self, stmt: &Stmt) {
         match &stmt.node {
             StmtKind::Global { names } | StmtKind::Nonlocal { names } => {
@@ -255,7 +255,7 @@ impl Visitor for Checker<'_> {
             _ => {}
         }
 
-        visitor::walk_stmt(self, stmt);
+        ast_visitor::walk_stmt(self, stmt);
 
         match &stmt.node {
             StmtKind::ClassDef { .. } => {
@@ -359,7 +359,7 @@ impl Visitor for Checker<'_> {
             _ => {}
         };
 
-        visitor::walk_expr(self, expr);
+        ast_visitor::walk_expr(self, expr);
 
         match &expr.node {
             ExprKind::GeneratorExp { .. }
@@ -466,7 +466,7 @@ impl Visitor for Checker<'_> {
             }
         }
 
-        visitor::walk_arguments(self, arguments);
+        ast_visitor::walk_arguments(self, arguments);
     }
 
     fn visit_arg(&mut self, arg: &Arg) {
@@ -478,7 +478,7 @@ impl Visitor for Checker<'_> {
                 location: arg.location,
             },
         );
-        visitor::walk_arg(self, arg);
+        ast_visitor::walk_arg(self, arg);
     }
 }
 

src/check_cst.rs

@@ -1,14 +1,11 @@
-use std::collections::{BTreeMap, BTreeSet};
+use std::collections::BTreeMap;
-use std::fmt::format;
 
-use rustpython_parser::ast::{
+use libcst_native::{Expression, If, Module};
-    Arg, Arguments, Expr, ExprContext, ExprKind, Location, Stmt, StmtKind, Suite,
+use rustpython_parser::ast::Location;
-};
 
-use crate::ast_visitor;
-use crate::ast_visitor::ASTVisitor;
-use crate::check_cst::ScopeKind::{Class, Function, Generator, Module};
 use crate::checks::{Check, CheckKind};
+use crate::cst_visitor;
+use crate::cst_visitor::CSTVisitor;
 use crate::settings::Settings;
 
 enum ScopeKind {
@@ -44,9 +41,6 @@ struct Binding {
 struct Checker<'a> {
     settings: &'a Settings,
     checks: Vec<Check>,
-    scopes: Vec<Scope>,
-    dead_scopes: Vec<Scope>,
-    in_f_string: bool,
 }
 
 impl Checker<'_> {
@@ -54,384 +48,27 @@ impl Checker<'_> {
         Checker {
             settings,
             checks: vec![],
-            scopes: vec![],
-            dead_scopes: vec![],
-            in_f_string: false,
         }
     }
 }
 
-impl ASTVisitor for Checker<'_> {
+impl CSTVisitor for Checker<'_> {
-    fn visit_stmt(&mut self, stmt: &Stmt) {
+    fn visit_If<'a>(&'a mut self, node: &'a If) -> &'a If {
-        match &stmt.node {
+        if let Expression::Tuple { .. } = node.test {
-            StmtKind::FunctionDef { name, .. } => {
-                self.push_scope(Scope {
-                    kind: Function,
-                    values: BTreeMap::new(),
-                });
-                self.add_binding(Binding {
-                    kind: BindingKind::ClassDefinition,
-                    name: name.clone(),
-                    used: false,
-                    location: stmt.location,
-                })
-            }
-            StmtKind::AsyncFunctionDef { name, .. } => {
-                self.push_scope(Scope {
-                    kind: Function,
-                    values: BTreeMap::new(),
-                });
-                self.add_binding(Binding {
-                    kind: BindingKind::ClassDefinition,
-                    name: name.clone(),
-                    used: false,
-                    location: stmt.location,
-                })
-            }
-            StmtKind::Return { .. } => {
-                if self
-                    .settings
-                    .select
-                    .contains(CheckKind::ReturnOutsideFunction.code())
-                {
-                    if let Some(scope) = self.scopes.last() {
-                        match scope.kind {
-                            Class | Module => {
-                                self.checks.push(Check {
-                                    kind: CheckKind::ReturnOutsideFunction,
-                                    location: stmt.location,
-                                });
-                            }
-                            _ => {}
-                        }
-                    }
-                }
-            }
-            StmtKind::ClassDef { .. } => self.push_scope(Scope {
-                kind: Class,
-                values: BTreeMap::new(),
-            }),
-            StmtKind::Import { names } => {
-                for alias in names {
-                    if alias.node.name.contains('.') && alias.node.asname.is_none() {
-                        self.add_binding(Binding {
-                            kind: BindingKind::SubmoduleImportation,
-                            name: alias.node.name.clone(),
-                            used: false,
-                            location: stmt.location,
-                        })
-                    } else {
-                        self.add_binding(Binding {
-                            kind: BindingKind::Importation,
-                            name: alias
-                                .node
-                                .asname
-                                .clone()
-                                .unwrap_or_else(|| alias.node.name.clone()),
-                            used: false,
-                            location: stmt.location,
-                        })
-                    }
-                }
-            }
-            StmtKind::ImportFrom { names, module, .. } => {
-                for alias in names {
-                    let name = alias
-                        .node
-                        .asname
-                        .clone()
-                        .unwrap_or_else(|| alias.node.name.clone());
-                    if module
-                        .clone()
-                        .map(|name| name == "future")
-                        .unwrap_or_default()
-                    {
-                        self.add_binding(Binding {
-                            kind: BindingKind::FutureImportation,
-                            name,
-                            used: true,
-                            location: stmt.location,
-                        });
-                    } else if alias.node.name == "*" {
-                        self.add_binding(Binding {
-                            kind: BindingKind::StarImportation,
-                            name,
-                            used: false,
-                            location: stmt.location,
-                        });
-
-                        if self
-                            .settings
-                            .select
-                            .contains(CheckKind::ImportStarUsage.code())
-                        {
-                            self.checks.push(Check {
-                                kind: CheckKind::ImportStarUsage,
-                                location: stmt.location,
-                            });
-                        }
-                    } else {
-                        self.add_binding(Binding {
-                            kind: BindingKind::Importation,
-                            name: match module {
-                                None => name,
-                                Some(parent) => format!("{}.{}", parent, name),
-                            },
-                            used: false,
-                            location: stmt.location,
-                        })
-                    }
-                }
-            }
-            StmtKind::If { test, .. } => {
-                if self.settings.select.contains(CheckKind::IfTuple.code()) {
-                    if let ExprKind::Tuple { .. } = test.node {
             self.checks.push(Check {
                 kind: CheckKind::IfTuple,
-                            location: stmt.location,
+                location: Default::default(),
             });
         }
-                }
+        cst_visitor::walk_If(self, node);
-            }
+        node
-            StmtKind::Raise { exc, .. } => {
-                if self
-                    .settings
-                    .select
-                    .contains(CheckKind::RaiseNotImplemented.code())
-                {
-                    if let Some(expr) = exc {
-                        match &expr.node {
-                            ExprKind::Call { func, .. } => {
-                                if let ExprKind::Name { id, .. } = &func.node {
-                                    if id == "NotImplemented" {
-                                        self.checks.push(Check {
-                                            kind: CheckKind::RaiseNotImplemented,
-                                            location: stmt.location,
-                                        });
-                                    }
-                                }
-                            }
-                            ExprKind::Name { id, .. } => {
-                                if id == "NotImplemented" {
-                                    self.checks.push(Check {
-                                        kind: CheckKind::RaiseNotImplemented,
-                                        location: stmt.location,
-                                    });
-                                }
-                            }
-                            _ => {}
-                        }
-                    }
-                }
-            }
-            StmtKind::AugAssign { target, .. } => {
-                self.handle_node_load(target);
-            }
-            _ => {}
-        }
-
-        ast_visitor::walk_stmt(self, stmt);
-
-        match &stmt.node {
-            StmtKind::ClassDef { .. }
-            | StmtKind::FunctionDef { .. }
-            | StmtKind::AsyncFunctionDef { .. } => {
-                self.pop_scope();
-            }
-            _ => {}
-        };
-
-        if let StmtKind::ClassDef { name, .. } = &stmt.node {
-            self.add_binding(Binding {
-                kind: BindingKind::Definition,
-                name: name.clone(),
-                used: false,
-                location: stmt.location,
-            });
-        }
-    }
-
-    fn visit_expr(&mut self, expr: &Expr) {
-        let initial = self.in_f_string;
-        match &expr.node {
-            ExprKind::Name { ctx, .. } => match ctx {
-                ExprContext::Load => self.handle_node_load(expr),
-                ExprContext::Store => self.handle_node_store(expr),
-                ExprContext::Del => {}
-            },
-            ExprKind::GeneratorExp { .. } => self.push_scope(Scope {
-                kind: Generator,
-                values: BTreeMap::new(),
-            }),
-            ExprKind::Lambda { .. } => self.push_scope(Scope {
-                kind: Function,
-                values: BTreeMap::new(),
-            }),
-            ExprKind::JoinedStr { values } => {
-                if !self.in_f_string
-                    && self
-                        .settings
-                        .select
-                        .contains(CheckKind::FStringMissingPlaceholders.code())
-                    && !values
-                        .iter()
-                        .any(|value| matches!(value.node, ExprKind::FormattedValue { .. }))
-                {
-                    self.checks.push(Check {
-                        kind: CheckKind::FStringMissingPlaceholders,
-                        location: expr.location,
-                    });
-                }
-                self.in_f_string = true;
-            }
-            _ => {}
-        };
-
-        ast_visitor::walk_expr(self, expr);
-
-        match &expr.node {
-            ExprKind::GeneratorExp { .. } | ExprKind::Lambda { .. } => {
-                if let Some(scope) = self.scopes.pop() {
-                    self.dead_scopes.push(scope);
-                }
-            }
-            ExprKind::JoinedStr { .. } => {
-                self.in_f_string = initial;
-            }
-            _ => {}
-        };
-    }
-
-    fn visit_arguments(&mut self, arguments: &Arguments) {
-        if self
-            .settings
-            .select
-            .contains(CheckKind::DuplicateArgumentName.code())
-        {
-            // Collect all the arguments into a single vector.
-            let mut all_arguments: Vec<&Arg> = arguments
-                .args
-                .iter()
-                .chain(arguments.posonlyargs.iter())
-                .chain(arguments.kwonlyargs.iter())
-                .collect();
-            if let Some(arg) = &arguments.vararg {
-                all_arguments.push(arg);
-            }
-            if let Some(arg) = &arguments.kwarg {
-                all_arguments.push(arg);
-            }
-
-            // Search for duplicates.
-            let mut idents: BTreeSet<String> = BTreeSet::new();
-            for arg in all_arguments {
-                let ident = &arg.node.arg;
-                if idents.contains(ident) {
-                    self.checks.push(Check {
-                        kind: CheckKind::DuplicateArgumentName,
-                        location: arg.location,
-                    });
-                    break;
-                }
-                idents.insert(ident.clone());
-            }
-        }
-
-        ast_visitor::walk_arguments(self, arguments);
-    }
-
-    fn visit_arg(&mut self, arg: &Arg) {
-        self.add_binding(Binding {
-            kind: BindingKind::Argument,
-            name: arg.node.arg.clone(),
-            used: false,
-            location: arg.location,
-        });
-        ast_visitor::walk_arg(self, arg);
     }
 }
 
-impl Checker<'_> {
+pub fn check_cst(python_cst: &Module, settings: &Settings) -> Vec<Check> {
-    fn push_scope(&mut self, scope: Scope) {
-        self.scopes.push(scope);
-    }
-
-    fn pop_scope(&mut self) {
-        self.dead_scopes
-            .push(self.scopes.pop().expect("Attempted to pop without scope."));
-    }
-
-    fn add_binding(&mut self, binding: Binding) {
-        // TODO(charlie): Don't treat annotations as assignments if there is an existing value.
-        let scope = self.scopes.last_mut().expect("No current scope found.");
-        scope.values.insert(
-            binding.name.clone(),
-            match scope.values.get(&binding.name) {
-                None => binding,
-                Some(existing) => Binding {
-                    kind: binding.kind,
-                    name: binding.name,
-                    location: binding.location,
-                    used: existing.used,
-                },
-            },
-        );
-    }
-
-    fn handle_node_load(&mut self, expr: &Expr) {
-        if let ExprKind::Name { id, .. } = &expr.node {
-            for scope in self.scopes.iter_mut().rev() {
-                if matches!(scope.kind, Class) {
-                    if id == "__class__" {
-                        return;
-                    } else {
-                        continue;
-                    }
-                }
-                if let Some(binding) = scope.values.get_mut(id) {
-                    binding.used = true;
-                }
-            }
-        }
-    }
-
-    fn handle_node_store(&mut self, expr: &Expr) {
-        if let ExprKind::Name { id, .. } = &expr.node {
-            // TODO(charlie): Handle alternate binding types (like `Annotation`).
-            self.add_binding(Binding {
-                kind: BindingKind::Assignment,
-                name: id.to_string(),
-                used: false,
-                location: expr.location,
-            });
-        }
-    }
-
-    fn check_dead_scopes(&mut self) {
-        // TODO(charlie): Handle `__all__`.
-        for scope in &self.dead_scopes {
-            for (name, binding) in scope.values.iter().rev() {
-                if !binding.used && matches!(binding.kind, BindingKind::Importation) {
-                    self.checks.push(Check {
-                        kind: CheckKind::UnusedImport(name.clone()),
-                        location: binding.location,
-                    });
-                }
-            }
-        }
-    }
-}
-
-pub fn check_ast(python_ast: &Suite, settings: &Settings) -> Vec<Check> {
     let mut checker = Checker::new(settings);
-    checker.push_scope(Scope {
+    for node in &python_cst.body {
-        kind: Module,
+        checker.visit_Statement(node);
-        values: BTreeMap::new(),
-    });
-    for stmt in python_ast {
-        checker.visit_stmt(stmt);
     }
-    checker.pop_scope();
-    checker.check_dead_scopes();
     checker.checks
 }

src/lib.rs

@@ -1,6 +1,5 @@
 mod ast_ops;
 mod ast_visitor;
-mod autofix;
 mod builtins;
 mod cache;
 pub mod check_ast;

src/linter.rs

@@ -5,6 +5,7 @@ use log::debug;
 use rustpython_parser::parser;
 
 use crate::check_ast::check_ast;
+use crate::check_cst::check_cst;
 use crate::check_lines::check_lines;
 use crate::checks::{Check, LintSource};
 use crate::message::Message;
@@ -24,19 +25,26 @@ pub fn check_path(path: &Path, settings: &Settings, mode: &cache::Mode) -> Resul
     // Aggregate all checks.
     let mut checks: Vec<Check> = vec![];
 
-    // Run the AST-based checks.
+    // Run the CST-based checks.
-    if settings
+    let python_cst = match libcst_native::parse_module(&contents, None) {
-        .select
+        Ok(m) => m,
-        .iter()
+        Err(e) => panic!("Failed to parse CST."),
-        .any(|check_code| matches!(check_code.lint_source(), LintSource::AST))
+    };
-    {
+    checks.extend(check_cst(&python_cst, settings));
-        let path = path.to_string_lossy();
-        let python_ast = parser::parse_program(&contents, &path)?;
-        checks.extend(check_ast(&python_ast, settings, &path));
-    }
 
-    // Run the lines-based checks.
+    // // Run the AST-based checks.
-    check_lines(&mut checks, &contents, settings);
+    // if settings
+    //     .select
+    //     .iter()
+    //     .any(|check_code| matches!(check_code.lint_source(), LintSource::AST))
+    // {
+    //     let path = path.to_string_lossy();
+    //     let python_ast = parser::parse_program(&contents, &path)?;
+    //     checks.extend(check_ast(&python_ast, settings, &path));
+    // }
+    //
+    // // Run the lines-based checks.
+    // check_lines(&mut checks, &contents, settings);
 
     // Convert to messages.
     let messages: Vec<Message> = checks
@@ -47,6 +55,7 @@ pub fn check_path(path: &Path, settings: &Settings, mode: &cache::Mode) -> Resul
             filename: path.to_string_lossy().to_string(),
         })
         .collect();
+
     cache::set(path, settings, &messages, mode);
 
     Ok(messages)