Implement E741 (#137)

README.md

@@ -153,6 +153,7 @@ Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis F
 | E713 | NotInTest | Test for membership should be `not in` |
 | E714 | NotIsTest | Test for object identity should be `is not` |
 | E731 | DoNotAssignLambda | Do not assign a lambda expression, use a def |
+| E741 | AmbiguousVariableName | ambiguous variable name '...' |
 | E902 | IOError | No such file or directory: `...` |
 | F401 | UnusedImport | `...` imported but unused |
 | F403 | ImportStarUsage | Unable to detect undefined names |

examples/generate_rules_table.rs

@@ -3,6 +3,7 @@ use ruff::checks::{CheckKind, RejectedCmpop};
 
 fn main() {
     let mut check_kinds: Vec<CheckKind> = vec![
+        CheckKind::AmbiguousVariableName("...".to_string()),
         CheckKind::AssertTuple,
         CheckKind::DefaultExceptNotLast,
         CheckKind::DoNotAssignLambda,

resources/test/fixtures/E741.py

@@ -0,0 +1,72 @@
+from contextlib import contextmanager
+
+l = 0
+I = 0
+O = 0
+l: int = 0
+
+a, l = 0, 1
+[a, l] = 0, 1
+a, *l = 0, 1, 2
+a = l = 0
+
+o = 0
+i = 0
+
+for l in range(3):
+    pass
+
+
+for a, l in zip(range(3), range(3)):
+    pass
+
+
+def f1():
+    global l
+    l = 0
+
+
+def f2():
+    l = 0
+
+    def f3():
+        nonlocal l
+        l = 1
+
+    f3()
+    return l
+
+
+def f4(l, /, I):
+    return l, I, O
+
+
+def f5(l=0, *, I=1):
+    return l, I
+
+
+def f6(*l, **I):
+    return l, I
+
+
+@contextmanager
+def ctx1():
+    yield 0
+
+
+with ctx1() as l:
+    pass
+
+
+@contextmanager
+def ctx2():
+    yield 0, 1
+
+
+with ctx2() as (a, l):
+    pass
+
+try:
+    pass
+except ValueError as l:
+    pass

resources/test/fixtures/pyproject.toml

@@ -9,6 +9,7 @@ select = [
     "E713",
     "E714",
     "E731",
+    "E741",
     "E902",
     "F401",
     "F403",

src/ast/checks.rs

@@ -93,6 +93,22 @@ pub fn check_do_not_assign_lambda(value: &Expr, location: Location) -> Option<Ch
     }
 }
 
+fn is_ambiguous_name(name: &str) -> bool {
+    name == "l" || name == "I" || name == "O"
+}
+
+/// Check AmbiguousVariableName compliance.
+pub fn check_ambiguous_variable_name(name: &str, location: Location) -> Option<Check> {
+    if is_ambiguous_name(name) {
+        Some(Check::new(
+            CheckKind::AmbiguousVariableName(name.to_string()),
+            location,
+        ))
+    } else {
+        None
+    }
+}
+
 /// Check UselessObjectInheritance compliance.
 pub fn check_useless_object_inheritance(
     stmt: &Stmt,

src/check_ast.rs

@@ -158,6 +158,12 @@ where
                         }
                     }
                 }
+
+                if self.settings.select.contains(&CheckCode::E741) {
+                    self.checks.extend(names.iter().filter_map(|name| {
+                        checks::check_ambiguous_variable_name(name, stmt.location)
+                    }));
+                }
             }
             StmtKind::FunctionDef {
                 name,
@@ -507,9 +513,16 @@ where
                     }
                 }
             }
-            ExprKind::Name { ctx, .. } => match ctx {
+            ExprKind::Name { id, ctx } => match ctx {
                 ExprContext::Load => self.handle_node_load(expr),
                 ExprContext::Store => {
+                    if self.settings.select.contains(&CheckCode::E741) {
+                        if let Some(check) =
+                            checks::check_ambiguous_variable_name(id, expr.location)
+                        {
+                            self.checks.push(check);
+                        }
+                    }
                     let parent =
                         self.parents[*(self.parent_stack.last().expect("No parent found."))];
                     self.handle_node_store(expr, Some(parent));
@@ -746,6 +759,13 @@ where
         match &excepthandler.node {
             ExcepthandlerKind::ExceptHandler { name, .. } => match name {
                 Some(name) => {
+                    if self.settings.select.contains(&CheckCode::E741) {
+                        if let Some(check) =
+                            checks::check_ambiguous_variable_name(name, excepthandler.location)
+                        {
+                            self.checks.push(check);
+                        }
+                    }
                     let scope =
                         &self.scopes[*(self.scope_stack.last().expect("No current scope found."))];
                     if scope.values.contains_key(name) {
@@ -838,6 +858,13 @@ where
                 location: arg.location,
             },
         );
+
+        if self.settings.select.contains(&CheckCode::E741) {
+            if let Some(check) = checks::check_ambiguous_variable_name(&arg.node.arg, arg.location)
+            {
+                self.checks.push(check);
+            }
+        }
     }
 }
 

src/checks.rs

@@ -15,6 +15,7 @@ pub enum CheckCode {
     E713,
     E714,
     E731,
+    E741,
     E902,
     F401,
     F403,
@@ -50,6 +51,7 @@ impl FromStr for CheckCode {
             "E713" => Ok(CheckCode::E713),
             "E714" => Ok(CheckCode::E714),
             "E731" => Ok(CheckCode::E731),
+            "E741" => Ok(CheckCode::E741),
             "E902" => Ok(CheckCode::E902),
             "F401" => Ok(CheckCode::F401),
             "F403" => Ok(CheckCode::F403),
@@ -86,6 +88,7 @@ impl CheckCode {
             CheckCode::E713 => "E713",
             CheckCode::E714 => "E714",
             CheckCode::E731 => "E731",
+            CheckCode::E741 => "E741",
             CheckCode::E902 => "E902",
             CheckCode::F401 => "F401",
             CheckCode::F403 => "F403",
@@ -120,6 +123,7 @@ impl CheckCode {
             CheckCode::E713 => &LintSource::AST,
             CheckCode::E714 => &LintSource::AST,
             CheckCode::E731 => &LintSource::AST,
+            CheckCode::E741 => &LintSource::AST,
             CheckCode::E902 => &LintSource::FileSystem,
             CheckCode::F401 => &LintSource::AST,
             CheckCode::F403 => &LintSource::AST,
@@ -161,6 +165,7 @@ pub enum RejectedCmpop {
 #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)]
 pub enum CheckKind {
     AssertTuple,
+    AmbiguousVariableName(String),
     DefaultExceptNotLast,
     DoNotAssignLambda,
     DuplicateArgumentName,
@@ -195,6 +200,7 @@ impl CheckKind {
     pub fn name(&self) -> &'static str {
         match self {
             CheckKind::AssertTuple => "AssertTuple",
+            CheckKind::AmbiguousVariableName(_) => "AmbiguousVariableName",
             CheckKind::DefaultExceptNotLast => "DefaultExceptNotLast",
             CheckKind::DuplicateArgumentName => "DuplicateArgumentName",
             CheckKind::FStringMissingPlaceholders => "FStringMissingPlaceholders",
@@ -239,6 +245,7 @@ impl CheckKind {
             CheckKind::ImportStarUsage => &CheckCode::F403,
             CheckKind::LineTooLong => &CheckCode::E501,
             CheckKind::DoNotAssignLambda => &CheckCode::E731,
+            CheckKind::AmbiguousVariableName(_) => &CheckCode::E741,
             CheckKind::ModuleImportNotAtTopOfFile => &CheckCode::E402,
             CheckKind::MultiValueRepeatedKeyLiteral => &CheckCode::F601,
             CheckKind::MultiValueRepeatedKeyVariable(_) => &CheckCode::F602,
@@ -285,6 +292,9 @@ impl CheckKind {
             CheckKind::DoNotAssignLambda => {
                 "Do not assign a lambda expression, use a def".to_string()
             }
+            CheckKind::AmbiguousVariableName(name) => {
+                format!("ambiguous variable name '{}'", name)
+            }
             CheckKind::ModuleImportNotAtTopOfFile => {
                 "Module level import not at top of file".to_string()
             }
@@ -366,6 +376,7 @@ impl CheckKind {
             CheckKind::IfTuple => false,
             CheckKind::ImportStarUsage => false,
             CheckKind::DoNotAssignLambda => false,
+            CheckKind::AmbiguousVariableName(_) => false,
             CheckKind::LineTooLong => false,
             CheckKind::ModuleImportNotAtTopOfFile => false,
             CheckKind::MultiValueRepeatedKeyLiteral => false,

src/linter.rs

@@ -298,6 +298,149 @@ mod tests {
         Ok(())
     }
 
+    #[test]
+    fn e741() -> Result<()> {
+        let mut actual = check_path(
+            Path::new("./resources/test/fixtures/E741.py"),
+            &settings::Settings {
+                line_length: 88,
+                exclude: vec![],
+                select: BTreeSet::from([CheckCode::E741]),
+            },
+            &fixer::Mode::Generate,
+        )?;
+        actual.sort_by_key(|check| check.location);
+        let expected = vec![
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(3, 1),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("I".to_string()),
+                location: Location::new(4, 1),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("O".to_string()),
+                location: Location::new(5, 1),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(6, 1),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(8, 4),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(9, 5),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(10, 5),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(11, 5),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(16, 5),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(20, 8),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(25, 5),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(26, 5),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(30, 5),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(33, 9),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(34, 9),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(40, 8),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("I".to_string()),
+                location: Location::new(40, 14),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(44, 8),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("I".to_string()),
+                location: Location::new(44, 16),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(48, 9),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("I".to_string()),
+                location: Location::new(48, 14),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(57, 16),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(66, 20),
+                fix: None,
+            },
+            Check {
+                kind: CheckKind::AmbiguousVariableName("l".to_string()),
+                location: Location::new(71, 1),
+                fix: None,
+            },
+        ];
+
+        assert_eq!(actual.len(), expected.len());
+        for i in 0..actual.len() {
+            assert_eq!(actual[i], expected[i]);
+        }
+
+        Ok(())
+    }
+
     #[test]
     fn f401() -> Result<()> {
         let mut actual = check_path(

src/pyproject.rs

@@ -266,6 +266,7 @@ other-attribute = 1
                     CheckCode::E713,
                     CheckCode::E714,
                     CheckCode::E731,
+                    CheckCode::E741,
                     CheckCode::E902,
                     CheckCode::F401,
                     CheckCode::F403,

src/settings.rs

@@ -51,6 +51,7 @@ impl Settings {
                     CheckCode::E713,
                     CheckCode::E714,
                     CheckCode::E731,
+                    CheckCode::E741,
                     CheckCode::E902,
                     CheckCode::F401,
                     CheckCode::F403,