From 95f7d13f12e158df759a68b931051cb1d4e94038 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 10 Jul 2023 22:43:40 -0400 Subject: [PATCH] [flake8-bugbear] Implement re-sub-positional-args (B034) --- .../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 +- ...lack_compatibility@py_311__pep_654.py.snap | 240 ------------------ ruff.schema.json | 1 + 10 files changed, 269 insertions(+), 258 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 delete mode 100644 crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_311__pep_654.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 00000000000000..1236259c6b57dd --- /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 c522703d8c7a54..4e4d65f9bbac94 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 9e9f01e9fb1dd0..03dd26fb827801 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 7e4864a60d6480..df1c4632503c45 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 e9f707a0a1b0d2..0cb4402221d060 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 00000000000000..5e9cdddb4cc137 --- /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 00000000000000..44efbb7d973077 --- /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 8683e09d15fa35..45e9622584ea9c 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/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_311__pep_654.py.snap b/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_311__pep_654.py.snap deleted file mode 100644 index b7995833b5183e..00000000000000 --- a/crates/ruff_python_formatter/tests/snapshots/black_compatibility@py_311__pep_654.py.snap +++ /dev/null @@ -1,240 +0,0 @@ ---- -source: crates/ruff_python_formatter/tests/fixtures.rs -input_file: crates/ruff_python_formatter/resources/test/fixtures/black/py_311/pep_654.py ---- -## Input - -```py -try: - raise OSError("blah") -except* ExceptionGroup as e: - pass - - -try: - async with trio.open_nursery() as nursery: - # Make two concurrent calls to child() - nursery.start_soon(child) - nursery.start_soon(child) -except* ValueError: - pass - -try: - try: - raise ValueError(42) - except: - try: - raise TypeError(int) - except* Exception: - pass - 1 / 0 -except Exception as e: - exc = e - -try: - try: - raise FalsyEG("eg", [TypeError(1), ValueError(2)]) - except* TypeError as e: - tes = e - raise - except* ValueError as e: - ves = e - pass -except Exception as e: - exc = e - -try: - try: - raise orig - except* (TypeError, ValueError) as e: - raise SyntaxError(3) from e -except BaseException as e: - exc = e - -try: - try: - raise orig - except* OSError as e: - raise TypeError(3) from e -except ExceptionGroup as e: - exc = e -``` - -## Black Differences - -```diff ---- Black -+++ Ruff -@@ -1,5 +1,5 @@ - try: -- raise OSError("blah") -+ NOT_YET_IMPLEMENTED_StmtRaise - except* ExceptionGroup as e: - pass - -@@ -14,10 +14,10 @@ - - try: - try: -- raise ValueError(42) -+ NOT_YET_IMPLEMENTED_StmtRaise - except: - try: -- raise TypeError(int) -+ NOT_YET_IMPLEMENTED_StmtRaise - except* Exception: - pass - 1 / 0 -@@ -26,10 +26,10 @@ - - try: - try: -- raise FalsyEG("eg", [TypeError(1), ValueError(2)]) -+ NOT_YET_IMPLEMENTED_StmtRaise - except* TypeError as e: - tes = e -- raise -+ NOT_YET_IMPLEMENTED_StmtRaise - except* ValueError as e: - ves = e - pass -@@ -38,16 +38,16 @@ - - try: - try: -- raise orig -+ NOT_YET_IMPLEMENTED_StmtRaise - except* (TypeError, ValueError) as e: -- raise SyntaxError(3) from e -+ NOT_YET_IMPLEMENTED_StmtRaise - except BaseException as e: - exc = e - - try: - try: -- raise orig -+ NOT_YET_IMPLEMENTED_StmtRaise - except* OSError as e: -- raise TypeError(3) from e -+ NOT_YET_IMPLEMENTED_StmtRaise - except ExceptionGroup as e: - exc = e -``` - -## Ruff Output - -```py -try: - NOT_YET_IMPLEMENTED_StmtRaise -except* ExceptionGroup as e: - pass - - -try: - async with trio.open_nursery() as nursery: - # Make two concurrent calls to child() - nursery.start_soon(child) - nursery.start_soon(child) -except* ValueError: - pass - -try: - try: - NOT_YET_IMPLEMENTED_StmtRaise - except: - try: - NOT_YET_IMPLEMENTED_StmtRaise - except* Exception: - pass - 1 / 0 -except Exception as e: - exc = e - -try: - try: - NOT_YET_IMPLEMENTED_StmtRaise - except* TypeError as e: - tes = e - NOT_YET_IMPLEMENTED_StmtRaise - except* ValueError as e: - ves = e - pass -except Exception as e: - exc = e - -try: - try: - NOT_YET_IMPLEMENTED_StmtRaise - except* (TypeError, ValueError) as e: - NOT_YET_IMPLEMENTED_StmtRaise -except BaseException as e: - exc = e - -try: - try: - NOT_YET_IMPLEMENTED_StmtRaise - except* OSError as e: - NOT_YET_IMPLEMENTED_StmtRaise -except ExceptionGroup as e: - exc = e -``` - -## Black Output - -```py -try: - raise OSError("blah") -except* ExceptionGroup as e: - pass - - -try: - async with trio.open_nursery() as nursery: - # Make two concurrent calls to child() - nursery.start_soon(child) - nursery.start_soon(child) -except* ValueError: - pass - -try: - try: - raise ValueError(42) - except: - try: - raise TypeError(int) - except* Exception: - pass - 1 / 0 -except Exception as e: - exc = e - -try: - try: - raise FalsyEG("eg", [TypeError(1), ValueError(2)]) - except* TypeError as e: - tes = e - raise - except* ValueError as e: - ves = e - pass -except Exception as e: - exc = e - -try: - try: - raise orig - except* (TypeError, ValueError) as e: - raise SyntaxError(3) from e -except BaseException as e: - exc = e - -try: - try: - raise orig - except* OSError as e: - raise TypeError(3) from e -except ExceptionGroup as e: - exc = e -``` - - diff --git a/ruff.schema.json b/ruff.schema.json index 84345a1cf2060b..ae611fd379760b 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1712,6 +1712,7 @@ "B031", "B032", "B033", + "B034", "B9", "B90", "B904",