From fac20fa8e6a5a26f84459821b040f8daf31ffe15 Mon Sep 17 00:00:00 2001 From: harupy Date: Sun, 25 Dec 2022 01:01:40 +0900 Subject: [PATCH 1/7] Fix B025 location --- .../plugins/duplicate_exceptions.rs | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/flake8_bugbear/plugins/duplicate_exceptions.rs b/src/flake8_bugbear/plugins/duplicate_exceptions.rs index b12a25bdd30c8..35dcf14489b98 100644 --- a/src/flake8_bugbear/plugins/duplicate_exceptions.rs +++ b/src/flake8_bugbear/plugins/duplicate_exceptions.rs @@ -1,5 +1,5 @@ use itertools::Itertools; -use rustc_hash::FxHashSet; +use rustc_hash::{FxHashMap, FxHashSet}; use rustpython_ast::{ Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind, Location, Stmt, }; @@ -26,17 +26,17 @@ fn duplicate_handler_exceptions<'a>( checker: &mut Checker, expr: &'a Expr, elts: &'a [Expr], -) -> FxHashSet> { - let mut seen: FxHashSet> = FxHashSet::default(); +) -> FxHashMap, Vec<&'a Expr>> { + let mut seen: FxHashMap, Vec<&Expr>> = FxHashMap::default(); let mut duplicates: FxHashSet> = FxHashSet::default(); let mut unique_elts: Vec<&Expr> = Vec::default(); for type_ in elts { let call_path = helpers::collect_call_paths(type_); if !call_path.is_empty() { - if seen.contains(&call_path) { + if seen.contains_key(&call_path) { duplicates.insert(call_path); } else { - seen.insert(call_path); + seen.entry(call_path).or_default().push(type_); unique_elts.push(type_); } } @@ -79,7 +79,7 @@ fn duplicate_handler_exceptions<'a>( pub fn duplicate_exceptions(checker: &mut Checker, stmt: &Stmt, handlers: &[Excepthandler]) { let mut seen: FxHashSet> = FxHashSet::default(); - let mut duplicates: FxHashSet> = FxHashSet::default(); + let mut duplicates: FxHashMap, Vec<&Expr>> = FxHashMap::default(); for handler in handlers { let ExcepthandlerKind::ExceptHandler { type_: Some(type_), .. } = &handler.node else { continue; @@ -89,16 +89,16 @@ pub fn duplicate_exceptions(checker: &mut Checker, stmt: &Stmt, handlers: &[Exce let call_path = helpers::collect_call_paths(type_); if !call_path.is_empty() { if seen.contains(&call_path) { - duplicates.insert(call_path); + duplicates.entry(call_path).or_default().push(type_) } else { seen.insert(call_path); } } } ExprKind::Tuple { elts, .. } => { - for name in duplicate_handler_exceptions(checker, type_, elts) { + for (name, exprs) in duplicate_handler_exceptions(checker, type_, elts) { if seen.contains(&name) { - duplicates.insert(name); + duplicates.entry(name).or_default().extend(exprs); } else { seen.insert(name); } @@ -109,11 +109,13 @@ pub fn duplicate_exceptions(checker: &mut Checker, stmt: &Stmt, handlers: &[Exce } if checker.settings.enabled.contains(&CheckCode::B025) { - for duplicate in duplicates.into_iter().sorted() { - checker.add_check(Check::new( - CheckKind::DuplicateTryBlockException(duplicate.join(".")), - Range::from_located(stmt), - )); + for (name, exprs) in duplicates { + for expr in exprs { + checker.add_check(Check::new( + CheckKind::DuplicateTryBlockException(name.join(".")), + Range::from_located(expr), + )); + } } } } From d343977126045a9cad1b5264620c7759f1e6e3fc Mon Sep 17 00:00:00 2001 From: harupy Date: Sun, 25 Dec 2022 01:23:27 +0900 Subject: [PATCH 2/7] Improve B025 location --- src/checkers/ast.rs | 2 +- .../plugins/duplicate_exceptions.rs | 8 ++--- ...__flake8_bugbear__tests__B025_B025.py.snap | 36 +++++++++---------- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index fbc521a97adb8..95ce7f33119be 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -1151,7 +1151,7 @@ where if self.settings.enabled.contains(&CheckCode::B014) || self.settings.enabled.contains(&CheckCode::B025) { - flake8_bugbear::plugins::duplicate_exceptions(self, stmt, handlers); + flake8_bugbear::plugins::duplicate_exceptions(self, handlers); } if self.settings.enabled.contains(&CheckCode::B013) { flake8_bugbear::plugins::redundant_tuple_in_exception_handler(self, handlers); diff --git a/src/flake8_bugbear/plugins/duplicate_exceptions.rs b/src/flake8_bugbear/plugins/duplicate_exceptions.rs index 35dcf14489b98..c51c6ba1d94c3 100644 --- a/src/flake8_bugbear/plugins/duplicate_exceptions.rs +++ b/src/flake8_bugbear/plugins/duplicate_exceptions.rs @@ -1,8 +1,6 @@ use itertools::Itertools; use rustc_hash::{FxHashMap, FxHashSet}; -use rustpython_ast::{ - Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind, Location, Stmt, -}; +use rustpython_ast::{Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind, Location}; use crate::ast::helpers; use crate::ast::types::Range; @@ -77,7 +75,7 @@ fn duplicate_handler_exceptions<'a>( seen } -pub fn duplicate_exceptions(checker: &mut Checker, stmt: &Stmt, handlers: &[Excepthandler]) { +pub fn duplicate_exceptions(checker: &mut Checker, handlers: &[Excepthandler]) { let mut seen: FxHashSet> = FxHashSet::default(); let mut duplicates: FxHashMap, Vec<&Expr>> = FxHashMap::default(); for handler in handlers { @@ -89,7 +87,7 @@ pub fn duplicate_exceptions(checker: &mut Checker, stmt: &Stmt, handlers: &[Exce let call_path = helpers::collect_call_paths(type_); if !call_path.is_empty() { if seen.contains(&call_path) { - duplicates.entry(call_path).or_default().push(type_) + duplicates.entry(call_path).or_default().push(type_); } else { seen.insert(call_path); } diff --git a/src/flake8_bugbear/snapshots/ruff__flake8_bugbear__tests__B025_B025.py.snap b/src/flake8_bugbear/snapshots/ruff__flake8_bugbear__tests__B025_B025.py.snap index cdd7648933d1f..8b074c180be6b 100644 --- a/src/flake8_bugbear/snapshots/ruff__flake8_bugbear__tests__B025_B025.py.snap +++ b/src/flake8_bugbear/snapshots/ruff__flake8_bugbear__tests__B025_B025.py.snap @@ -5,37 +5,37 @@ expression: checks - kind: DuplicateTryBlockException: ValueError location: - row: 15 - column: 0 + row: 19 + column: 7 end_location: - row: 20 - column: 9 + row: 19 + column: 17 fix: ~ - kind: DuplicateTryBlockException: pickle.PickleError location: - row: 22 - column: 0 + row: 28 + column: 7 end_location: - row: 29 - column: 9 + row: 28 + column: 25 fix: ~ - kind: - DuplicateTryBlockException: TypeError + DuplicateTryBlockException: ValueError location: - row: 31 - column: 0 + row: 35 + column: 7 end_location: - row: 38 - column: 9 + row: 35 + column: 17 fix: ~ - kind: - DuplicateTryBlockException: ValueError + DuplicateTryBlockException: TypeError location: - row: 31 - column: 0 + row: 37 + column: 17 end_location: - row: 38 - column: 9 + row: 37 + column: 26 fix: ~ From 2f1d5b73c7622e5b86f49ca847a3646b760134fc Mon Sep 17 00:00:00 2001 From: harupy Date: Sun, 25 Dec 2022 01:38:36 +0900 Subject: [PATCH 3/7] Fix --- src/flake8_bugbear/plugins/duplicate_exceptions.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/flake8_bugbear/plugins/duplicate_exceptions.rs b/src/flake8_bugbear/plugins/duplicate_exceptions.rs index c51c6ba1d94c3..db30a8edd571a 100644 --- a/src/flake8_bugbear/plugins/duplicate_exceptions.rs +++ b/src/flake8_bugbear/plugins/duplicate_exceptions.rs @@ -24,8 +24,8 @@ fn duplicate_handler_exceptions<'a>( checker: &mut Checker, expr: &'a Expr, elts: &'a [Expr], -) -> FxHashMap, Vec<&'a Expr>> { - let mut seen: FxHashMap, Vec<&Expr>> = FxHashMap::default(); +) -> FxHashMap, &'a Expr> { + let mut seen: FxHashMap, &Expr> = FxHashMap::default(); let mut duplicates: FxHashSet> = FxHashSet::default(); let mut unique_elts: Vec<&Expr> = Vec::default(); for type_ in elts { @@ -34,7 +34,7 @@ fn duplicate_handler_exceptions<'a>( if seen.contains_key(&call_path) { duplicates.insert(call_path); } else { - seen.entry(call_path).or_default().push(type_); + seen.insert(call_path, type_); unique_elts.push(type_); } } @@ -94,9 +94,9 @@ pub fn duplicate_exceptions(checker: &mut Checker, handlers: &[Excepthandler]) { } } ExprKind::Tuple { elts, .. } => { - for (name, exprs) in duplicate_handler_exceptions(checker, type_, elts) { + for (name, expr) in duplicate_handler_exceptions(checker, type_, elts) { if seen.contains(&name) { - duplicates.entry(name).or_default().extend(exprs); + duplicates.entry(name).or_default().push(expr); } else { seen.insert(name); } From 65b86ee06aff86787118d5361ab63c281340fb94 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 24 Dec 2022 12:13:53 -0500 Subject: [PATCH 4/7] Foo --- .../plugins/duplicate_exceptions.rs | 39 +++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/flake8_bugbear/plugins/duplicate_exceptions.rs b/src/flake8_bugbear/plugins/duplicate_exceptions.rs index db30a8edd571a..992a25f4ee245 100644 --- a/src/flake8_bugbear/plugins/duplicate_exceptions.rs +++ b/src/flake8_bugbear/plugins/duplicate_exceptions.rs @@ -1,6 +1,7 @@ use itertools::Itertools; use rustc_hash::{FxHashMap, FxHashSet}; use rustpython_ast::{Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind, Location}; +use std::collections::{HashMap, HashSet}; use crate::ast::helpers; use crate::ast::types::Range; @@ -20,6 +21,22 @@ fn type_pattern(elts: Vec<&Expr>) -> Expr { ) } +/// Given a list of imports (like `foo.bar.baz`), ensure that there's only one import per module +/// (i.e., if we see `foo.bar.baz`, flag `foo.bop` as a duplicate). +fn detect_duplicates(members: &[&str]) { + let mut seen: HashMap = HashMap::default(); + let mut duplicates: HashSet = HashSet::default(); + for member in members { + let module = member.split('.').next().unwrap().to_string(); + if seen.contains_key(&module) { + duplicates.insert(module); + } else { + seen.insert(module, member); + } + } + // Do something with `seen`, etc. +} + fn duplicate_handler_exceptions<'a>( checker: &mut Checker, expr: &'a Expr, @@ -28,17 +45,17 @@ fn duplicate_handler_exceptions<'a>( let mut seen: FxHashMap, &Expr> = FxHashMap::default(); let mut duplicates: FxHashSet> = FxHashSet::default(); let mut unique_elts: Vec<&Expr> = Vec::default(); - for type_ in elts { - let call_path = helpers::collect_call_paths(type_); - if !call_path.is_empty() { - if seen.contains_key(&call_path) { - duplicates.insert(call_path); - } else { - seen.insert(call_path, type_); - unique_elts.push(type_); - } - } - } + // for type_ in elts { + // let call_path = helpers::collect_call_paths(type_); + // if !call_path.is_empty() { + // if seen.contains_key(&call_path) { + // duplicates.insert(call_path); + // } else { + // seen.insert(call_path, type_); + // unique_elts.push(type_); + // } + // } + // } if checker.settings.enabled.contains(&CheckCode::B014) { // TODO(charlie): Handle "BaseException" and redundant exception aliases. From fd5f9c547ed745605b6fdb3dab5b9ab19f56a661 Mon Sep 17 00:00:00 2001 From: harupy Date: Sun, 25 Dec 2022 02:18:03 +0900 Subject: [PATCH 5/7] Use entry --- src/flake8_bugbear/plugins/duplicate_exceptions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/flake8_bugbear/plugins/duplicate_exceptions.rs b/src/flake8_bugbear/plugins/duplicate_exceptions.rs index db30a8edd571a..b5aecb5caf087 100644 --- a/src/flake8_bugbear/plugins/duplicate_exceptions.rs +++ b/src/flake8_bugbear/plugins/duplicate_exceptions.rs @@ -34,7 +34,7 @@ fn duplicate_handler_exceptions<'a>( if seen.contains_key(&call_path) { duplicates.insert(call_path); } else { - seen.insert(call_path, type_); + seen.entry(call_path).or_insert(type_); unique_elts.push(type_); } } From e7a9c94646b9f5288d7e16e4cf9e567136e829ba Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 24 Dec 2022 12:20:37 -0500 Subject: [PATCH 6/7] Ignore false Clippy warning --- .../plugins/duplicate_exceptions.rs | 40 ++++++------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/src/flake8_bugbear/plugins/duplicate_exceptions.rs b/src/flake8_bugbear/plugins/duplicate_exceptions.rs index 992a25f4ee245..bd18e465afa35 100644 --- a/src/flake8_bugbear/plugins/duplicate_exceptions.rs +++ b/src/flake8_bugbear/plugins/duplicate_exceptions.rs @@ -1,7 +1,6 @@ use itertools::Itertools; use rustc_hash::{FxHashMap, FxHashSet}; use rustpython_ast::{Excepthandler, ExcepthandlerKind, Expr, ExprContext, ExprKind, Location}; -use std::collections::{HashMap, HashSet}; use crate::ast::helpers; use crate::ast::types::Range; @@ -21,22 +20,7 @@ fn type_pattern(elts: Vec<&Expr>) -> Expr { ) } -/// Given a list of imports (like `foo.bar.baz`), ensure that there's only one import per module -/// (i.e., if we see `foo.bar.baz`, flag `foo.bop` as a duplicate). -fn detect_duplicates(members: &[&str]) { - let mut seen: HashMap = HashMap::default(); - let mut duplicates: HashSet = HashSet::default(); - for member in members { - let module = member.split('.').next().unwrap().to_string(); - if seen.contains_key(&module) { - duplicates.insert(module); - } else { - seen.insert(module, member); - } - } - // Do something with `seen`, etc. -} - +#[allow(clippy::map_entry)] fn duplicate_handler_exceptions<'a>( checker: &mut Checker, expr: &'a Expr, @@ -45,17 +29,17 @@ fn duplicate_handler_exceptions<'a>( let mut seen: FxHashMap, &Expr> = FxHashMap::default(); let mut duplicates: FxHashSet> = FxHashSet::default(); let mut unique_elts: Vec<&Expr> = Vec::default(); - // for type_ in elts { - // let call_path = helpers::collect_call_paths(type_); - // if !call_path.is_empty() { - // if seen.contains_key(&call_path) { - // duplicates.insert(call_path); - // } else { - // seen.insert(call_path, type_); - // unique_elts.push(type_); - // } - // } - // } + for type_ in elts { + let call_path = helpers::collect_call_paths(type_); + if !call_path.is_empty() { + if seen.contains_key(&call_path) { + duplicates.insert(call_path); + } else { + seen.insert(call_path, type_); + unique_elts.push(type_); + } + } + } if checker.settings.enabled.contains(&CheckCode::B014) { // TODO(charlie): Handle "BaseException" and redundant exception aliases. From 2aa4b30a0171ed1e577df4ef5171ecb635ccfe74 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 24 Dec 2022 12:21:11 -0500 Subject: [PATCH 7/7] Remove allow --- src/flake8_bugbear/plugins/duplicate_exceptions.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/flake8_bugbear/plugins/duplicate_exceptions.rs b/src/flake8_bugbear/plugins/duplicate_exceptions.rs index 9c0dca0012f37..b5aecb5caf087 100644 --- a/src/flake8_bugbear/plugins/duplicate_exceptions.rs +++ b/src/flake8_bugbear/plugins/duplicate_exceptions.rs @@ -20,7 +20,6 @@ fn type_pattern(elts: Vec<&Expr>) -> Expr { ) } -#[allow(clippy::map_entry)] fn duplicate_handler_exceptions<'a>( checker: &mut Checker, expr: &'a Expr,