diff --git a/Cargo.lock b/Cargo.lock index 4099dce1f6..5fb33fe492 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -188,6 +188,25 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d468802bab17cbc0cc575e9b053f41e72aa36bfa6b7f55e3529ffa43161b97fa" +[[package]] +name = "autoincrement" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dc81b6ad0c9122715c741113fb97ba283f723b1dd86e186dd7df00a2b6877bfe" +dependencies = [ + "autoincrement_derive", +] + +[[package]] +name = "autoincrement_derive" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1259ce9b9a586a3f5ada3d236800ad0aa446b0d4c3dbac4158084a1dd704a228" +dependencies = [ + "quote", + "syn", +] + [[package]] name = "base64" version = "0.10.1" @@ -1638,6 +1657,7 @@ name = "ruff" version = "0.0.19" dependencies = [ "anyhow", + "autoincrement", "bincode", "cacache", "chrono", @@ -1662,7 +1682,7 @@ dependencies = [ [[package]] name = "rustpython-ast" version = "0.1.0" -source = "git+https://github.com/RustPython/RustPython.git?rev=7a688e0b6c2904f286acac1e4af3f1628dd38589#7a688e0b6c2904f286acac1e4af3f1628dd38589" +source = "git+https://github.com/charliermarsh/RustPython.git?rev=97a9d9de687227595932d6c8a04014dcfc892ff4#97a9d9de687227595932d6c8a04014dcfc892ff4" dependencies = [ "num-bigint", "rustpython-compiler-core", @@ -1671,7 +1691,7 @@ dependencies = [ [[package]] name = "rustpython-compiler-core" version = "0.1.2" -source = "git+https://github.com/RustPython/RustPython.git?rev=7a688e0b6c2904f286acac1e4af3f1628dd38589#7a688e0b6c2904f286acac1e4af3f1628dd38589" +source = "git+https://github.com/charliermarsh/RustPython.git?rev=97a9d9de687227595932d6c8a04014dcfc892ff4#97a9d9de687227595932d6c8a04014dcfc892ff4" dependencies = [ "bincode", "bitflags", @@ -1688,7 +1708,7 @@ dependencies = [ [[package]] name = "rustpython-parser" version = "0.1.2" -source = "git+https://github.com/RustPython/RustPython.git?rev=7a688e0b6c2904f286acac1e4af3f1628dd38589#7a688e0b6c2904f286acac1e4af3f1628dd38589" +source = "git+https://github.com/charliermarsh/RustPython.git?rev=97a9d9de687227595932d6c8a04014dcfc892ff4#97a9d9de687227595932d6c8a04014dcfc892ff4" dependencies = [ "ahash", "anyhow", diff --git a/Cargo.toml b/Cargo.toml index 22eda13281..138c512c51 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,6 +8,7 @@ name = "ruff" [dependencies] anyhow = { version = "1.0.60" } +autoincrement = "1.0.1" bincode = { version = "1.3.3" } cacache = { version = "10.0.1" } chrono = { version = "0.4.21" } @@ -22,12 +23,14 @@ log = { version = "0.4.17" } notify = { version = "4.0.17" } rayon = { version = "1.5.3" } regex = { version = "1.6.0" } -rustpython-parser = { features = ["lalrpop"], git = "https://github.com/RustPython/RustPython.git", rev = "7a688e0b6c2904f286acac1e4af3f1628dd38589" } +rustpython-parser = { features = ["lalrpop"], git = "https://github.com/charliermarsh/RustPython.git", rev = "97a9d9de687227595932d6c8a04014dcfc892ff4" } serde = { version = "1.0.143", features = ["derive"] } serde_json = { version = "1.0.83" } toml = { version = "0.5.9" } walkdir = { version = "2.3.2" } [profile.release] -lto = true panic = "abort" +lto = "thin" +codegen-units = 1 +opt-level = 3 diff --git a/resources/test/src/F832.py b/resources/test/src/F832.py new file mode 100644 index 0000000000..44cba4873d --- /dev/null +++ b/resources/test/src/F832.py @@ -0,0 +1,17 @@ +my_dict = {} +my_var = 0 + + +def foo(): + my_var += 1 + + +def bar(): + global my_var + my_var += 1 + + +def baz(): + global my_var + global my_dict + my_dict[my_var] += 1 diff --git a/resources/test/src/pyproject.toml b/resources/test/src/pyproject.toml index 4a5591b8e4..fae64b5a36 100644 --- a/resources/test/src/pyproject.toml +++ b/resources/test/src/pyproject.toml @@ -9,5 +9,6 @@ select = [ "F634", "F706", "F831", + "F832", "F901", ] diff --git a/src/check_ast.rs b/src/check_ast.rs index ce5a89641f..4d990c0e92 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -1,4 +1,5 @@ use std::collections::{BTreeMap, BTreeSet}; +use std::sync::atomic::{AtomicUsize, Ordering}; use rustpython_parser::ast::{ Arg, Arguments, Constant, Expr, ExprContext, ExprKind, Location, Stmt, StmtKind, Suite, @@ -11,6 +12,11 @@ use crate::settings::Settings; use crate::visitor; use crate::visitor::Visitor; +fn id() -> usize { + static COUNTER: AtomicUsize = AtomicUsize::new(1); + COUNTER.fetch_add(1, Ordering::Relaxed) +} + enum ScopeKind { Class, Function, @@ -19,10 +25,21 @@ enum ScopeKind { } struct Scope { + id: usize, kind: ScopeKind, values: BTreeMap, } +impl Scope { + fn new(kind: ScopeKind) -> Self { + Scope { + id: id(), + kind, + values: BTreeMap::new(), + } + } +} + enum BindingKind { Argument, Assignment, @@ -38,7 +55,7 @@ struct Binding { kind: BindingKind, name: String, location: Location, - used: bool, + used: Option, } struct Checker<'a> { @@ -68,27 +85,42 @@ impl Checker<'_> { impl Visitor for Checker<'_> { fn visit_stmt(&mut self, stmt: &Stmt) { 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; + if current_scope_id != global_scope_id { + for name in names { + for scope in self.scopes.iter_mut().skip(global_scope_index + 1) { + scope.values.insert( + name.to_string(), + Binding { + kind: BindingKind::Assignment, + name: name.clone(), + used: Some(global_scope_id), + location: stmt.location, + }, + ); + } + } + } + } StmtKind::FunctionDef { name, .. } => { - self.push_scope(Scope { - kind: Function, - values: BTreeMap::new(), - }); + self.push_scope(Scope::new(Function)); self.add_binding(Binding { kind: BindingKind::ClassDefinition, name: name.clone(), - used: false, + used: None, location: stmt.location, }) } StmtKind::AsyncFunctionDef { name, .. } => { - self.push_scope(Scope { - kind: Function, - values: BTreeMap::new(), - }); + self.push_scope(Scope::new(Function)); self.add_binding(Binding { kind: BindingKind::ClassDefinition, name: name.clone(), - used: false, + used: None, location: stmt.location, }) } @@ -111,17 +143,14 @@ impl Visitor for Checker<'_> { } } } - StmtKind::ClassDef { .. } => self.push_scope(Scope { - kind: Class, - values: BTreeMap::new(), - }), + StmtKind::ClassDef { .. } => self.push_scope(Scope::new(Class)), 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, + used: None, location: stmt.location, }) } else { @@ -138,7 +167,7 @@ impl Visitor for Checker<'_> { .asname .clone() .unwrap_or_else(|| alias.node.name.clone()), - used: false, + used: None, location: stmt.location, }) } @@ -155,14 +184,14 @@ impl Visitor for Checker<'_> { self.add_binding(Binding { kind: BindingKind::FutureImportation, name, - used: true, + used: Some(self.scopes.last().expect("No current scope found.").id), location: stmt.location, }); } else if alias.node.name == "*" { self.add_binding(Binding { kind: BindingKind::StarImportation, name, - used: false, + used: None, location: stmt.location, }); @@ -183,7 +212,7 @@ impl Visitor for Checker<'_> { Some(parent) => format!("{}.{}", parent, name), }), name, - used: false, + used: None, location: stmt.location, }) } @@ -251,7 +280,7 @@ impl Visitor for Checker<'_> { self.add_binding(Binding { kind: BindingKind::Definition, name: name.clone(), - used: false, + used: None, location: stmt.location, }); } @@ -272,14 +301,11 @@ impl Visitor for Checker<'_> { 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::GeneratorExp { .. } + | ExprKind::ListComp { .. } + | ExprKind::DictComp { .. } + | ExprKind::SetComp { .. } => self.push_scope(Scope::new(Generator)), + ExprKind::Lambda { .. } => self.push_scope(Scope::new(Function)), ExprKind::JoinedStr { values } => { if !self.in_f_string && self @@ -307,7 +333,11 @@ impl Visitor for Checker<'_> { visitor::walk_expr(self, expr); match &expr.node { - ExprKind::GeneratorExp { .. } | ExprKind::Lambda { .. } => { + ExprKind::GeneratorExp { .. } + | ExprKind::ListComp { .. } + | ExprKind::DictComp { .. } + | ExprKind::SetComp { .. } + | ExprKind::Lambda { .. } => { if let Some(scope) = self.scopes.pop() { self.dead_scopes.push(scope); } @@ -361,7 +391,7 @@ impl Visitor for Checker<'_> { self.add_binding(Binding { kind: BindingKind::Argument, name: arg.node.arg.clone(), - used: false, + used: None, location: arg.location, }); visitor::walk_arg(self, arg); @@ -379,9 +409,9 @@ impl Checker<'_> { } 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."); + // TODO(charlie): Don't treat annotations as assignments if there is an existing value. scope.values.insert( binding.name.clone(), match scope.values.get(&binding.name) { @@ -398,6 +428,7 @@ 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; for scope in self.scopes.iter_mut().rev() { if matches!(scope.kind, Class) { if id == "__class__" { @@ -407,7 +438,7 @@ impl Checker<'_> { } } if let Some(binding) = scope.values.get_mut(id) { - binding.used = true; + binding.used = Some(scope_id); } } } @@ -415,11 +446,35 @@ impl Checker<'_> { fn handle_node_store(&mut self, expr: &Expr) { if let ExprKind::Name { id, .. } = &expr.node { + if self.settings.select.contains(&CheckCode::F832) { + let current = self.scopes.last().expect("No current scope found."); + if matches!(current.kind, ScopeKind::Function) && !current.values.contains_key(id) { + for scope in self.scopes.iter().rev().skip(1) { + if matches!(scope.kind, ScopeKind::Function) || matches!(scope.kind, Module) + { + let used = scope + .values + .get(id) + .map(|binding| binding.used) + .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, + }); + } + } + } + } + } + } + // TODO(charlie): Handle alternate binding types (like `Annotation`). self.add_binding(Binding { kind: BindingKind::Assignment, name: id.to_string(), - used: false, + used: None, location: expr.location, }); } @@ -438,7 +493,7 @@ impl Checker<'_> { // TODO(charlie): Handle `__all__`. for scope in &self.dead_scopes { for (_, binding) in scope.values.iter().rev() { - if !binding.used { + if binding.used.is_none() { if let BindingKind::Importation(name) = &binding.kind { self.checks.push(Check { kind: CheckKind::UnusedImport(name.clone()), @@ -454,10 +509,7 @@ impl Checker<'_> { pub fn check_ast(python_ast: &Suite, settings: &Settings, path: &str) -> Vec { let mut checker = Checker::new(settings); - checker.push_scope(Scope { - kind: Module, - values: BTreeMap::new(), - }); + checker.push_scope(Scope::new(Module)); for stmt in python_ast { checker.visit_stmt(stmt); diff --git a/src/checks.rs b/src/checks.rs index f8642d94e8..51cff6cede 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -13,6 +13,7 @@ pub enum CheckCode { F634, F706, F831, + F832, F901, } @@ -28,6 +29,7 @@ impl FromStr for CheckCode { "F634" => Ok(CheckCode::F634), "F706" => Ok(CheckCode::F706), "F831" => Ok(CheckCode::F831), + "F832" => Ok(CheckCode::F832), "F901" => Ok(CheckCode::F901), _ => Err(anyhow::anyhow!("Unknown check code: {s}")), } @@ -44,6 +46,7 @@ impl CheckCode { CheckCode::F634 => "F634", CheckCode::F706 => "F706", CheckCode::F831 => "F831", + CheckCode::F832 => "F832", CheckCode::F901 => "F901", } } @@ -58,6 +61,7 @@ impl CheckCode { CheckCode::F634 => &LintSource::AST, CheckCode::F706 => &LintSource::AST, CheckCode::F831 => &LintSource::AST, + CheckCode::F832 => &LintSource::AST, CheckCode::F901 => &LintSource::AST, } } @@ -78,6 +82,7 @@ pub enum CheckKind { LineTooLong, RaiseNotImplemented, ReturnOutsideFunction, + UndefinedLocal(String), UnusedImport(String), } @@ -92,6 +97,7 @@ impl CheckKind { CheckKind::LineTooLong => &CheckCode::E501, CheckKind::RaiseNotImplemented => &CheckCode::F901, CheckKind::ReturnOutsideFunction => &CheckCode::F706, + CheckKind::UndefinedLocal(_) => &CheckCode::F832, CheckKind::UnusedImport(_) => &CheckCode::F401, } } @@ -116,6 +122,9 @@ impl CheckKind { CheckKind::ReturnOutsideFunction => { "a `return` statement outside of a function/method".to_string() } + CheckKind::UndefinedLocal(name) => { + format!("Local variable `{name}` referenced before assignment") + } CheckKind::UnusedImport(name) => format!("`{name}` imported but unused"), } } diff --git a/src/linter.rs b/src/linter.rs index 6539657d77..56bc06417c 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -291,6 +291,30 @@ mod tests { Ok(()) } + #[test] + fn f832() -> Result<()> { + let actual = check_path( + &Path::new("./resources/test/src/F832.py"), + &settings::Settings { + line_length: 88, + exclude: vec![], + select: BTreeSet::from([CheckCode::F832]), + }, + &cache::Mode::None, + )?; + let expected = vec![Message { + kind: CheckKind::UndefinedLocal("my_var".to_string()), + location: Location::new(6, 5), + filename: "./resources/test/src/F832.py".to_string(), + }]; + assert_eq!(actual.len(), expected.len()); + for i in 0..actual.len() { + assert_eq!(actual[i], expected[i]); + } + + Ok(()) + } + #[test] fn f901() -> Result<()> { let actual = check_path( diff --git a/src/pyproject.rs b/src/pyproject.rs index 945b7c926c..f06f9ff5e1 100644 --- a/src/pyproject.rs +++ b/src/pyproject.rs @@ -234,6 +234,7 @@ other-attribute = 1 CheckCode::F634, CheckCode::F706, CheckCode::F831, + CheckCode::F832, CheckCode::F901, ])), } diff --git a/src/settings.rs b/src/settings.rs index 9c2d2acc1f..f4488b6845 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -49,6 +49,7 @@ impl Settings { CheckCode::F634, CheckCode::F706, CheckCode::F831, + CheckCode::F832, CheckCode::F901, ]) }),