Check for parenthesis in implicit str concat in PT006 (#3955)

crates/ruff/resources/test/fixtures/flake8_pytest_style/PT006.py

@@ -49,3 +49,18 @@ def test_list_expressions(param1, param2):
 @pytest.mark.parametrize([some_expr, "param2"], [1, 2, 3])
 def test_list_mixed_expr_literal(param1, param2):
     ...
+
+
+@pytest.mark.parametrize(("param1, " "param2, " "param3"), [(1, 2, 3), (4, 5, 6)])
+def test_implicit_str_concat_with_parens(param1, param2, param3):
+    ...
+
+
+@pytest.mark.parametrize("param1, " "param2, " "param3", [(1, 2, 3), (4, 5, 6)])
+def test_implicit_str_concat_no_parens(param1, param2, param3):
+    ...
+
+
+@pytest.mark.parametrize((("param1, " "param2, " "param3")), [(1, 2, 3), (4, 5, 6)])
+def test_implicit_str_concat_with_multi_parens(param1, param2, param3):
+    ...

crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs

@@ -8,7 +8,7 @@ use crate::checkers::ast::Checker;
 
 const ITERABLE_INITIALIZERS: &[&str] = &["dict", "frozenset", "list", "tuple", "set"];
 
-pub fn get_mark_decorators(decorators: &[Expr]) -> impl Iterator<Item = (&Expr, CallPath)> {
+pub(super) fn get_mark_decorators(decorators: &[Expr]) -> impl Iterator<Item = (&Expr, CallPath)> {
     decorators.iter().filter_map(|decorator| {
         let Some(call_path) = collect_call_path(map_callable(decorator)) else {
             return None;
@@ -21,7 +21,7 @@ pub fn get_mark_decorators(decorators: &[Expr]) -> impl Iterator<Item = (&Expr,
     })
 }
 
-pub fn is_pytest_fail(call: &Expr, checker: &Checker) -> bool {
+pub(super) fn is_pytest_fail(call: &Expr, checker: &Checker) -> bool {
     checker
         .ctx
         .resolve_call_path(call)
@@ -30,7 +30,7 @@ pub fn is_pytest_fail(call: &Expr, checker: &Checker) -> bool {
         })
 }
 
-pub fn is_pytest_fixture(decorator: &Expr, checker: &Checker) -> bool {
+pub(super) fn is_pytest_fixture(decorator: &Expr, checker: &Checker) -> bool {
     checker
         .ctx
         .resolve_call_path(if let ExprKind::Call { func, .. } = &decorator.node {
@@ -43,7 +43,7 @@ pub fn is_pytest_fixture(decorator: &Expr, checker: &Checker) -> bool {
         })
 }
 
-pub fn is_pytest_yield_fixture(decorator: &Expr, checker: &Checker) -> bool {
+pub(super) fn is_pytest_yield_fixture(decorator: &Expr, checker: &Checker) -> bool {
     checker
         .ctx
         .resolve_call_path(map_callable(decorator))
@@ -52,7 +52,7 @@ pub fn is_pytest_yield_fixture(decorator: &Expr, checker: &Checker) -> bool {
         })
 }
 
-pub fn is_abstractmethod_decorator(decorator: &Expr, checker: &Checker) -> bool {
+pub(super) fn is_abstractmethod_decorator(decorator: &Expr, checker: &Checker) -> bool {
     checker
         .ctx
         .resolve_call_path(decorator)
@@ -62,10 +62,10 @@ pub fn is_abstractmethod_decorator(decorator: &Expr, checker: &Checker) -> bool
 }
 
 /// Check if the expression is a constant that evaluates to false.
-pub fn is_falsy_constant(expr: &Expr) -> bool {
+pub(super) fn is_falsy_constant(expr: &Expr) -> bool {
     match &expr.node {
         ExprKind::Constant { value, .. } => match value {
-            Constant::Bool(value) => !value,
+            Constant::Bool(value) => !*value,
             Constant::None => true,
             Constant::Str(string) => string.is_empty(),
             Constant::Bytes(bytes) => bytes.is_empty(),
@@ -103,7 +103,7 @@ pub fn is_falsy_constant(expr: &Expr) -> bool {
     }
 }
 
-pub fn is_pytest_parametrize(decorator: &Expr, checker: &Checker) -> bool {
+pub(super) fn is_pytest_parametrize(decorator: &Expr, checker: &Checker) -> bool {
     checker
         .ctx
         .resolve_call_path(map_callable(decorator))
@@ -112,7 +112,7 @@ pub fn is_pytest_parametrize(decorator: &Expr, checker: &Checker) -> bool {
         })
 }
 
-pub fn keyword_is_literal(kw: &Keyword, literal: &str) -> bool {
+pub(super) fn keyword_is_literal(kw: &Keyword, literal: &str) -> bool {
     if let ExprKind::Constant {
         value: Constant::Str(string),
         ..
@@ -124,7 +124,7 @@ pub fn keyword_is_literal(kw: &Keyword, literal: &str) -> bool {
     }
 }
 
-pub fn is_empty_or_null_string(expr: &Expr) -> bool {
+pub(super) fn is_empty_or_null_string(expr: &Expr) -> bool {
     match &expr.node {
         ExprKind::Constant {
             value: Constant::Str(string),
@@ -139,7 +139,7 @@ pub fn is_empty_or_null_string(expr: &Expr) -> bool {
     }
 }
 
-pub fn split_names(names: &str) -> Vec<&str> {
+pub(super) fn split_names(names: &str) -> Vec<&str> {
     // Match the following pytest code:
     //    [x.strip() for x in argnames.split(",") if x.strip()]
     names

crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs

@@ -1,4 +1,5 @@
 use rustpython_parser::ast::{Constant, Expr, ExprContext, ExprKind};
+use rustpython_parser::{lexer, Mode, Tok};
 
 use ruff_diagnostics::{AlwaysAutofixableViolation, Violation};
 use ruff_diagnostics::{Diagnostic, Edit};
@@ -80,8 +81,54 @@ fn elts_to_csv(elts: &[Expr], checker: &Checker) -> Option<String> {
     ))
 }
 
+/// Returns the range of the `name` argument of `@pytest.mark.parametrize`.
+///
+/// This accounts for implicit string concatenation with parenthesis.
+/// For example, the following code will return the range marked with `^`:
+/// ```python
+/// @pytest.mark.parametrize(("a, " "b"), [(1, 2)])
+/// #                        ^^^^^^^^^^^
+/// #                        implicit string concatenation with parenthesis
+/// def test(a, b):
+///     ...
+/// ```
+///
+/// This method assumes that the first argument is a string.
+fn get_parametrize_name_range(checker: &Checker, decorator: &Expr, expr: &Expr) -> Range {
+    let mut locations = Vec::new();
+    let mut implicit_concat = None;
+
+    // The parenthesis are not part of the AST, so we need to tokenize the
+    // decorator to find them.
+    for (start, tok, end) in lexer::lex_located(
+        checker.locator.slice(decorator),
+        Mode::Module,
+        decorator.location,
+    )
+    .flatten()
+    {
+        match tok {
+            Tok::Lpar => locations.push(start),
+            Tok::Rpar => {
+                if let Some(start) = locations.pop() {
+                    implicit_concat = Some(Range::new(start, end));
+                }
+            }
+            // Stop after the first argument.
+            Tok::Comma => break,
+            _ => (),
+        }
+    }
+
+    if let Some(range) = implicit_concat {
+        range
+    } else {
+        Range::from(expr)
+    }
+}
+
 /// PT006
-fn check_names(checker: &mut Checker, expr: &Expr) {
+fn check_names(checker: &mut Checker, decorator: &Expr, expr: &Expr) {
     let names_type = checker.settings.flake8_pytest_style.parametrize_names_type;
 
     match &expr.node {
@@ -93,11 +140,12 @@ fn check_names(checker: &mut Checker, expr: &Expr) {
             if names.len() > 1 {
                 match names_type {
                     types::ParametrizeNameType::Tuple => {
+                        let name_range = get_parametrize_name_range(checker, decorator, expr);
                         let mut diagnostic = Diagnostic::new(
                             PytestParametrizeNamesWrongType {
                                 expected: names_type,
                             },
-                            Range::from(expr),
+                            name_range,
                         );
                         if checker.patch(diagnostic.kind.rule()) {
                             diagnostic.set_fix(Edit::replacement(
@@ -119,18 +167,19 @@ fn check_names(checker: &mut Checker, expr: &Expr) {
                                         checker.stylist,
                                     )
                                 ),
-                                expr.location,
+                                name_range.location,
-                                expr.end_location.unwrap(),
+                                name_range.end_location,
                             ));
                         }
                         checker.diagnostics.push(diagnostic);
                     }
                     types::ParametrizeNameType::List => {
+                        let name_range = get_parametrize_name_range(checker, decorator, expr);
                         let mut diagnostic = Diagnostic::new(
                             PytestParametrizeNamesWrongType {
                                 expected: names_type,
                             },
-                            Range::from(expr),
+                            name_range,
                         );
                         if checker.patch(diagnostic.kind.rule()) {
                             diagnostic.set_fix(Edit::replacement(
@@ -149,8 +198,8 @@ fn check_names(checker: &mut Checker, expr: &Expr) {
                                     }),
                                     checker.stylist,
                                 ),
-                                expr.location,
+                                name_range.location,
-                                expr.end_location.unwrap(),
+                                name_range.end_location,
                             ));
                         }
                         checker.diagnostics.push(diagnostic);
@@ -383,7 +432,7 @@ pub fn parametrize(checker: &mut Checker, decorators: &[Expr]) {
                     .enabled(Rule::PytestParametrizeNamesWrongType)
                 {
                     if let Some(names) = args.get(0) {
-                        check_names(checker, names);
+                        check_names(checker, decorator, names);
                     }
                 }
                 if checker

crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT006_default.snap

@@ -151,5 +151,62 @@ PT006.py:49:26: PT006 [*] Wrong name(s) type in `@pytest.mark.parametrize`, expe
    49 |+@pytest.mark.parametrize((some_expr, "param2"), [1, 2, 3])
 50 50 | def test_list_mixed_expr_literal(param1, param2):
 51 51 |     ...
+52 52 | 
+
+PT006.py:54:26: PT006 [*] Wrong name(s) type in `@pytest.mark.parametrize`, expected `tuple`
+   |
+54 | @pytest.mark.parametrize(("param1, " "param2, " "param3"), [(1, 2, 3), (4, 5, 6)])
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT006
+55 | def test_implicit_str_concat_with_parens(param1, param2, param3):
+56 |     ...
+   |
+   = help: Use a `tuple` for parameter names
+
+ℹ Suggested fix
+51 51 |     ...
+52 52 | 
+53 53 | 
+54    |-@pytest.mark.parametrize(("param1, " "param2, " "param3"), [(1, 2, 3), (4, 5, 6)])
+   54 |+@pytest.mark.parametrize(("param1", "param2", "param3"), [(1, 2, 3), (4, 5, 6)])
+55 55 | def test_implicit_str_concat_with_parens(param1, param2, param3):
+56 56 |     ...
+57 57 | 
+
+PT006.py:59:26: PT006 [*] Wrong name(s) type in `@pytest.mark.parametrize`, expected `tuple`
+   |
+59 | @pytest.mark.parametrize("param1, " "param2, " "param3", [(1, 2, 3), (4, 5, 6)])
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT006
+60 | def test_implicit_str_concat_no_parens(param1, param2, param3):
+61 |     ...
+   |
+   = help: Use a `tuple` for parameter names
+
+ℹ Suggested fix
+56 56 |     ...
+57 57 | 
+58 58 | 
+59    |-@pytest.mark.parametrize("param1, " "param2, " "param3", [(1, 2, 3), (4, 5, 6)])
+   59 |+@pytest.mark.parametrize(("param1", "param2", "param3"), [(1, 2, 3), (4, 5, 6)])
+60 60 | def test_implicit_str_concat_no_parens(param1, param2, param3):
+61 61 |     ...
+62 62 | 
+
+PT006.py:64:26: PT006 [*] Wrong name(s) type in `@pytest.mark.parametrize`, expected `tuple`
+   |
+64 | @pytest.mark.parametrize((("param1, " "param2, " "param3")), [(1, 2, 3), (4, 5, 6)])
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT006
+65 | def test_implicit_str_concat_with_multi_parens(param1, param2, param3):
+66 |     ...
+   |
+   = help: Use a `tuple` for parameter names
+
+ℹ Suggested fix
+61 61 |     ...
+62 62 | 
+63 63 | 
+64    |-@pytest.mark.parametrize((("param1, " "param2, " "param3")), [(1, 2, 3), (4, 5, 6)])
+   64 |+@pytest.mark.parametrize(("param1", "param2", "param3"), [(1, 2, 3), (4, 5, 6)])
+65 65 | def test_implicit_str_concat_with_multi_parens(param1, param2, param3):
+66 66 |     ...
 
 

crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT006_list.snap

@@ -115,4 +115,60 @@ PT006.py:39:26: PT006 [*] Wrong name(s) type in `@pytest.mark.parametrize`, expe
 41 41 |     ...
 42 42 | 
 
+PT006.py:54:26: PT006 [*] Wrong name(s) type in `@pytest.mark.parametrize`, expected `list`
+   |
+54 | @pytest.mark.parametrize(("param1, " "param2, " "param3"), [(1, 2, 3), (4, 5, 6)])
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT006
+55 | def test_implicit_str_concat_with_parens(param1, param2, param3):
+56 |     ...
+   |
+   = help: Use a `list` for parameter names
+
+ℹ Suggested fix
+51 51 |     ...
+52 52 | 
+53 53 | 
+54    |-@pytest.mark.parametrize(("param1, " "param2, " "param3"), [(1, 2, 3), (4, 5, 6)])
+   54 |+@pytest.mark.parametrize(["param1", "param2", "param3"], [(1, 2, 3), (4, 5, 6)])
+55 55 | def test_implicit_str_concat_with_parens(param1, param2, param3):
+56 56 |     ...
+57 57 | 
+
+PT006.py:59:26: PT006 [*] Wrong name(s) type in `@pytest.mark.parametrize`, expected `list`
+   |
+59 | @pytest.mark.parametrize("param1, " "param2, " "param3", [(1, 2, 3), (4, 5, 6)])
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT006
+60 | def test_implicit_str_concat_no_parens(param1, param2, param3):
+61 |     ...
+   |
+   = help: Use a `list` for parameter names
+
+ℹ Suggested fix
+56 56 |     ...
+57 57 | 
+58 58 | 
+59    |-@pytest.mark.parametrize("param1, " "param2, " "param3", [(1, 2, 3), (4, 5, 6)])
+   59 |+@pytest.mark.parametrize(["param1", "param2", "param3"], [(1, 2, 3), (4, 5, 6)])
+60 60 | def test_implicit_str_concat_no_parens(param1, param2, param3):
+61 61 |     ...
+62 62 | 
+
+PT006.py:64:26: PT006 [*] Wrong name(s) type in `@pytest.mark.parametrize`, expected `list`
+   |
+64 | @pytest.mark.parametrize((("param1, " "param2, " "param3")), [(1, 2, 3), (4, 5, 6)])
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PT006
+65 | def test_implicit_str_concat_with_multi_parens(param1, param2, param3):
+66 |     ...
+   |
+   = help: Use a `list` for parameter names
+
+ℹ Suggested fix
+61 61 |     ...
+62 62 | 
+63 63 | 
+64    |-@pytest.mark.parametrize((("param1, " "param2, " "param3")), [(1, 2, 3), (4, 5, 6)])
+   64 |+@pytest.mark.parametrize(["param1", "param2", "param3"], [(1, 2, 3), (4, 5, 6)])
+65 65 | def test_implicit_str_concat_with_multi_parens(param1, param2, param3):
+66 66 |     ...
+