From c4565fe0f5847d7b51fad615ef3183bedd3ea74d Mon Sep 17 00:00:00 2001 From: Harutaka Kawamura Date: Mon, 12 Sep 2022 01:30:28 +0900 Subject: [PATCH] Implement E741 (#137) --- README.md | 1 + examples/generate_rules_table.rs | 1 + resources/test/fixtures/E741.py | 72 +++++++++++++ resources/test/fixtures/pyproject.toml | 1 + src/ast/checks.rs | 16 +++ src/check_ast.rs | 29 ++++- src/checks.rs | 11 ++ src/linter.rs | 143 +++++++++++++++++++++++++ src/pyproject.rs | 1 + src/settings.rs | 1 + 10 files changed, 275 insertions(+), 1 deletion(-) create mode 100644 resources/test/fixtures/E741.py diff --git a/README.md b/README.md index 7502b92ee1..54b1e0f859 100644 --- a/README.md +++ b/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 | diff --git a/examples/generate_rules_table.rs b/examples/generate_rules_table.rs index ac72924765..8be8319ef6 100644 --- a/examples/generate_rules_table.rs +++ b/examples/generate_rules_table.rs @@ -3,6 +3,7 @@ use ruff::checks::{CheckKind, RejectedCmpop}; fn main() { let mut check_kinds: Vec = vec![ + CheckKind::AmbiguousVariableName("...".to_string()), CheckKind::AssertTuple, CheckKind::DefaultExceptNotLast, CheckKind::DoNotAssignLambda, diff --git a/resources/test/fixtures/E741.py b/resources/test/fixtures/E741.py new file mode 100644 index 0000000000..d8fcf11922 --- /dev/null +++ b/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 diff --git a/resources/test/fixtures/pyproject.toml b/resources/test/fixtures/pyproject.toml index 6109a1843d..8b4e8780f5 100644 --- a/resources/test/fixtures/pyproject.toml +++ b/resources/test/fixtures/pyproject.toml @@ -9,6 +9,7 @@ select = [ "E713", "E714", "E731", + "E741", "E902", "F401", "F403", diff --git a/src/ast/checks.rs b/src/ast/checks.rs index 8df3edd794..503a6477f0 100644 --- a/src/ast/checks.rs +++ b/src/ast/checks.rs @@ -93,6 +93,22 @@ pub fn check_do_not_assign_lambda(value: &Expr, location: Location) -> Option bool { + name == "l" || name == "I" || name == "O" +} + +/// Check AmbiguousVariableName compliance. +pub fn check_ambiguous_variable_name(name: &str, location: Location) -> Option { + 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, diff --git a/src/check_ast.rs b/src/check_ast.rs index 7b966f524b..823f4a8b5e 100644 --- a/src/check_ast.rs +++ b/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); + } + } } } diff --git a/src/checks.rs b/src/checks.rs index 0b7e390184..4e1b62fc2e 100644 --- a/src/checks.rs +++ b/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, diff --git a/src/linter.rs b/src/linter.rs index b972bf1476..084f8456e8 100644 --- a/src/linter.rs +++ b/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( diff --git a/src/pyproject.rs b/src/pyproject.rs index d6ad365c5f..2e6ec7105c 100644 --- a/src/pyproject.rs +++ b/src/pyproject.rs @@ -266,6 +266,7 @@ other-attribute = 1 CheckCode::E713, CheckCode::E714, CheckCode::E731, + CheckCode::E741, CheckCode::E902, CheckCode::F401, CheckCode::F403, diff --git a/src/settings.rs b/src/settings.rs index e05f92c89e..1b71e1644c 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -51,6 +51,7 @@ impl Settings { CheckCode::E713, CheckCode::E714, CheckCode::E731, + CheckCode::E741, CheckCode::E902, CheckCode::F401, CheckCode::F403,