ruff icon indicating copy to clipboard operation
ruff copied to clipboard

Come up with a more declarative interface for adding simple checks to `Checker`

Open charliermarsh opened this issue 3 years ago • 2 comments

Right now, whenever we add a check to Checker, we have to expand the visit methods.

It'd be nice if we could instead define a mapping from node kind to check methods (and the codes they correspond to), then add those methods to Checker ahead-of-time, and iterate over them -- or something like that, I haven't really explored any concrete options. Generally, what I'd like is for adding a check to be closer to "adding a (node, code, method) pair to a list" rather than expanding the Checker.

There could also be a benefit here in that if we only add the methods we need ahead-of-time, maybe we can get a small performance gain by avoiding so many self.settings.enabled calls, but I'm not sure.

I'd love proposals or ideas here.

charliermarsh avatar Nov 17 '22 22:11 charliermarsh

The AST structure makes this kind of difficult, because each check function has a slightly different interface. It'd be nice if every ExprKind had its own struct, instead of being an enum, so we could define checks as functions on (Checker, ExprKind).

charliermarsh avatar Nov 17 '22 22:11 charliermarsh

Just putting this up here.

pub trait AstPass {
    fn check_classdef(&self, checker: &mut Checker, stmt: &Stmt) {}
    fn check_call(&self, checker: &mut Checker, expr: &Expr) {}
    ...
}

pub struct AbstractBaseClass;

impl AstPass for AbstractBaseClass {
    fn check_classdef(&self, checker: &mut Checker, stmt: &Stmt) {
         do_stuff();
    }
}

pub struct UselessExpression;

impl AstPass for UselessExpression {
    fn check_functiondef(&self, checker: &mut Checker, stmt: &Stmt) {
        do_stuff();
    }
}
let passes: Vec<Box<dyn AstPass>> = vec![
    Box::new(flake8_bugbear::plugins::UselessExpressionInfo),
    Box::new(flake8_bugbear::plugins::AbstractBaseClass),
];

// somewhere in the ast checker
for pass in &passes {
    pass.check_classdef(self, &stmt);
}

Don't know enough about rust yet to understand any potential performance effect using trait objects here.

squiddy avatar Dec 18 '22 06:12 squiddy

This isn't super actionable -- closing for now.

charliermarsh avatar Sep 14 '23 19:09 charliermarsh