diff --git a/README.md b/README.md index 2a739f8183..ad9ae8aa80 100644 --- a/README.md +++ b/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 | diff --git a/resources/test/fixtures/F402.py b/resources/test/fixtures/F402.py new file mode 100644 index 0000000000..347a5aa0e7 --- /dev/null +++ b/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 diff --git a/src/ast/types.rs b/src/ast/types.rs index fb292e5b65..e02d30e2c0 100644 --- a/src/ast/types.rs +++ b/src/ast/types.rs @@ -44,6 +44,7 @@ pub enum BindingKind { Argument, Assignment, Binding, + LoopVar, Builtin, ClassDefinition, Definition, diff --git a/src/check_ast.rs b/src/check_ast.rs index 13a8ec9dad..e5f192036c 100644 --- a/src/check_ast.rs +++ b/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 { - kind: binding.kind, - location: binding.location, - used: existing.used, - }, + 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 { diff --git a/src/checks.rs b/src/checks.rs index d463f05249..881f118cd6 100644 --- a/src/checks.rs +++ b/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") } diff --git a/src/linter.rs b/src/linter.rs index a25b0d5459..3a8a948d0d 100644 --- a/src/linter.rs +++ b/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( diff --git a/src/snapshots/ruff__linter__tests__f402.snap b/src/snapshots/ruff__linter__tests__f402.snap new file mode 100644 index 0000000000..9e03a8c342 --- /dev/null +++ b/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: ~ +