Skip to content

Commit

Permalink
[flake8-bugbear] Implement re-sub-positional-args (B034)
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Jul 11, 2023
1 parent e7e2f44 commit 95f7d13
Show file tree
Hide file tree
Showing 10 changed files with 269 additions and 258 deletions.
27 changes: 27 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_bugbear/B034.py
Original file line number Diff line number Diff line change
@@ -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)
17 changes: 11 additions & 6 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),

Expand Down
3 changes: 2 additions & 1 deletion crates/ruff/src/rules/flake8_bugbear/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand All @@ -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"))]
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_bugbear/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
112 changes: 112 additions & 0 deletions crates/ruff/src/rules/flake8_bugbear/rules/re_sub_positional_args.rs
Original file line number Diff line number Diff line change
@@ -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,
}
}
}
Original file line number Diff line number Diff line change
@@ -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
|


22 changes: 11 additions & 11 deletions crates/ruff/src/rules/pylint/rules/type_name_incorrect_variance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 95f7d13

Please sign in to comment.