From 7d04aca9f228d2a7b63231e70da21838aa9362f6 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 5 Jun 2023 17:43:47 -0400 Subject: [PATCH] Move duplicate-value rule to flake8-bugbear (#4882) --- .../B033.py} | 0 crates/ruff/src/checkers/ast/mod.rs | 2 +- crates/ruff/src/codes.rs | 2 +- crates/ruff/src/rule_redirects.rs | 1 + crates/ruff/src/rules/flake8_bugbear/mod.rs | 47 +++++++-------- .../flake8_bugbear/rules/duplicate_value.rs | 57 +++++++++++++++++++ .../src/rules/flake8_bugbear/rules/mod.rs | 2 + ...__flake8_bugbear__tests__B033_B033.py.snap | 23 ++++++++ crates/ruff/src/rules/pylint/mod.rs | 1 - crates/ruff/src/rules/pylint/rules/mod.rs | 2 - ...nt__tests__PLW0130_duplicate_value.py.snap | 23 -------- ruff.schema.json | 2 +- 12 files changed, 110 insertions(+), 52 deletions(-) rename crates/ruff/resources/test/fixtures/{pylint/duplicate_value.py => flake8_bugbear/B033.py} (100%) create mode 100644 crates/ruff/src/rules/flake8_bugbear/rules/duplicate_value.rs create mode 100644 crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B033_B033.py.snap delete mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW0130_duplicate_value.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/duplicate_value.py b/crates/ruff/resources/test/fixtures/flake8_bugbear/B033.py similarity index 100% rename from crates/ruff/resources/test/fixtures/pylint/duplicate_value.py rename to crates/ruff/resources/test/fixtures/flake8_bugbear/B033.py diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index f5e130f11a49b4..24a0c8b177e396 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/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(_) => { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 0f5effd416d7ed..d0eb6f83f5180f 100644 --- a/crates/ruff/src/codes.rs +++ b/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), diff --git a/crates/ruff/src/rule_redirects.rs b/crates/ruff/src/rule_redirects.rs index 4749b64ccacb4b..ebf63dddd036c7 100644 --- a/crates/ruff/src/rule_redirects.rs +++ b/crates/ruff/src/rule_redirects.rs @@ -93,5 +93,6 @@ static REDIRECTS: Lazy> = Lazy::new(|| { // TODO(charlie): Remove by 2023-06-01. ("RUF004", "B026"), ("PIE802", "C419"), + ("PLW0130", "B033"), ]) }); diff --git a/crates/ruff/src/rules/flake8_bugbear/mod.rs b/crates/ruff/src/rules/flake8_bugbear/mod.rs index 896b977adfd8c7..2bf567630621ca 100644 --- a/crates/ruff/src/rules/flake8_bugbear/mod.rs +++ b/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()); diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/duplicate_value.rs b/crates/ruff/src/rules/flake8_bugbear/rules/duplicate_value.rs new file mode 100644 index 00000000000000..e66da4623c2836 --- /dev/null +++ b/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) { + let mut seen_values: FxHashSet = 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(), + )); + } + }; + } +} diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs b/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs index 77714559fb629f..f350160a579449 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs +++ b/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; diff --git a/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B033_B033.py.snap b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B033_B033.py.snap new file mode 100644 index 00000000000000..a71540f42f781c --- /dev/null +++ b/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 | ### + | + + diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index fc7fb547addcd2..c7536d1922faaa 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/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"))] diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index a8c45ec0c8bd16..70f088b3135a90 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/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; diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW0130_duplicate_value.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW0130_duplicate_value.py.snap deleted file mode 100644 index 6fdf6149d72020..00000000000000 --- a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW0130_duplicate_value.py.snap +++ /dev/null @@ -1,23 +0,0 @@ ---- -source: crates/ruff/src/rules/pylint/mod.rs ---- -duplicate_value.py:4:35: PLW0130 Duplicate value `"value1"` in set - | -4 | # Errors. -5 | ### -6 | incorrect_set = {"value1", 23, 5, "value1"} - | ^^^^^^^^ PLW0130 -7 | incorrect_set = {1, 1} - | - -duplicate_value.py:5:21: PLW0130 Duplicate value `1` in set - | -5 | ### -6 | incorrect_set = {"value1", 23, 5, "value1"} -7 | incorrect_set = {1, 1} - | ^ PLW0130 -8 | -9 | ### - | - - diff --git a/ruff.schema.json b/ruff.schema.json index f99ffe9dee8154..29b6d3f32ed89a 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1639,6 +1639,7 @@ "B030", "B031", "B032", + "B033", "B9", "B90", "B904", @@ -2137,7 +2138,6 @@ "PLW0120", "PLW0129", "PLW013", - "PLW0130", "PLW0131", "PLW04", "PLW040",