From 032a84b1674c73db8796f3f50b7b0d56a63e9a3a Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 13 Apr 2023 23:26:18 +0530 Subject: [PATCH] Check for parenthesis in implicit str concat in `PT006` (#3955) --- .../fixtures/flake8_pytest_style/PT006.py | 15 +++++ .../flake8_pytest_style/rules/helpers.rs | 22 +++---- .../flake8_pytest_style/rules/parametrize.rs | 65 ++++++++++++++++--- ...e8_pytest_style__tests__PT006_default.snap | 57 ++++++++++++++++ ...lake8_pytest_style__tests__PT006_list.snap | 56 ++++++++++++++++ 5 files changed, 196 insertions(+), 19 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT006.py b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT006.py index 9e5b229b80..3bfac3ecd8 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT006.py +++ b/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): + ... diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs index e3b83fe3e0..6433bbb672 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs +++ b/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 { +pub(super) fn get_mark_decorators(decorators: &[Expr]) -> impl Iterator { 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 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 diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs index eff5528fc1..ca5c446b39 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs +++ b/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 { )) } +/// 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, - expr.end_location.unwrap(), + name_range.location, + 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, - expr.end_location.unwrap(), + name_range.location, + 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 diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT006_default.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT006_default.snap index 7e280eccf3..2aad073076 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT006_default.snap +++ b/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 | ... diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT006_list.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT006_list.snap index bba294df51..c052285e34 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT006_list.snap +++ b/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 | ... +