From bcc15b0be12061808cf445f8af77d55680c32308 Mon Sep 17 00:00:00 2001 From: yt2b Date: Sun, 24 Mar 2024 07:29:49 +0900 Subject: [PATCH 1/5] Add function --- .../flake8_bugbear/rules/reuse_of_groupby_generator.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs index 2b39e349af25a..e5e59a7611815 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs @@ -100,6 +100,16 @@ impl<'a> GroupNameFinder<'a> { self.usage_count += value; } } + + /// Reset the usage count for the group name by the given value. + /// This function is called when there is a continue, break, return statement. + fn reset_usage_count(&mut self) { + if let Some(last) = self.counter_stack.last_mut() { + *last.last_mut().unwrap() = 0; + } else { + self.usage_count = 0; + } + } } impl<'a> Visitor<'a> for GroupNameFinder<'a> { From e7488975d9c66ec3e8b29d793a0d11442de1ff7a Mon Sep 17 00:00:00 2001 From: yt2b Date: Sun, 24 Mar 2024 07:34:54 +0900 Subject: [PATCH 2/5] Reset usage count --- .../flake8_bugbear/rules/reuse_of_groupby_generator.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs index e5e59a7611815..13348d4408bbd 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs @@ -207,6 +207,15 @@ impl<'a> Visitor<'a> for GroupNameFinder<'a> { self.visit_expr(expr); } } + Stmt::Break(_) | Stmt::Continue(_) => { + self.reset_usage_count(); + } + Stmt::Return(ast::StmtReturn { value, range: _ }) => { + if let Some(expr) = value { + self.visit_expr(expr); + } + self.reset_usage_count(); + } _ => visitor::walk_stmt(self, stmt), } } From 699586d92c9a874ba91cd0a5bba7be13354ff92c Mon Sep 17 00:00:00 2001 From: yt2b Date: Sun, 24 Mar 2024 07:39:27 +0900 Subject: [PATCH 3/5] Add test case --- .../test/fixtures/flake8_bugbear/B031.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B031.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B031.py index 61f62c4b67d8b..5a55fce5c3647 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B031.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B031.py @@ -174,6 +174,24 @@ def collect_shop_items(shopper, items): collect_shop_items("Jane", group[1]) collect_shop_items("Joe", group[1]) +# Shouldn't trigger the warning when there is a continue, break statement. +for _section, section_items in groupby(items, key=lambda p: p[1]): + if _section == "greens": + collect_shop_items(shopper, section_items) + continue + elif _section == "frozen items": + collect_shop_items(shopper, section_items) + break + collect_shop_items(shopper, section_items) + +# Shouldn't trigger the warning when there is a return statement. +for _section, section_items in groupby(items, key=lambda p: p[1]): + if _section == "greens": + collect_shop_items(shopper, section_items) + return + elif _section == "frozen items": + return section_items + collect_shop_items(shopper, section_items) # Let's redefine the `groupby` function to make sure we pick up the correct one. # NOTE: This should always be at the end of the file. From 43108530bc95f1677dd96b10ea490fcfb6ae106c Mon Sep 17 00:00:00 2001 From: yt2b Date: Sun, 24 Mar 2024 08:15:54 +0900 Subject: [PATCH 4/5] Change the order --- .../rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs index 13348d4408bbd..af074bede057f 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs @@ -207,7 +207,7 @@ impl<'a> Visitor<'a> for GroupNameFinder<'a> { self.visit_expr(expr); } } - Stmt::Break(_) | Stmt::Continue(_) => { + Stmt::Continue(_) | Stmt::Break(_) => { self.reset_usage_count(); } Stmt::Return(ast::StmtReturn { value, range: _ }) => { From 00449cf972a7c3bb1d43e830f96c6435e014c766 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 24 Mar 2024 20:31:13 -0400 Subject: [PATCH 5/5] Add more tests --- .../test/fixtures/flake8_bugbear/B031.py | 25 +++++++++++++++++ .../rules/reuse_of_groupby_generator.rs | 2 +- ...__flake8_bugbear__tests__B031_B031.py.snap | 27 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B031.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B031.py index 5a55fce5c3647..412b890ef7b69 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B031.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B031.py @@ -193,6 +193,31 @@ def collect_shop_items(shopper, items): return section_items collect_shop_items(shopper, section_items) +# Should trigger the warning for duplicate access, even if is a return statement after. +for _section, section_items in groupby(items, key=lambda p: p[1]): + if _section == "greens": + collect_shop_items(shopper, section_items) + collect_shop_items(shopper, section_items) + return + +# Should trigger the warning for duplicate access, even if is a return in another branch. +for _section, section_items in groupby(items, key=lambda p: p[1]): + if _section == "greens": + collect_shop_items(shopper, section_items) + return + elif _section == "frozen items": + collect_shop_items(shopper, section_items) + collect_shop_items(shopper, section_items) + +# Should trigger, since only one branch has a return statement. +for _section, section_items in groupby(items, key=lambda p: p[1]): + if _section == "greens": + collect_shop_items(shopper, section_items) + return + elif _section == "frozen items": + collect_shop_items(shopper, section_items) + collect_shop_items(shopper, section_items) # B031 + # Let's redefine the `groupby` function to make sure we pick up the correct one. # NOTE: This should always be at the end of the file. def groupby(data, key=None): diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs index af074bede057f..b495276c02449 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/reuse_of_groupby_generator.rs @@ -102,7 +102,7 @@ impl<'a> GroupNameFinder<'a> { } /// Reset the usage count for the group name by the given value. - /// This function is called when there is a continue, break, return statement. + /// This function is called when there is a `continue`, `break`, or `return` statement. fn reset_usage_count(&mut self) { if let Some(last) = self.counter_stack.last_mut() { *last.last_mut().unwrap() = 0; diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B031_B031.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B031_B031.py.snap index b2de85a2f4714..075bffbe6cae7 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B031_B031.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B031_B031.py.snap @@ -195,4 +195,31 @@ B031.py:144:33: B031 Using the generator returned from `itertools.groupby()` mor 146 | for group in groupby(items, key=lambda p: p[1]): | +B031.py:200:37: B031 Using the generator returned from `itertools.groupby()` more than once will do nothing on the second usage + | +198 | if _section == "greens": +199 | collect_shop_items(shopper, section_items) +200 | collect_shop_items(shopper, section_items) + | ^^^^^^^^^^^^^ B031 +201 | return + | + +B031.py:210:37: B031 Using the generator returned from `itertools.groupby()` more than once will do nothing on the second usage + | +208 | elif _section == "frozen items": +209 | collect_shop_items(shopper, section_items) +210 | collect_shop_items(shopper, section_items) + | ^^^^^^^^^^^^^ B031 +211 | +212 | # Should trigger, since only one branch has a return statement. + | +B031.py:219:33: B031 Using the generator returned from `itertools.groupby()` more than once will do nothing on the second usage + | +217 | elif _section == "frozen items": +218 | collect_shop_items(shopper, section_items) +219 | collect_shop_items(shopper, section_items) # B031 + | ^^^^^^^^^^^^^ B031 +220 | +221 | # Let's redefine the `groupby` function to make sure we pick up the correct one. + |