Implement F823 (#44)

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",

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

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

resources/test/src/pyproject.toml

@@ -9,5 +9,6 @@ select = [
     "F634",
     "F706",
     "F831",
+    "F832",
     "F901",
 ]

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<String, Binding>,
 }
 
+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<usize>,
 }
 
 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 {
+                self.push_scope(Scope::new(Function));
-                    kind: Function,
-                    values: BTreeMap::new(),
-                });
                 self.add_binding(Binding {
                     kind: BindingKind::ClassDefinition,
                     name: name.clone(),
-                    used: false,
+                    used: None,
                     location: stmt.location,
                 })
             }
             StmtKind::AsyncFunctionDef { name, .. } => {
-                self.push_scope(Scope {
+                self.push_scope(Scope::new(Function));
-                    kind: Function,
-                    values: BTreeMap::new(),
-                });
                 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 {
+            StmtKind::ClassDef { .. } => self.push_scope(Scope::new(Class)),
-                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,
+                            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 {
+            ExprKind::GeneratorExp { .. }
-                kind: Generator,
+            | ExprKind::ListComp { .. }
-                values: BTreeMap::new(),
+            | ExprKind::DictComp { .. }
-            }),
+            | ExprKind::SetComp { .. } => self.push_scope(Scope::new(Generator)),
-            ExprKind::Lambda { .. } => self.push_scope(Scope {
+            ExprKind::Lambda { .. } => self.push_scope(Scope::new(Function)),
-                kind: Function,
-                values: BTreeMap::new(),
-            }),
             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<Check> {
     let mut checker = Checker::new(settings);
-    checker.push_scope(Scope {
+    checker.push_scope(Scope::new(Module));
-        kind: Module,
-        values: BTreeMap::new(),
-    });
 
     for stmt in python_ast {
         checker.visit_stmt(stmt);

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"),
         }
     }

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(

src/pyproject.rs

@@ -234,6 +234,7 @@ other-attribute = 1
                     CheckCode::F634,
                     CheckCode::F706,
                     CheckCode::F831,
+                    CheckCode::F832,
                     CheckCode::F901,
                 ])),
             }

src/settings.rs

@@ -49,6 +49,7 @@ impl Settings {
                     CheckCode::F634,
                     CheckCode::F706,
                     CheckCode::F831,
+                    CheckCode::F832,
                     CheckCode::F901,
                 ])
             }),