Fix pytest-parametrize-names-wrong-type (PT006) to edit both argnames and argvalues if both of them are single-element tuples/lists

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

@@ -81,3 +81,78 @@ def test_not_decorator(param1, param2):
 @pytest.mark.parametrize(argnames=("param1,param2"), argvalues=[(1, 2), (3, 4)])
 def test_keyword_arguments(param1, param2):
     ...
+
+
+@pytest.mark.parametrize(("param",), [(1,), (2,)])
+def test_single_element_tuple(param):
+    ...
+
+
+@pytest.mark.parametrize(("param",), [[1], [2]])
+def test_single_element_list(param):
+    ...
+
+
+@pytest.mark.parametrize(("param",), [[1], [2]])
+def test_single_element_list(param):
+    ...
+
+
+# Unsafe fix
+@pytest.mark.parametrize(
+    (
+        # comment
+        "param",
+    ),
+    [[1], [2]],
+)
+def test_comment_in_argnames(param):
+    ...
+
+# Unsafe fix
+@pytest.mark.parametrize(
+    ("param",),
+    [
+        (
+            # comment
+            1,
+        ),
+        (2,),
+    ],
+)
+def test_comment_in_argvalues(param):
+    ...
+
+
+# Safe fix
+@pytest.mark.parametrize(
+    ("param",),
+    [
+        (1,),
+        # comment
+        (2,),
+    ],
+)
+def test_comment_between_argvalues_items(param):
+    ...
+
+
+# A fix should be suggested for `argnames`, but not for `argvalues`.
+@pytest.mark.parametrize(
+    ("param",),
+    [
+        (1,),
+        (2, 3),
+    ],
+)
+def test_invalid_argvalues(param):
+    """
+    pytest throws the following error for this test:
+    ------------------------------------------------
+    a.py::test_comment_between_argvalues_items: in "parametrize" the number of names (1):
+        ('param',)
+    must be equal to the number of values (2):
+        (2, 3)
+    ------------------------------------------------
+    """
+    ...

crates/ruff_linter/resources/test/fixtures/flake8_pytest_style/PT006_and_PT007.py

@@ -0,0 +1,5 @@
+import pytest
+
+@pytest.mark.parametrize(("param",), [[1], [2]])
+def test_PT006_and_PT007_do_not_conflict(param):
+    ...

crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs

@@ -293,6 +293,26 @@ mod tests {
         Ok(())
     }
 
+    /// This test ensure that PT006 and PT007 don't conflict when both of them suggest a fix that
+    /// edits `argvalues` for `pytest.mark.parametrize`.
+    #[test]
+    fn test_pytest_style_pt006_and_pt007() -> Result<()> {
+        let diagnostics = test_path(
+            Path::new("flake8_pytest_style")
+                .join(Path::new("PT006_and_PT007.py"))
+                .as_path(),
+            &settings::LinterSettings {
+                preview: PreviewMode::Enabled,
+                ..settings::LinterSettings::for_rules(vec![
+                    Rule::PytestParametrizeNamesWrongType,
+                    Rule::PytestParametrizeValuesWrongType,
+                ])
+            },
+        )?;
+        assert_messages!("PT006_and_PT007", diagnostics);
+        Ok(())
+    }
+
     #[test_case(Rule::PytestParametrizeNamesWrongType, Path::new("PT006.py"))]
     fn test_pytest_style_preview(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!(

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

@@ -331,7 +331,7 @@ fn get_parametrize_name_range(
 }
 
 /// PT006
-fn check_names(checker: &mut Checker, call: &ExprCall, expr: &Expr) {
+fn check_names(checker: &mut Checker, call: &ExprCall, expr: &Expr, argvalues: &Expr) {
     let names_type = checker.settings.flake8_pytest_style.parametrize_names_type;
 
     match expr {
@@ -414,7 +414,7 @@ fn check_names(checker: &mut Checker, call: &ExprCall, expr: &Expr) {
         }
         Expr::Tuple(ast::ExprTuple { elts, .. }) => {
             if elts.len() == 1 {
-                handle_single_name(checker, expr, &elts[0]);
+                handle_single_name(checker, expr, &elts[0], argvalues);
             } else {
                 match names_type {
                     types::ParametrizeNameType::Tuple => {}
@@ -458,7 +458,7 @@ fn check_names(checker: &mut Checker, call: &ExprCall, expr: &Expr) {
         }
         Expr::List(ast::ExprList { elts, .. }) => {
             if elts.len() == 1 {
-                handle_single_name(checker, expr, &elts[0]);
+                handle_single_name(checker, expr, &elts[0], argvalues);
             } else {
                 match names_type {
                     types::ParametrizeNameType::List => {}
@@ -678,23 +678,85 @@ fn check_duplicates(checker: &mut Checker, values: &Expr) {
     }
 }
 
-fn handle_single_name(checker: &mut Checker, expr: &Expr, value: &Expr) {
+fn handle_single_name(checker: &mut Checker, argnames: &Expr, value: &Expr, argvalues: &Expr) {
     let mut diagnostic = Diagnostic::new(
         PytestParametrizeNamesWrongType {
             single_argument: true,
             expected: types::ParametrizeNameType::Csv,
         },
-        expr.range(),
+        argnames.range(),
     );
-
-    let node = value.clone();
-    diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
-        checker.generator().expr(&node),
-        expr.range(),
-    )));
+    // If `argnames` and all items in `argvalues` are single-element sequences,
+    // they all should be unpacked. Here's an example:
+    //
+    // ```python
+    // @pytest.mark.parametrize(("x",), [(1,), (2,)])
+    // def test_foo(x):
+    //     assert isinstance(x, int)
+    // ```
+    //
+    // The code above should be transformed into:
+    //
+    // ```python
+    // @pytest.mark.parametrize("x", [1, 2])
+    // def test_foo(x):
+    //     assert isinstance(x, int)
+    // ```
+    //
+    // Only unpacking `argnames` would break the test:
+    //
+    // ```python
+    // @pytest.mark.parametrize("x", [(1,), (2,)])
+    // def test_foo(x):
+    //     assert isinstance(x, int)  # fails because `x` is a tuple, not an int
+    // ```
+    let argvalues_edits = unpack_single_element_items(checker, argvalues);
+    let argnames_edit = Edit::range_replacement(checker.generator().expr(value), argnames.range());
+    let fix = if checker.comment_ranges().intersects(argnames_edit.range())
+        || argvalues_edits
+            .iter()
+            .any(|edit| checker.comment_ranges().intersects(edit.range()))
+    {
+        Fix::unsafe_edits(argnames_edit, argvalues_edits)
+    } else {
+        Fix::safe_edits(argnames_edit, argvalues_edits)
+    };
+    diagnostic.set_fix(fix);
     checker.diagnostics.push(diagnostic);
 }
 
+/// Generate [`Edit`]s to unpack single-element lists or tuples in the given [`Expr`].
+/// For instance, `[(1,) (2,)]` will be transformed into `[1, 2]`.
+fn unpack_single_element_items(checker: &Checker, expr: &Expr) -> Vec<Edit> {
+    let (Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. })) = expr
+    else {
+        return vec![];
+    };
+
+    let mut edits = Vec::with_capacity(elts.len());
+    for value in elts {
+        let (Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. })) =
+            value
+        else {
+            return vec![];
+        };
+
+        let [elt] = elts.as_slice() else {
+            return vec![];
+        };
+
+        if matches!(elt, Expr::Starred(_)) {
+            return vec![];
+        }
+
+        edits.push(Edit::range_replacement(
+            checker.generator().expr(elt),
+            value.range(),
+        ));
+    }
+    edits
+}
+
 fn handle_value_rows(
     checker: &mut Checker,
     elts: &[Expr],
@@ -801,12 +863,18 @@ pub(crate) fn parametrize(checker: &mut Checker, call: &ExprCall) {
     }
 
     if checker.enabled(Rule::PytestParametrizeNamesWrongType) {
-        if let Some(names) = if checker.settings.preview.is_enabled() {
+        let names = if checker.settings.preview.is_enabled() {
             call.arguments.find_argument("argnames", 0)
         } else {
             call.arguments.find_positional(0)
-        } {
-            check_names(checker, call, names);
+        };
+        let values = if checker.settings.preview.is_enabled() {
+            call.arguments.find_argument("argvalues", 1)
+        } else {
+            call.arguments.find_positional(1)
+        };
+        if let (Some(names), Some(values)) = (names, values) {
+            check_names(checker, call, names, values);
         }
     }
     if checker.enabled(Rule::PytestParametrizeValuesWrongType) {

crates/ruff_linter/src/rules/flake8_pytest_style/snapshots/ruff_linter__rules__flake8_pytest_style__tests__PT006_and_PT007.snap

@@ -0,0 +1,60 @@
+---
+source: crates/ruff_linter/src/rules/flake8_pytest_style/mod.rs
+snapshot_kind: text
+---
+PT006_and_PT007.py:3:26: PT006 [*] Wrong type passed to first argument of `pytest.mark.parametrize`; expected `str`
+  |
+1 | import pytest
+2 | 
+3 | @pytest.mark.parametrize(("param",), [[1], [2]])
+  |                          ^^^^^^^^^^ PT006
+4 | def test_PT006_and_PT007_do_not_conflict(param):
+5 |     ...
+  |
+  = help: Use a string for the first argument
+
+ℹ Safe fix
+1 1 | import pytest
+2 2 | 
+3   |-@pytest.mark.parametrize(("param",), [[1], [2]])
+  3 |+@pytest.mark.parametrize("param", [1, 2])
+4 4 | def test_PT006_and_PT007_do_not_conflict(param):
+5 5 |     ...
+
+PT006_and_PT007.py:3:39: PT007 [*] Wrong values type in `pytest.mark.parametrize` expected `list` of `tuple`
+  |
+1 | import pytest
+2 | 
+3 | @pytest.mark.parametrize(("param",), [[1], [2]])
+  |                                       ^^^ PT007
+4 | def test_PT006_and_PT007_do_not_conflict(param):
+5 |     ...
+  |
+  = help: Use `list` of `tuple` for parameter values
+
+ℹ Unsafe fix
+1 1 | import pytest
+2 2 | 
+3   |-@pytest.mark.parametrize(("param",), [[1], [2]])
+  3 |+@pytest.mark.parametrize(("param",), [(1,), [2]])
+4 4 | def test_PT006_and_PT007_do_not_conflict(param):
+5 5 |     ...
+
+PT006_and_PT007.py:3:44: PT007 [*] Wrong values type in `pytest.mark.parametrize` expected `list` of `tuple`
+  |
+1 | import pytest
+2 | 
+3 | @pytest.mark.parametrize(("param",), [[1], [2]])
+  |                                            ^^^ PT007
+4 | def test_PT006_and_PT007_do_not_conflict(param):
+5 |     ...
+  |
+  = help: Use `list` of `tuple` for parameter values
+
+ℹ Unsafe fix
+1 1 | import pytest
+2 2 | 
+3   |-@pytest.mark.parametrize(("param",), [[1], [2]])
+  3 |+@pytest.mark.parametrize(("param",), [[1], (2,)])
+4 4 | def test_PT006_and_PT007_do_not_conflict(param):
+5 5 |     ...