Implement F402 (#221)

README.md

@@ -151,7 +151,7 @@ ruff's goal is to achieve feature-parity with Flake8 when used (1) without any p
 stylistic checks; limiting to Python 3 obviates the need for certain compatibility checks.)
 
 Under those conditions, Flake8 implements about 60 rules, give or take. At time of writing, ruff
-implements 42 rules. (Note that these 42 rules likely cover a disproportionate share of errors:
+implements 43 rules. (Note that these 43 rules likely cover a disproportionate share of errors:
 unused imports, undefined variables, etc.)
 
 The unimplemented rules are tracked in #170, and include:
@@ -185,6 +185,7 @@ Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis F
 | E902 | IOError | No such file or directory: `...` |
 | E999 | SyntaxError | SyntaxError: ... |
 | F401 | UnusedImport | `...` imported but unused |
+| F402 | ImportShadowedByLoopVar | import '...' from line 1 shadowed by loop variable |
 | F403 | ImportStarUsage | `from ... import *` used; unable to detect undefined names |
 | F404 | LateFutureImport | from __future__ imports must occur at the beginning of the file |
 | F406 | ImportStarNotPermitted | `from ... import *` only allowed at module level |

resources/test/fixtures/F402.py

@@ -0,0 +1,9 @@
+import os
+import os.path as path
+
+
+for os in range(3):
+    pass
+
+for path in range(3):
+    pass

src/ast/types.rs

@@ -44,6 +44,7 @@ pub enum BindingKind {
     Argument,
     Assignment,
     Binding,
+    LoopVar,
     Builtin,
     ClassDefinition,
     Definition,

src/check_ast.rs

@@ -1110,12 +1110,24 @@ impl<'a> Checker<'a> {
         // TODO(charlie): Don't treat annotations as assignments if there is an existing value.
         let binding = match scope.values.get(&name) {
             None => binding,
-            Some(existing) => Binding {
+            Some(existing) => {
+                if self.settings.select.contains(&CheckCode::F402)
+                    && matches!(existing.kind, BindingKind::Importation(_))
+                    && matches!(binding.kind, BindingKind::LoopVar)
+                {
+                    self.checks.push(Check::new(
+                        CheckKind::ImportShadowedByLoopVar(name.clone(), existing.location.row()),
+                        binding.location,
+                    ));
+                }
+                Binding {
                     kind: binding.kind,
                     location: binding.location,
                     used: existing.used,
-            },
+                }
+            }
         };
+
         scope.values.insert(name, binding);
     }
 
@@ -1198,8 +1210,19 @@ impl<'a> Checker<'a> {
             if matches!(
                 parent.node,
                 StmtKind::For { .. } | StmtKind::AsyncFor { .. }
-            ) || operations::is_unpacking_assignment(parent)
-            {
+            ) {
+                self.add_binding(
+                    id.to_string(),
+                    Binding {
+                        kind: BindingKind::LoopVar,
+                        used: None,
+                        location: expr.location,
+                    },
+                );
+                return;
+            }
+
+            if operations::is_unpacking_assignment(parent) {
                 self.add_binding(
                     id.to_string(),
                     Binding {

src/checks.rs

@@ -6,7 +6,7 @@ use regex::Regex;
 use rustpython_parser::ast::Location;
 use serde::{Deserialize, Serialize};
 
-pub const ALL_CHECK_CODES: [CheckCode; 42] = [
+pub const ALL_CHECK_CODES: [CheckCode; 43] = [
     CheckCode::E402,
     CheckCode::E501,
     CheckCode::E711,
@@ -22,6 +22,7 @@ pub const ALL_CHECK_CODES: [CheckCode; 42] = [
     CheckCode::E902,
     CheckCode::E999,
     CheckCode::F401,
+    CheckCode::F402,
     CheckCode::F403,
     CheckCode::F404,
     CheckCode::F406,
@@ -68,6 +69,7 @@ pub enum CheckCode {
     E902,
     E999,
     F401,
+    F402,
     F403,
     F404,
     F406,
@@ -117,6 +119,7 @@ impl FromStr for CheckCode {
             "E902" => Ok(CheckCode::E902),
             "E999" => Ok(CheckCode::E999),
             "F401" => Ok(CheckCode::F401),
+            "F402" => Ok(CheckCode::F402),
             "F403" => Ok(CheckCode::F403),
             "F404" => Ok(CheckCode::F404),
             "F406" => Ok(CheckCode::F406),
@@ -167,6 +170,7 @@ impl CheckCode {
             CheckCode::E902 => "E902",
             CheckCode::E999 => "E999",
             CheckCode::F401 => "F401",
+            CheckCode::F402 => "F402",
             CheckCode::F403 => "F403",
             CheckCode::F404 => "F404",
             CheckCode::F406 => "F406",
@@ -224,6 +228,7 @@ impl CheckCode {
             CheckCode::F407 => CheckKind::FutureFeatureNotDefined("...".to_string()),
             CheckCode::E902 => CheckKind::IOError("...".to_string()),
             CheckCode::F634 => CheckKind::IfTuple,
+            CheckCode::F402 => CheckKind::ImportShadowedByLoopVar("...".to_string(), 1),
             CheckCode::F406 => CheckKind::ImportStarNotPermitted("...".to_string()),
             CheckCode::F403 => CheckKind::ImportStarUsage("...".to_string()),
             CheckCode::F633 => CheckKind::InvalidPrintSyntax,
@@ -285,6 +290,7 @@ pub enum CheckKind {
     FutureFeatureNotDefined(String),
     IOError(String),
     IfTuple,
+    ImportShadowedByLoopVar(String, usize),
     ImportStarNotPermitted(String),
     ImportStarUsage(String),
     InvalidPrintSyntax,
@@ -333,6 +339,7 @@ impl CheckKind {
             CheckKind::FutureFeatureNotDefined(_) => "FutureFeatureNotDefined",
             CheckKind::IOError(_) => "IOError",
             CheckKind::IfTuple => "IfTuple",
+            CheckKind::ImportShadowedByLoopVar(_, _) => "ImportShadowedByLoopVar",
             CheckKind::ImportStarNotPermitted(_) => "ImportStarNotPermitted",
             CheckKind::ImportStarUsage(_) => "ImportStarUsage",
             CheckKind::InvalidPrintSyntax => "InvalidPrintSyntax",
@@ -383,6 +390,7 @@ impl CheckKind {
             CheckKind::FutureFeatureNotDefined(_) => &CheckCode::F407,
             CheckKind::IOError(_) => &CheckCode::E902,
             CheckKind::IfTuple => &CheckCode::F634,
+            CheckKind::ImportShadowedByLoopVar(_, _) => &CheckCode::F402,
             CheckKind::ImportStarNotPermitted(_) => &CheckCode::F406,
             CheckKind::ImportStarUsage(_) => &CheckCode::F403,
             CheckKind::InvalidPrintSyntax => &CheckCode::F633,
@@ -452,6 +460,9 @@ impl CheckKind {
             CheckKind::IOError(message) => message.clone(),
             CheckKind::IfTuple => "If test is a tuple, which is always `True`".to_string(),
             CheckKind::InvalidPrintSyntax => "use of >> is invalid with print function".to_string(),
+            CheckKind::ImportShadowedByLoopVar(name, line) => {
+                format!("import '{name}' from line {line} shadowed by loop variable")
+            }
             CheckKind::ImportStarNotPermitted(name) => {
                 format!("`from {name} import *` only allowed at module level")
             }

src/linter.rs

@@ -332,6 +332,23 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn f402() -> Result<()> {
+        let mut checks = check_path(
+            Path::new("./resources/test/fixtures/F402.py"),
+            &settings::Settings {
+                line_length: 88,
+                exclude: vec![],
+                extend_exclude: vec![],
+                select: BTreeSet::from([CheckCode::F402]),
+            },
+            &fixer::Mode::Generate,
+        )?;
+        checks.sort_by_key(|check| check.location);
+        insta::assert_yaml_snapshot!(checks);
+        Ok(())
+    }
+
     #[test]
     fn f403() -> Result<()> {
         let mut checks = check_path(

src/snapshots/ruff__linter__tests__f402.snap

@@ -0,0 +1,21 @@
+---
+source: src/linter.rs
+expression: checks
+---
+- kind:
+    ImportShadowedByLoopVar:
+      - os
+      - 1
+  location:
+    row: 5
+    column: 5
+  fix: ~
+- kind:
+    ImportShadowedByLoopVar:
+      - path
+      - 2
+  location:
+    row: 8
+    column: 5
+  fix: ~
+