Move duplicate-value rule to flake8-bugbear (#4882)

crates/ruff/src/checkers/ast/mod.rs

@@ -3060,7 +3060,7 @@ where
             }
             Expr::Set(ast::ExprSet { elts, range: _ }) => {
                 if self.enabled(Rule::DuplicateValue) {
-                    pylint::rules::duplicate_value(self, elts);
+                    flake8_bugbear::rules::duplicate_value(self, elts);
                 }
             }
             Expr::Yield(_) => {

crates/ruff/src/codes.rs

@@ -197,7 +197,6 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Pylint, "R5501") => (RuleGroup::Unspecified, rules::pylint::rules::CollapsibleElseIf),
         (Pylint, "W0120") => (RuleGroup::Unspecified, rules::pylint::rules::UselessElseOnLoop),
         (Pylint, "W0129") => (RuleGroup::Unspecified, rules::pylint::rules::AssertOnStringLiteral),
-        (Pylint, "W0130") => (RuleGroup::Unspecified, rules::pylint::rules::DuplicateValue),
         (Pylint, "W0131") => (RuleGroup::Unspecified, rules::pylint::rules::NamedExprWithoutContext),
         (Pylint, "W0406") => (RuleGroup::Unspecified, rules::pylint::rules::ImportSelf),
         (Pylint, "W0602") => (RuleGroup::Unspecified, rules::pylint::rules::GlobalVariableNotAssigned),
@@ -249,6 +248,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Flake8Bugbear, "030") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::ExceptWithNonExceptionClasses),
         (Flake8Bugbear, "031") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::ReuseOfGroupbyGenerator),
         (Flake8Bugbear, "032") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::UnintentionalTypeAnnotation),
+        (Flake8Bugbear, "033") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::DuplicateValue),
         (Flake8Bugbear, "904") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::RaiseWithoutFromInsideExcept),
         (Flake8Bugbear, "905") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::ZipWithoutExplicitStrict),
 

crates/ruff/src/rule_redirects.rs

@@ -93,5 +93,6 @@ static REDIRECTS: Lazy<HashMap<&'static str, &'static str>> = Lazy::new(|| {
         // TODO(charlie): Remove by 2023-06-01.
         ("RUF004", "B026"),
         ("PIE802", "C419"),
+        ("PLW0130", "B033"),
     ])
 });

crates/ruff/src/rules/flake8_bugbear/mod.rs

@@ -14,39 +14,40 @@ mod tests {
     use crate::settings::Settings;
     use crate::test::test_path;
 
-    #[test_case(Rule::UnaryPrefixIncrement, Path::new("B002.py"))]
-    #[test_case(Rule::AssignmentToOsEnviron, Path::new("B003.py"))]
-    #[test_case(Rule::UnreliableCallableCheck, Path::new("B004.py"))]
-    #[test_case(Rule::StripWithMultiCharacters, Path::new("B005.py"))]
-    #[test_case(Rule::MutableArgumentDefault, Path::new("B006_B008.py"))]
-    #[test_case(Rule::UnusedLoopControlVariable, Path::new("B007.py"))]
-    #[test_case(Rule::FunctionCallInDefaultArgument, Path::new("B006_B008.py"))]
-    #[test_case(Rule::GetAttrWithConstant, Path::new("B009_B010.py"))]
-    #[test_case(Rule::SetAttrWithConstant, Path::new("B009_B010.py"))]
+    #[test_case(Rule::AbstractBaseClassWithoutAbstractMethod, Path::new("B024.py"))]
     #[test_case(Rule::AssertFalse, Path::new("B011.py"))]
-    #[test_case(Rule::JumpStatementInFinally, Path::new("B012.py"))]
-    #[test_case(Rule::RedundantTupleInExceptionHandler, Path::new("B013.py"))]
-    #[test_case(Rule::DuplicateHandlerException, Path::new("B014.py"))]
-    #[test_case(Rule::UselessComparison, Path::new("B015.py"))]
-    #[test_case(Rule::CannotRaiseLiteral, Path::new("B016.py"))]
     #[test_case(Rule::AssertRaisesException, Path::new("B017.py"))]
-    #[test_case(Rule::UselessExpression, Path::new("B018.py"))]
+    #[test_case(Rule::AssignmentToOsEnviron, Path::new("B003.py"))]
     #[test_case(Rule::CachedInstanceMethod, Path::new("B019.py"))]
-    #[test_case(Rule::LoopVariableOverridesIterator, Path::new("B020.py"))]
-    #[test_case(Rule::FStringDocstring, Path::new("B021.py"))]
-    #[test_case(Rule::UselessContextlibSuppress, Path::new("B022.py"))]
-    #[test_case(Rule::FunctionUsesLoopVariable, Path::new("B023.py"))]
-    #[test_case(Rule::AbstractBaseClassWithoutAbstractMethod, Path::new("B024.py"))]
+    #[test_case(Rule::CannotRaiseLiteral, Path::new("B016.py"))]
+    #[test_case(Rule::DuplicateHandlerException, Path::new("B014.py"))]
     #[test_case(Rule::DuplicateTryBlockException, Path::new("B025.py"))]
-    #[test_case(Rule::StarArgUnpackingAfterKeywordArg, Path::new("B026.py"))]
+    #[test_case(Rule::DuplicateValue, Path::new("B033.py"))]
     #[test_case(Rule::EmptyMethodWithoutAbstractDecorator, Path::new("B027.py"))]
     #[test_case(Rule::EmptyMethodWithoutAbstractDecorator, Path::new("B027.pyi"))]
-    #[test_case(Rule::NoExplicitStacklevel, Path::new("B028.py"))]
     #[test_case(Rule::ExceptWithEmptyTuple, Path::new("B029.py"))]
     #[test_case(Rule::ExceptWithNonExceptionClasses, Path::new("B030.py"))]
+    #[test_case(Rule::FStringDocstring, Path::new("B021.py"))]
+    #[test_case(Rule::FunctionCallInDefaultArgument, Path::new("B006_B008.py"))]
+    #[test_case(Rule::FunctionUsesLoopVariable, Path::new("B023.py"))]
+    #[test_case(Rule::GetAttrWithConstant, Path::new("B009_B010.py"))]
+    #[test_case(Rule::JumpStatementInFinally, Path::new("B012.py"))]
+    #[test_case(Rule::LoopVariableOverridesIterator, Path::new("B020.py"))]
+    #[test_case(Rule::MutableArgumentDefault, Path::new("B006_B008.py"))]
+    #[test_case(Rule::NoExplicitStacklevel, Path::new("B028.py"))]
+    #[test_case(Rule::RaiseWithoutFromInsideExcept, Path::new("B904.py"))]
+    #[test_case(Rule::RedundantTupleInExceptionHandler, Path::new("B013.py"))]
     #[test_case(Rule::ReuseOfGroupbyGenerator, Path::new("B031.py"))]
+    #[test_case(Rule::SetAttrWithConstant, Path::new("B009_B010.py"))]
+    #[test_case(Rule::StarArgUnpackingAfterKeywordArg, Path::new("B026.py"))]
+    #[test_case(Rule::StripWithMultiCharacters, Path::new("B005.py"))]
+    #[test_case(Rule::UnaryPrefixIncrement, Path::new("B002.py"))]
     #[test_case(Rule::UnintentionalTypeAnnotation, Path::new("B032.py"))]
-    #[test_case(Rule::RaiseWithoutFromInsideExcept, Path::new("B904.py"))]
+    #[test_case(Rule::UnreliableCallableCheck, Path::new("B004.py"))]
+    #[test_case(Rule::UnusedLoopControlVariable, Path::new("B007.py"))]
+    #[test_case(Rule::UselessComparison, Path::new("B015.py"))]
+    #[test_case(Rule::UselessContextlibSuppress, Path::new("B022.py"))]
+    #[test_case(Rule::UselessExpression, Path::new("B018.py"))]
     #[test_case(Rule::ZipWithoutExplicitStrict, Path::new("B905.py"))]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());

crates/ruff/src/rules/flake8_bugbear/rules/duplicate_value.rs

@@ -0,0 +1,57 @@
+use rustc_hash::FxHashSet;
+use rustpython_parser::ast::{self, Expr, Ranged};
+
+use ruff_diagnostics::{Diagnostic, Violation};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::comparable::ComparableExpr;
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for set literals that contain duplicate items.
+///
+/// ## Why is this bad?
+/// In Python, sets are unordered collections of unique elements. Including a
+/// duplicate item in a set literal is redundant, as the duplicate item will be
+/// replaced with a single item at runtime.
+///
+/// ## Example
+/// ```python
+/// {1, 2, 3, 1}
+/// ```
+///
+/// Use instead:
+/// ```python
+/// {1, 2, 3}
+/// ```
+#[violation]
+pub struct DuplicateValue {
+    value: String,
+}
+
+impl Violation for DuplicateValue {
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        let DuplicateValue { value } = self;
+        format!("Sets should not contain duplicate item `{value}`")
+    }
+}
+
+/// B033
+pub(crate) fn duplicate_value(checker: &mut Checker, elts: &Vec<Expr>) {
+    let mut seen_values: FxHashSet<ComparableExpr> = FxHashSet::default();
+    for elt in elts {
+        if let Expr::Constant(ast::ExprConstant { value, .. }) = elt {
+            let comparable_value: ComparableExpr = elt.into();
+
+            if !seen_values.insert(comparable_value) {
+                checker.diagnostics.push(Diagnostic::new(
+                    DuplicateValue {
+                        value: checker.generator().constant(value),
+                    },
+                    elt.range(),
+                ));
+            }
+        };
+    }
+}

crates/ruff/src/rules/flake8_bugbear/rules/mod.rs

@@ -10,6 +10,7 @@ pub(crate) use cannot_raise_literal::{cannot_raise_literal, CannotRaiseLiteral};
 pub(crate) use duplicate_exceptions::{
     duplicate_exceptions, DuplicateHandlerException, DuplicateTryBlockException,
 };
+pub(crate) use duplicate_value::{duplicate_value, DuplicateValue};
 pub(crate) use except_with_empty_tuple::{except_with_empty_tuple, ExceptWithEmptyTuple};
 pub(crate) use except_with_non_exception_classes::{
     except_with_non_exception_classes, ExceptWithNonExceptionClasses,
@@ -66,6 +67,7 @@ mod assignment_to_os_environ;
 mod cached_instance_method;
 mod cannot_raise_literal;
 mod duplicate_exceptions;
+mod duplicate_value;
 mod except_with_empty_tuple;
 mod except_with_non_exception_classes;
 mod f_string_docstring;

crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B033_B033.py.snap

@@ -0,0 +1,23 @@
+---
+source: crates/ruff/src/rules/flake8_bugbear/mod.rs
+---
+B033.py:4:35: B033 Sets should not contain duplicate item `"value1"`
+  |
+4 | # Errors.
+5 | ###
+6 | incorrect_set = {"value1", 23, 5, "value1"}
+  |                                   ^^^^^^^^ B033
+7 | incorrect_set = {1, 1}
+  |
+
+B033.py:5:21: B033 Sets should not contain duplicate item `1`
+  |
+5 | ###
+6 | incorrect_set = {"value1", 23, 5, "value1"}
+7 | incorrect_set = {1, 1}
+  |                     ^ B033
+8 | 
+9 | ###
+  |
+
+

crates/ruff/src/rules/pylint/mod.rs

@@ -54,7 +54,6 @@ mod tests {
     #[test_case(Rule::InvalidAllObject, Path::new("invalid_all_object.py"))]
     #[test_case(Rule::InvalidStrReturnType, Path::new("invalid_return_type_str.py"))]
     #[test_case(Rule::DuplicateBases, Path::new("duplicate_bases.py"))]
-    #[test_case(Rule::DuplicateValue, Path::new("duplicate_value.py"))]
     #[test_case(Rule::InvalidCharacterBackspace, Path::new("invalid_characters.py"))]
     #[test_case(Rule::InvalidCharacterEsc, Path::new("invalid_characters.py"))]
     #[test_case(Rule::InvalidCharacterNul, Path::new("invalid_characters.py"))]

crates/ruff/src/rules/pylint/rules/mod.rs

@@ -9,7 +9,6 @@ pub(crate) use compare_to_empty_string::{compare_to_empty_string, CompareToEmpty
 pub(crate) use comparison_of_constant::{comparison_of_constant, ComparisonOfConstant};
 pub(crate) use continue_in_finally::{continue_in_finally, ContinueInFinally};
 pub(crate) use duplicate_bases::{duplicate_bases, DuplicateBases};
-pub(crate) use duplicate_value::{duplicate_value, DuplicateValue};
 pub(crate) use global_statement::{global_statement, GlobalStatement};
 pub(crate) use global_variable_not_assigned::GlobalVariableNotAssigned;
 pub(crate) use import_self::{import_from_self, import_self, ImportSelf};
@@ -66,7 +65,6 @@ mod compare_to_empty_string;
 mod comparison_of_constant;
 mod continue_in_finally;
 mod duplicate_bases;
-mod duplicate_value;
 mod global_statement;
 mod global_variable_not_assigned;
 mod import_self;