From 7ea6180691f54d091be970f7b82f8ee1447fda61 Mon Sep 17 00:00:00 2001 From: harupy Date: Sun, 20 Aug 2023 12:47:03 +0900 Subject: [PATCH 1/6] Autofix PT014 --- .../fixtures/flake8_pytest_style/PT014.py | 10 ++- .../flake8_pytest_style/rules/parametrize.rs | 22 ++++++- ...es__flake8_pytest_style__tests__PT014.snap | 65 ++++++++++++++++--- 3 files changed, 85 insertions(+), 12 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py index e81cbc15f5aab..2136a6ee0933c 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py +++ b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py @@ -16,7 +16,15 @@ def test_error_expr_simple(x): ... -@pytest.mark.parametrize("x", [(a, b), (a, b), (b, c)]) +@pytest.mark.parametrize( + "x", + [ + (a, b), + # comment + (a, b), + (b, c), + ], +) def test_error_expr_complex(x): ... 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 9165aef160b97..6f9cae6da626c 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs @@ -215,11 +215,17 @@ pub struct PytestDuplicateParametrizeTestCases { } impl Violation for PytestDuplicateParametrizeTestCases { + const AUTOFIX: AutofixKind = AutofixKind::Sometimes; + #[derive_message_formats] fn message(&self) -> String { let PytestDuplicateParametrizeTestCases { index } = self; format!("Duplicate of test case at index {index} in `@pytest_mark.parametrize`") } + + fn autofix_title(&self) -> Option { + Some("Remove duplicate test case".to_string()) + } } fn elts_to_csv(elts: &[Expr], generator: Generator) -> Option { @@ -558,19 +564,31 @@ fn check_duplicates(checker: &mut Checker, values: &Expr) { else { return; }; + if elts.len() < 2 { + return; + } let mut seen: FxHashMap = FxHashMap::with_capacity_and_hasher(elts.len(), BuildHasherDefault::default()); + let mut prev = &elts[0]; for (index, elt) in elts.iter().enumerate() { let expr = ComparableExpr::from(elt); seen.entry(expr) .and_modify(|index| { - checker.diagnostics.push(Diagnostic::new( + let mut diagnostic = Diagnostic::new( PytestDuplicateParametrizeTestCases { index: *index }, elt.range(), - )); + ); + if checker.patch(diagnostic.kind.rule()) { + let del_range = TextRange::new(prev.end(), elt.end()); + if !checker.indexer().comment_ranges().intersects(del_range) { + diagnostic.set_fix(Fix::automatic(Edit::range_deletion(del_range))); + } + } + checker.diagnostics.push(diagnostic); }) .or_insert(index); + prev = elt; } } diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap index 45369f5551b12..fd7067d572007 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap +++ b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap @@ -1,44 +1,91 @@ --- source: crates/ruff/src/rules/flake8_pytest_style/mod.rs --- -PT014.py:4:35: PT014 Duplicate of test case at index 0 in `@pytest_mark.parametrize` +PT014.py:4:35: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.parametrize` | 4 | @pytest.mark.parametrize("x", [1, 1, 2]) | ^ PT014 5 | def test_error_literal(x): 6 | ... | + = help: Remove duplicate test case -PT014.py:14:35: PT014 Duplicate of test case at index 0 in `@pytest_mark.parametrize` +ℹ Fix +1 1 | import pytest +2 2 | +3 3 | +4 |-@pytest.mark.parametrize("x", [1, 1, 2]) + 4 |+@pytest.mark.parametrize("x", [1, 2]) +5 5 | def test_error_literal(x): +6 6 | ... +7 7 | + +PT014.py:14:35: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.parametrize` | 14 | @pytest.mark.parametrize("x", [a, a, b, b, b, c]) | ^ PT014 15 | def test_error_expr_simple(x): 16 | ... | + = help: Remove duplicate test case + +ℹ Fix +11 11 | c = 3 +12 12 | +13 13 | +14 |-@pytest.mark.parametrize("x", [a, a, b, b, b, c]) + 14 |+@pytest.mark.parametrize("x", [a, b, b, b, c]) +15 15 | def test_error_expr_simple(x): +16 16 | ... +17 17 | -PT014.py:14:41: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize` +PT014.py:14:41: PT014 [*] Duplicate of test case at index 2 in `@pytest_mark.parametrize` | 14 | @pytest.mark.parametrize("x", [a, a, b, b, b, c]) | ^ PT014 15 | def test_error_expr_simple(x): 16 | ... | + = help: Remove duplicate test case -PT014.py:14:44: PT014 Duplicate of test case at index 2 in `@pytest_mark.parametrize` +ℹ Fix +11 11 | c = 3 +12 12 | +13 13 | +14 |-@pytest.mark.parametrize("x", [a, a, b, b, b, c]) + 14 |+@pytest.mark.parametrize("x", [a, a, b, b, c]) +15 15 | def test_error_expr_simple(x): +16 16 | ... +17 17 | + +PT014.py:14:44: PT014 [*] Duplicate of test case at index 2 in `@pytest_mark.parametrize` | 14 | @pytest.mark.parametrize("x", [a, a, b, b, b, c]) | ^ PT014 15 | def test_error_expr_simple(x): 16 | ... | + = help: Remove duplicate test case + +ℹ Fix +11 11 | c = 3 +12 12 | +13 13 | +14 |-@pytest.mark.parametrize("x", [a, a, b, b, b, c]) + 14 |+@pytest.mark.parametrize("x", [a, a, b, b, c]) +15 15 | def test_error_expr_simple(x): +16 16 | ... +17 17 | -PT014.py:19:40: PT014 Duplicate of test case at index 0 in `@pytest_mark.parametrize` +PT014.py:24:9: PT014 Duplicate of test case at index 0 in `@pytest_mark.parametrize` | -19 | @pytest.mark.parametrize("x", [(a, b), (a, b), (b, c)]) - | ^^^^^^ PT014 -20 | def test_error_expr_complex(x): -21 | ... +22 | (a, b), +23 | # comment +24 | (a, b), + | ^^^^^^ PT014 +25 | (b, c), +26 | ], | + = help: Remove duplicate test case From a16cdada2dd2b4bcd778e0f4b83ce74ee3b15cc9 Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 21 Aug 2023 00:15:43 +0900 Subject: [PATCH 2/6] Address comments --- .../src/rules/flake8_pytest_style/rules/parametrize.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) 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 6f9cae6da626c..345d92febe111 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs @@ -564,14 +564,16 @@ fn check_duplicates(checker: &mut Checker, values: &Expr) { else { return; }; - if elts.len() < 2 { + let [first, rest @ ..] = elts.as_slice() else { return; }; + if rest.is_empty() { return; } - let mut seen: FxHashMap = FxHashMap::with_capacity_and_hasher(elts.len(), BuildHasherDefault::default()); - let mut prev = &elts[0]; - for (index, elt) in elts.iter().enumerate() { + seen.insert(ComparableExpr::from(first), 0); + let mut prev = first; + for (index, elt) in rest.iter().enumerate() { + let index = index + 1; let expr = ComparableExpr::from(elt); seen.entry(expr) .and_modify(|index| { From 0e11ed2cb0f0b9bbb6b4973d031c801e5eafa6cf Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 21 Aug 2023 00:18:21 +0900 Subject: [PATCH 3/6] Test case for parentheses --- .../ruff/resources/test/fixtures/flake8_pytest_style/PT014.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py index 2136a6ee0933c..eb5772d3abbad 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py +++ b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py @@ -11,7 +11,7 @@ def test_error_literal(x): c = 3 -@pytest.mark.parametrize("x", [a, a, b, b, b, c]) +@pytest.mark.parametrize("x", [a, a, b, b, (b), c]) def test_error_expr_simple(x): ... From d2e8e777534e489d2d864309284e39fb0d8800cb Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 21 Aug 2023 01:41:41 +0900 Subject: [PATCH 4/6] Add tests --- .../fixtures/flake8_pytest_style/PT014.py | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py index eb5772d3abbad..b408063d9b235 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py +++ b/crates/ruff/resources/test/fixtures/flake8_pytest_style/PT014.py @@ -11,7 +11,7 @@ def test_error_literal(x): c = 3 -@pytest.mark.parametrize("x", [a, a, b, b, (b), c]) +@pytest.mark.parametrize("x", [a, a, b, b, b, c]) def test_error_expr_simple(x): ... @@ -29,6 +29,25 @@ def test_error_expr_complex(x): ... +@pytest.mark.parametrize("x", [a, b, (a), c, ((a))]) +def test_error_parentheses(x): + ... + + +@pytest.mark.parametrize( + "x", + [ + a, + b, + (a), + c, + ((a)), + ], +) +def test_error_parentheses_trailing_comma(x): + ... + + @pytest.mark.parametrize("x", [1, 2]) def test_ok(x): ... From fd74b228e4f8244752f53e71e0fc4c89a02d5458 Mon Sep 17 00:00:00 2001 From: harupy Date: Mon, 21 Aug 2023 01:50:30 +0900 Subject: [PATCH 5/6] Handle parentheses --- .../flake8_pytest_style/rules/parametrize.rs | 39 +++++++++- ...es__flake8_pytest_style__tests__PT014.snap | 78 +++++++++++++++++++ 2 files changed, 113 insertions(+), 4 deletions(-) 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 345d92febe111..ee8d5c26f8b71 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs @@ -5,7 +5,7 @@ use ruff_python_ast::{ self as ast, Arguments, Constant, Decorator, Expr, ExprContext, PySourceType, Ranged, }; use ruff_python_parser::{lexer, AsMode, Tok}; -use ruff_text_size::TextRange; +use ruff_text_size::{TextRange, TextSize}; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -557,6 +557,30 @@ fn check_values(checker: &mut Checker, names: &Expr, values: &Expr) { } } +/// ```python +/// @pytest.mark.parametrize( +/// "x", +/// [.., (elt), ..], +/// ^^^^^ +/// Tokenize this range to locate the true end of `elt`. +/// ) +/// ``` +fn true_elt_end(checker: &Checker, elt_end: TextSize, values_end: TextSize) -> TextSize { + for (tok, range) in lexer::lex_starts_at( + checker.locator().slice(TextRange::new(elt_end, values_end)), + checker.source_type.as_mode(), + elt_end, + ) + .flatten() + { + match tok { + Tok::Comma => return range.start(), + _ => continue, + } + } + values_end +} + /// PT014 fn check_duplicates(checker: &mut Checker, values: &Expr) { let (Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. })) = @@ -582,9 +606,16 @@ fn check_duplicates(checker: &mut Checker, values: &Expr) { elt.range(), ); if checker.patch(diagnostic.kind.rule()) { - let del_range = TextRange::new(prev.end(), elt.end()); - if !checker.indexer().comment_ranges().intersects(del_range) { - diagnostic.set_fix(Fix::automatic(Edit::range_deletion(del_range))); + let values_end = values.range().sub_end(1.into()).end(); + let elt_end = true_elt_end(checker, elt.end(), values_end); + let prev_end = true_elt_end(checker, prev.end(), values_end); + let deletion_range = TextRange::new(prev_end, elt_end); + if !checker + .indexer() + .comment_ranges() + .intersects(deletion_range) + { + diagnostic.set_fix(Fix::automatic(Edit::range_deletion(deletion_range))); } } checker.diagnostics.push(diagnostic); diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap index fd7067d572007..242cbe75e78d7 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap +++ b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap @@ -88,4 +88,82 @@ PT014.py:24:9: PT014 Duplicate of test case at index 0 in `@pytest_mark.parametr | = help: Remove duplicate test case +PT014.py:32:39: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.parametrize` + | +32 | @pytest.mark.parametrize("x", [a, b, (a), c, ((a))]) + | ^ PT014 +33 | def test_error_parentheses(x): +34 | ... + | + = help: Remove duplicate test case + +ℹ Fix +29 29 | ... +30 30 | +31 31 | +32 |-@pytest.mark.parametrize("x", [a, b, (a), c, ((a))]) + 32 |+@pytest.mark.parametrize("x", [a, b, c, ((a))]) +33 33 | def test_error_parentheses(x): +34 34 | ... +35 35 | + +PT014.py:32:48: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.parametrize` + | +32 | @pytest.mark.parametrize("x", [a, b, (a), c, ((a))]) + | ^ PT014 +33 | def test_error_parentheses(x): +34 | ... + | + = help: Remove duplicate test case + +ℹ Fix +29 29 | ... +30 30 | +31 31 | +32 |-@pytest.mark.parametrize("x", [a, b, (a), c, ((a))]) + 32 |+@pytest.mark.parametrize("x", [a, b, (a), c]) +33 33 | def test_error_parentheses(x): +34 34 | ... +35 35 | + +PT014.py:42:10: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.parametrize` + | +40 | a, +41 | b, +42 | (a), + | ^ PT014 +43 | c, +44 | ((a)), + | + = help: Remove duplicate test case + +ℹ Fix +39 39 | [ +40 40 | a, +41 41 | b, +42 |- (a), +43 42 | c, +44 43 | ((a)), +45 44 | ], + +PT014.py:44:11: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.parametrize` + | +42 | (a), +43 | c, +44 | ((a)), + | ^ PT014 +45 | ], +46 | ) + | + = help: Remove duplicate test case + +ℹ Fix +41 41 | b, +42 42 | (a), +43 43 | c, +44 |- ((a)), +45 44 | ], +46 45 | ) +47 46 | def test_error_parentheses_trailing_comma(x): + From 66eebf3e123934c0a041db434a9a17d8e30102af Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 20 Aug 2023 23:35:26 -0400 Subject: [PATCH 6/6] Rename --- .../flake8_pytest_style/rules/parametrize.rs | 64 ++++++++----------- ...es__flake8_pytest_style__tests__PT014.snap | 16 ++--- 2 files changed, 36 insertions(+), 44 deletions(-) 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 af1afd633df20..e06727400271c 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs @@ -11,6 +11,7 @@ use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::comparable::ComparableExpr; use ruff_python_codegen::Generator; +use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_source_file::Locator; use crate::checkers::ast::Checker; @@ -557,28 +558,19 @@ fn check_values(checker: &mut Checker, names: &Expr, values: &Expr) { } } +/// Given an element in a list, return the comma that follows it: /// ```python /// @pytest.mark.parametrize( /// "x", /// [.., (elt), ..], /// ^^^^^ -/// Tokenize this range to locate the true end of `elt`. +/// Tokenize this range to locate the comma. /// ) /// ``` -fn true_elt_end(checker: &Checker, elt_end: TextSize, values_end: TextSize) -> TextSize { - for (tok, range) in lexer::lex_starts_at( - checker.locator().slice(TextRange::new(elt_end, values_end)), - checker.source_type.as_mode(), - elt_end, - ) - .flatten() - { - match tok { - Tok::Comma => return range.start(), - _ => continue, - } - } - values_end +fn trailing_comma(element: &Expr, source: &str) -> Option { + SimpleTokenizer::starts_at(element.end(), source) + .find(|token| token.kind == SimpleTokenKind::Comma) + .map(|token| token.start()) } /// PT014 @@ -588,40 +580,40 @@ fn check_duplicates(checker: &mut Checker, values: &Expr) { else { return; }; - let [first, rest @ ..] = elts.as_slice() else { return; }; - if rest.is_empty() { - return; - } + let mut seen: FxHashMap = FxHashMap::with_capacity_and_hasher(elts.len(), BuildHasherDefault::default()); - seen.insert(ComparableExpr::from(first), 0); - let mut prev = first; - for (index, elt) in rest.iter().enumerate() { - let index = index + 1; - let expr = ComparableExpr::from(elt); + let mut prev = None; + for (index, element) in elts.iter().enumerate() { + let expr = ComparableExpr::from(element); seen.entry(expr) .and_modify(|index| { let mut diagnostic = Diagnostic::new( PytestDuplicateParametrizeTestCases { index: *index }, - elt.range(), + element.range(), ); if checker.patch(diagnostic.kind.rule()) { - let values_end = values.range().sub_end(1.into()).end(); - let elt_end = true_elt_end(checker, elt.end(), values_end); - let prev_end = true_elt_end(checker, prev.end(), values_end); - let deletion_range = TextRange::new(prev_end, elt_end); - if !checker - .indexer() - .comment_ranges() - .intersects(deletion_range) - { - diagnostic.set_fix(Fix::automatic(Edit::range_deletion(deletion_range))); + if let Some(prev) = prev { + let values_end = values.range().end() - TextSize::new(1); + let previous_end = trailing_comma(prev, checker.locator().contents()) + .unwrap_or(values_end); + let element_end = trailing_comma(element, checker.locator().contents()) + .unwrap_or(values_end); + let deletion_range = TextRange::new(previous_end, element_end); + if !checker + .indexer() + .comment_ranges() + .intersects(deletion_range) + { + diagnostic + .set_fix(Fix::suggested(Edit::range_deletion(deletion_range))); + } } } checker.diagnostics.push(diagnostic); }) .or_insert(index); - prev = elt; + prev = Some(element); } } diff --git a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap index 242cbe75e78d7..091cc25bc48ad 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap +++ b/crates/ruff/src/rules/flake8_pytest_style/snapshots/ruff__rules__flake8_pytest_style__tests__PT014.snap @@ -10,7 +10,7 @@ PT014.py:4:35: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.para | = help: Remove duplicate test case -ℹ Fix +ℹ Suggested fix 1 1 | import pytest 2 2 | 3 3 | @@ -29,7 +29,7 @@ PT014.py:14:35: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.par | = help: Remove duplicate test case -ℹ Fix +ℹ Suggested fix 11 11 | c = 3 12 12 | 13 13 | @@ -48,7 +48,7 @@ PT014.py:14:41: PT014 [*] Duplicate of test case at index 2 in `@pytest_mark.par | = help: Remove duplicate test case -ℹ Fix +ℹ Suggested fix 11 11 | c = 3 12 12 | 13 13 | @@ -67,7 +67,7 @@ PT014.py:14:44: PT014 [*] Duplicate of test case at index 2 in `@pytest_mark.par | = help: Remove duplicate test case -ℹ Fix +ℹ Suggested fix 11 11 | c = 3 12 12 | 13 13 | @@ -97,7 +97,7 @@ PT014.py:32:39: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.par | = help: Remove duplicate test case -ℹ Fix +ℹ Suggested fix 29 29 | ... 30 30 | 31 31 | @@ -116,7 +116,7 @@ PT014.py:32:48: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.par | = help: Remove duplicate test case -ℹ Fix +ℹ Suggested fix 29 29 | ... 30 30 | 31 31 | @@ -137,7 +137,7 @@ PT014.py:42:10: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.par | = help: Remove duplicate test case -ℹ Fix +ℹ Suggested fix 39 39 | [ 40 40 | a, 41 41 | b, @@ -157,7 +157,7 @@ PT014.py:44:11: PT014 [*] Duplicate of test case at index 0 in `@pytest_mark.par | = help: Remove duplicate test case -ℹ Fix +ℹ Suggested fix 41 41 | b, 42 42 | (a), 43 43 | c,