From 9f486fa841f4c9f9ce4e09ec65df69dea87e76e2 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 10 Jul 2023 23:52:55 -0400 Subject: [PATCH] [`flake8-bugbear`] Implement `re-sub-positional-args` (`B034`) (#5669) ## Summary Needed to do some coding to end the day. Closes #5665. --- .../test/fixtures/flake8_bugbear/B034.py | 27 +++++ crates/ruff/src/checkers/ast/mod.rs | 17 ++- crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/flake8_bugbear/mod.rs | 3 +- .../src/rules/flake8_bugbear/rules/mod.rs | 2 + .../rules/re_sub_positional_args.rs | 112 ++++++++++++++++++ ...__flake8_bugbear__tests__B034_B034.py.snap | 102 ++++++++++++++++ .../rules/type_name_incorrect_variance.rs | 22 ++-- ruff.schema.json | 1 + 9 files changed, 269 insertions(+), 18 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/flake8_bugbear/B034.py create mode 100644 crates/ruff/src/rules/flake8_bugbear/rules/re_sub_positional_args.rs create mode 100644 crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B034_B034.py.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_bugbear/B034.py b/crates/ruff/resources/test/fixtures/flake8_bugbear/B034.py new file mode 100644 index 0000000000000..1236259c6b57d --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_bugbear/B034.py @@ -0,0 +1,27 @@ +import re +from re import sub + +# B034 +re.sub("a", "b", "aaa", re.IGNORECASE) +re.sub("a", "b", "aaa", 5) +re.sub("a", "b", "aaa", 5, re.IGNORECASE) +re.subn("a", "b", "aaa", re.IGNORECASE) +re.subn("a", "b", "aaa", 5) +re.subn("a", "b", "aaa", 5, re.IGNORECASE) +re.split(" ", "a a a a", re.I) +re.split(" ", "a a a a", 2) +re.split(" ", "a a a a", 2, re.I) +sub("a", "b", "aaa", re.IGNORECASE) + +# OK +re.sub("a", "b", "aaa") +re.sub("a", "b", "aaa", flags=re.IGNORECASE) +re.sub("a", "b", "aaa", count=5) +re.sub("a", "b", "aaa", count=5, flags=re.IGNORECASE) +re.subn("a", "b", "aaa") +re.subn("a", "b", "aaa", flags=re.IGNORECASE) +re.subn("a", "b", "aaa", count=5) +re.subn("a", "b", "aaa", count=5, flags=re.IGNORECASE) +re.split(" ", "a a a a", flags=re.I) +re.split(" ", "a a a a", maxsplit=2) +re.split(" ", "a a a a", maxsplit=2, flags=re.I) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 48593c06d58e0..11f8d3600f27c 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2426,12 +2426,14 @@ where } pandas_vet::rules::attr(self, attr, value, expr); } - Expr::Call(ast::ExprCall { - func, - args, - keywords, - range: _, - }) => { + Expr::Call( + call @ ast::ExprCall { + func, + args, + keywords, + range: _, + }, + ) => { if let Expr::Name(ast::ExprName { id, ctx, range: _ }) = func.as_ref() { if id == "locals" && matches!(ctx, ExprContext::Load) { let scope = self.semantic.scope_mut(); @@ -2597,6 +2599,9 @@ where ]) { flake8_bandit::rules::suspicious_function_call(self, expr); } + if self.enabled(Rule::ReSubPositionalArgs) { + flake8_bugbear::rules::re_sub_positional_args(self, call); + } if self.enabled(Rule::UnreliableCallableCheck) { flake8_bugbear::rules::unreliable_callable_check(self, expr, func, args); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 9e9f01e9fb1dd..03dd26fb82780 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -266,6 +266,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (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, "034") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::ReSubPositionalArgs), (Flake8Bugbear, "904") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::RaiseWithoutFromInsideExcept), (Flake8Bugbear, "905") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::ZipWithoutExplicitStrict), diff --git a/crates/ruff/src/rules/flake8_bugbear/mod.rs b/crates/ruff/src/rules/flake8_bugbear/mod.rs index 7e4864a60d648..df1c4632503c4 100644 --- a/crates/ruff/src/rules/flake8_bugbear/mod.rs +++ b/crates/ruff/src/rules/flake8_bugbear/mod.rs @@ -19,7 +19,6 @@ mod tests { #[test_case(Rule::AssertRaisesException, Path::new("B017.py"))] #[test_case(Rule::AssignmentToOsEnviron, Path::new("B003.py"))] #[test_case(Rule::CachedInstanceMethod, Path::new("B019.py"))] - #[test_case(Rule::RaiseLiteral, Path::new("B016.py"))] #[test_case(Rule::DuplicateHandlerException, Path::new("B014.py"))] #[test_case(Rule::DuplicateTryBlockException, Path::new("B025.py"))] #[test_case(Rule::DuplicateValue, Path::new("B033.py"))] @@ -35,7 +34,9 @@ mod tests { #[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::RaiseLiteral, Path::new("B016.py"))] #[test_case(Rule::RaiseWithoutFromInsideExcept, Path::new("B904.py"))] + #[test_case(Rule::ReSubPositionalArgs, Path::new("B034.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"))] diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs b/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs index e9f707a0a1b0d..0cb4402221d06 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_bugbear/rules/mod.rs @@ -17,6 +17,7 @@ pub(crate) use mutable_argument_default::*; pub(crate) use no_explicit_stacklevel::*; pub(crate) use raise_literal::*; pub(crate) use raise_without_from_inside_except::*; +pub(crate) use re_sub_positional_args::*; pub(crate) use redundant_tuple_in_exception_handler::*; pub(crate) use reuse_of_groupby_generator::*; pub(crate) use setattr_with_constant::*; @@ -50,6 +51,7 @@ mod mutable_argument_default; mod no_explicit_stacklevel; mod raise_literal; mod raise_without_from_inside_except; +mod re_sub_positional_args; mod redundant_tuple_in_exception_handler; mod reuse_of_groupby_generator; mod setattr_with_constant; diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/re_sub_positional_args.rs b/crates/ruff/src/rules/flake8_bugbear/rules/re_sub_positional_args.rs new file mode 100644 index 0000000000000..5e9cdddb4cc13 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bugbear/rules/re_sub_positional_args.rs @@ -0,0 +1,112 @@ +use std::fmt; + +use rustpython_parser::ast::{self, Ranged}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for calls to `re.sub`, `re.subn`, and `re.split` that pass `count`, +/// `maxsplit`, or `flags` as positional arguments. +/// +/// ## Why is this bad? +/// Passing `count`, `maxsplit`, or `flags` as positional arguments to +/// `re.sub`, re.subn`, or `re.split` can lead to confusion, as most methods in +/// the `re` module accepts `flags` as the third positional argument, while +/// `re.sub`, `re.subn`, and `re.split` have different signatures. +/// +/// Instead, pass `count`, `maxsplit`, and `flags` as keyword arguments. +/// +/// ## Example +/// ```python +/// import re +/// +/// re.split("pattern", "replacement", 1) +/// ``` +/// +/// Use instead: +/// ```python +/// import re +/// +/// re.split("pattern", "replacement", maxsplit=1) +/// ``` +/// +/// ## References +/// - [Python documentation: `re.sub`](https://docs.python.org/3/library/re.html#re.sub) +/// - [Python documentation: `re.subn`](https://docs.python.org/3/library/re.html#re.subn) +/// - [Python documentation: `re.split`](https://docs.python.org/3/library/re.html#re.split) +#[violation] +pub struct ReSubPositionalArgs { + method: Method, +} + +impl Violation for ReSubPositionalArgs { + #[derive_message_formats] + fn message(&self) -> String { + let ReSubPositionalArgs { method } = self; + let param_name = method.param_name(); + format!( + "`{method}` should pass `{param_name}` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions" + ) + } +} + +/// B034 +pub(crate) fn re_sub_positional_args(checker: &mut Checker, call: &ast::ExprCall) { + let Some(method) = checker + .semantic() + .resolve_call_path(&call.func) + .and_then(|call_path| match call_path.as_slice() { + ["re", "sub"] => Some(Method::Sub), + ["re", "subn"] => Some(Method::Subn), + ["re", "split"] => Some(Method::Split), + _ => None, + }) + else { + return; + }; + + if call.args.len() > method.num_args() { + checker.diagnostics.push(Diagnostic::new( + ReSubPositionalArgs { method }, + call.range(), + )); + } +} + +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +enum Method { + Sub, + Subn, + Split, +} + +impl fmt::Display for Method { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::Sub => fmt.write_str("re.sub"), + Self::Subn => fmt.write_str("re.subn"), + Self::Split => fmt.write_str("re.split"), + } + } +} + +impl Method { + const fn param_name(self) -> &'static str { + match self { + Self::Sub => "count", + Self::Subn => "count", + Self::Split => "maxsplit", + } + } + + const fn num_args(self) -> usize { + match self { + Self::Sub => 3, + Self::Subn => 3, + Self::Split => 2, + } + } +} diff --git a/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B034_B034.py.snap b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B034_B034.py.snap new file mode 100644 index 0000000000000..44efbb7d97307 --- /dev/null +++ b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B034_B034.py.snap @@ -0,0 +1,102 @@ +--- +source: crates/ruff/src/rules/flake8_bugbear/mod.rs +--- +B034.py:5:1: B034 `re.sub` should pass `count` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions + | +4 | # B034 +5 | re.sub("a", "b", "aaa", re.IGNORECASE) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B034 +6 | re.sub("a", "b", "aaa", 5) +7 | re.sub("a", "b", "aaa", 5, re.IGNORECASE) + | + +B034.py:6:1: B034 `re.sub` should pass `count` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions + | +4 | # B034 +5 | re.sub("a", "b", "aaa", re.IGNORECASE) +6 | re.sub("a", "b", "aaa", 5) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ B034 +7 | re.sub("a", "b", "aaa", 5, re.IGNORECASE) +8 | re.subn("a", "b", "aaa", re.IGNORECASE) + | + +B034.py:7:1: B034 `re.sub` should pass `count` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions + | +5 | re.sub("a", "b", "aaa", re.IGNORECASE) +6 | re.sub("a", "b", "aaa", 5) +7 | re.sub("a", "b", "aaa", 5, re.IGNORECASE) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B034 +8 | re.subn("a", "b", "aaa", re.IGNORECASE) +9 | re.subn("a", "b", "aaa", 5) + | + +B034.py:8:1: B034 `re.subn` should pass `count` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions + | + 6 | re.sub("a", "b", "aaa", 5) + 7 | re.sub("a", "b", "aaa", 5, re.IGNORECASE) + 8 | re.subn("a", "b", "aaa", re.IGNORECASE) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B034 + 9 | re.subn("a", "b", "aaa", 5) +10 | re.subn("a", "b", "aaa", 5, re.IGNORECASE) + | + +B034.py:9:1: B034 `re.subn` should pass `count` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions + | + 7 | re.sub("a", "b", "aaa", 5, re.IGNORECASE) + 8 | re.subn("a", "b", "aaa", re.IGNORECASE) + 9 | re.subn("a", "b", "aaa", 5) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ B034 +10 | re.subn("a", "b", "aaa", 5, re.IGNORECASE) +11 | re.split(" ", "a a a a", re.I) + | + +B034.py:10:1: B034 `re.subn` should pass `count` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions + | + 8 | re.subn("a", "b", "aaa", re.IGNORECASE) + 9 | re.subn("a", "b", "aaa", 5) +10 | re.subn("a", "b", "aaa", 5, re.IGNORECASE) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B034 +11 | re.split(" ", "a a a a", re.I) +12 | re.split(" ", "a a a a", 2) + | + +B034.py:11:1: B034 `re.split` should pass `maxsplit` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions + | + 9 | re.subn("a", "b", "aaa", 5) +10 | re.subn("a", "b", "aaa", 5, re.IGNORECASE) +11 | re.split(" ", "a a a a", re.I) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B034 +12 | re.split(" ", "a a a a", 2) +13 | re.split(" ", "a a a a", 2, re.I) + | + +B034.py:12:1: B034 `re.split` should pass `maxsplit` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions + | +10 | re.subn("a", "b", "aaa", 5, re.IGNORECASE) +11 | re.split(" ", "a a a a", re.I) +12 | re.split(" ", "a a a a", 2) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ B034 +13 | re.split(" ", "a a a a", 2, re.I) +14 | sub("a", "b", "aaa", re.IGNORECASE) + | + +B034.py:13:1: B034 `re.split` should pass `maxsplit` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions + | +11 | re.split(" ", "a a a a", re.I) +12 | re.split(" ", "a a a a", 2) +13 | re.split(" ", "a a a a", 2, re.I) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B034 +14 | sub("a", "b", "aaa", re.IGNORECASE) + | + +B034.py:14:1: B034 `re.sub` should pass `count` and `flags` as keyword arguments to avoid confusion due to unintuitive argument positions + | +12 | re.split(" ", "a a a a", 2) +13 | re.split(" ", "a a a a", 2, re.I) +14 | sub("a", "b", "aaa", re.IGNORECASE) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ B034 +15 | +16 | # OK + | + + diff --git a/crates/ruff/src/rules/pylint/rules/type_name_incorrect_variance.rs b/crates/ruff/src/rules/pylint/rules/type_name_incorrect_variance.rs index 8683e09d15fa3..45e9622584ea9 100644 --- a/crates/ruff/src/rules/pylint/rules/type_name_incorrect_variance.rs +++ b/crates/ruff/src/rules/pylint/rules/type_name_incorrect_variance.rs @@ -66,14 +66,14 @@ impl Violation for TypeNameIncorrectVariance { /// PLC0105 pub(crate) fn type_name_incorrect_variance(checker: &mut Checker, value: &Expr) { let Expr::Call(ast::ExprCall { - func, - args, - keywords, - .. - }) = value - else { - return; - }; + func, + args, + keywords, + .. + }) = value + else { + return; + }; let Some(param_name) = type_param_name(args, keywords) else { return; @@ -121,9 +121,9 @@ pub(crate) fn type_name_incorrect_variance(checker: &mut Checker, value: &Expr) None } }) - else { - return; - }; + else { + return; + }; let variance = variance(covariant, contravariant); let name_root = param_name diff --git a/ruff.schema.json b/ruff.schema.json index 84345a1cf2060..ae611fd379760 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1712,6 +1712,7 @@ "B031", "B032", "B033", + "B034", "B9", "B90", "B904",