Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-bugbear] Implement re-sub-positional-args (B034) #5669

Merged
merged 1 commit into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.