From 9792a01df1a634b206e8722f146eaa933bfc4e51 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Mon, 9 Dec 2024 22:26:23 -0600 Subject: [PATCH 1/5] first attempt at delete and replace around comments --- crates/ruff_linter/src/fix/edits.rs | 81 ++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/fix/edits.rs b/crates/ruff_linter/src/fix/edits.rs index e7de716f9fa49..3d03a238baa34 100644 --- a/crates/ruff_linter/src/fix/edits.rs +++ b/crates/ruff_linter/src/fix/edits.rs @@ -2,6 +2,7 @@ use anyhow::{Context, Result}; +use itertools::Itertools; use ruff_diagnostics::Edit; use ruff_python_ast::parenthesize::parenthesized_range; use ruff_python_ast::{self as ast, Arguments, ExceptHandler, Expr, ExprList, Parameters, Stmt}; @@ -10,8 +11,8 @@ use ruff_python_codegen::Stylist; use ruff_python_index::Indexer; use ruff_python_trivia::textwrap::dedent_to; use ruff_python_trivia::{ - has_leading_content, is_python_whitespace, CommentRanges, PythonWhitespace, SimpleTokenKind, - SimpleTokenizer, + has_leading_content, indentation_at_offset, is_python_whitespace, CommentRanges, + PythonWhitespace, SimpleTokenKind, SimpleTokenizer, }; use ruff_source_file::{LineRanges, NewlineWithTrailingNewline, UniversalNewlines}; use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; @@ -107,6 +108,82 @@ pub(crate) fn delete_comment(range: TextRange, locator: &Locator) -> Edit { } } +pub(crate) fn delete_around_comments( + start: TextSize, + end: TextSize, + comment_ranges: &CommentRanges, + source: &str, +) -> Vec { + let delete_range = TextRange::new(start, end); + // Begin by restricting attention to the comment ranges + // that intersect the deletion range. Suppose it looks like: + // + // (s1,e1),(s2,e2),...,(sn,en) + // + // Flatten, then prepend and append by the start and + // end of the deletion range to get: + // + // s0, s1, e1, s2, ..., sn, en, e0 + // + // Now pair up to get the desired deletion ranges + // in the complement of the comments: + // + // (s0,s1), (e1,s2), ..., (en,e0) + let mut edits: Vec = std::iter::once(start) + .chain( + comment_ranges + .iter() + .filter(|range| range.intersect(delete_range).is_some()) + // extend comment range to include last newline + .flat_map(|range| [range.start(), range.end() + TextSize::from(1)]), + ) + .chain(std::iter::once(end)) + // Here we clamp the comment intervals to the deletion range + // (somewhat awkwardly: each conditional branch triggers + // at most once, and the remaining is a no-op.) + .map(|locn| { + if locn < start { + return start; + } + if locn > end { + return end; + } + locn + }) + .tuples::<(_, _)>() + .map(|(left, right)| Edit::deletion(left, right)) + .collect(); + // Adjust the last deletion so remaining content matches the + // indentation at start of original deletion range. + let Some(last_edit) = edits.last_mut() else { + return edits; + }; + let Some(start_indent) = indentation_at_offset(start, source) else { + return edits; + }; + if !start_indent.is_empty() { + *last_edit = Edit::range_replacement(start_indent.to_string(), last_edit.range()); + }; + edits +} + +pub(crate) fn replace_around_comments( + content: &str, + start: TextSize, + end: TextSize, + comment_ranges: &CommentRanges, + source: &str, +) -> Vec { + let mut edits = delete_around_comments(start, end, comment_ranges, source); + let Some(last_edit) = edits.last_mut() else { + return edits; + }; + *last_edit = Edit::range_replacement( + format!("{}{}", last_edit.content().unwrap_or_default(), content), + last_edit.range(), + ); + edits +} /// Generate a `Fix` to remove the specified imports from an `import` statement. pub(crate) fn remove_unused_imports<'a>( member_names: impl Iterator, From a8b6be4947c9d3e7340f30d766ddc1c121b570a9 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Mon, 9 Dec 2024 22:27:19 -0600 Subject: [PATCH 2/5] implement in a couple rules --- .../unnecessary_literal_within_list_call.rs | 29 +++++++++++++++---- .../unnecessary_literal_within_tuple_call.rs | 15 +++++++--- ...8_comprehensions__tests__C409_C409.py.snap | 9 +++--- ...8_comprehensions__tests__C410_C410.py.snap | 9 +++--- ...ensions__tests__preview__C409_C409.py.snap | 9 +++--- 5 files changed, 50 insertions(+), 21 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs index 7dbc47496bb2e..a1060885379b0 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs @@ -1,3 +1,4 @@ +use crate::fix::edits::delete_around_comments; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::{self as ast, Expr}; @@ -89,10 +90,20 @@ pub(crate) fn unnecessary_literal_within_list_call(checker: &mut Checker, call: // Convert `list([1, 2])` to `[1, 2]` let fix = { // Delete from the start of the call to the start of the argument. - let call_start = Edit::deletion(call.start(), argument.start()); + let mut call_start_edits = delete_around_comments( + call.start(), + argument.start(), + &checker.comment_ranges(), + checker.locator().contents(), + ); // Delete from the end of the argument to the end of the call. - let call_end = Edit::deletion(argument.end(), call.end()); + let call_end_edits = delete_around_comments( + argument.end(), + call.end(), + &checker.comment_ranges(), + checker.locator().contents(), + ); // If this is a tuple, we also need to convert the inner argument to a list. if argument.is_tuple_expr() { @@ -109,10 +120,18 @@ pub(crate) fn unnecessary_literal_within_list_call(checker: &mut Checker, call: argument.end() - TextSize::from(1), argument.end(), ); - - Fix::unsafe_edits(call_start, [argument_start, argument_end, call_end]) + call_start_edits.extend( + std::iter::once(argument_start) + .chain(std::iter::once(argument_end)) + .chain(call_end_edits), + ); + let (edit, rest) = call_start_edits.split_first().unwrap(); + Fix::unsafe_edits(edit.clone(), rest.to_owned()) } else { - Fix::unsafe_edits(call_start, [call_end]) + call_start_edits.extend(call_end_edits); + + let (edit, rest) = call_start_edits.split_first().unwrap(); + Fix::unsafe_edits(edit.clone(), rest.to_owned()) } }; diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.rs index 0cec64136ce4c..28a1b2092c3bd 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.rs @@ -1,4 +1,4 @@ -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::any_over_expr; use ruff_python_ast::{self as ast, Expr}; @@ -6,6 +6,7 @@ use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::checkers::ast::Checker; +use crate::fix::edits::replace_around_comments; use crate::rules::flake8_comprehensions::fixes; use super::helpers; @@ -129,13 +130,15 @@ pub(crate) fn unnecessary_literal_within_tuple_call( }; // Replace `[` with `(`. - let elt_start = Edit::replacement( + let mut elt_start = replace_around_comments( "(".into(), call.start(), argument.start() + TextSize::from(1), + &checker.comment_ranges(), + checker.locator().contents(), ); // Replace `]` with `)` or `,)`. - let elt_end = Edit::replacement( + let elt_end = replace_around_comments( if needs_trailing_comma { ",)".into() } else { @@ -143,8 +146,12 @@ pub(crate) fn unnecessary_literal_within_tuple_call( }, argument.end() - TextSize::from(1), call.end(), + &checker.comment_ranges(), + checker.locator().contents(), ); - Fix::unsafe_edits(elt_start, [elt_end]) + elt_start.extend(elt_end); + let (edit, rest) = elt_start.split_first().unwrap(); + Fix::unsafe_edits(edit.clone(), rest.to_owned()) }); } diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap index b432b42187d35..8a5c2162ad957 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap @@ -130,10 +130,11 @@ C409.py:12:1: C409 [*] Unnecessary list literal passed to `tuple()` (rewrite as 12 |-tuple( # comment 13 |- [1, 2] 14 |-) - 12 |+(1, 2) -15 13 | -16 14 | tuple([ # comment -17 15 | 1, 2 + 12 |+# comment + 13 |+(1, 2) +15 14 | +16 15 | tuple([ # comment +17 16 | 1, 2 C409.py:16:1: C409 [*] Unnecessary list literal passed to `tuple()` (rewrite as a tuple literal) | diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C410_C410.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C410_C410.py.snap index f33a4ed1ffd2f..f0cc14e153afb 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C410_C410.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C410_C410.py.snap @@ -91,10 +91,11 @@ C410.py:7:1: C410 [*] Unnecessary list literal passed to `list()` (remove the ou 7 |-list( # comment 8 |- [1, 2] 9 |-) - 7 |+[1, 2] -10 8 | -11 9 | list([ # comment -12 10 | 1, 2 + 7 |+# comment + 8 |+[1, 2] +10 9 | +11 10 | list([ # comment +12 11 | 1, 2 C410.py:11:1: C410 [*] Unnecessary list literal passed to `list()` (remove the outer call to `list()`) | diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C409_C409.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C409_C409.py.snap index 0b220bdcf385c..6c00bc59d5820 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C409_C409.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C409_C409.py.snap @@ -130,10 +130,11 @@ C409.py:12:1: C409 [*] Unnecessary list literal passed to `tuple()` (rewrite as 12 |-tuple( # comment 13 |- [1, 2] 14 |-) - 12 |+(1, 2) -15 13 | -16 14 | tuple([ # comment -17 15 | 1, 2 + 12 |+# comment + 13 |+(1, 2) +15 14 | +16 15 | tuple([ # comment +17 16 | 1, 2 C409.py:16:1: C409 [*] Unnecessary list literal passed to `tuple()` (rewrite as a tuple literal) | From 47dcb08b837e92abd0b1ea51a49483555709a81f Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Mon, 9 Dec 2024 22:43:31 -0600 Subject: [PATCH 3/5] clippy --- .../rules/unnecessary_literal_within_list_call.rs | 4 ++-- .../rules/unnecessary_literal_within_tuple_call.rs | 12 ++++-------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs index a1060885379b0..6ad3f7bbe0cec 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs @@ -93,7 +93,7 @@ pub(crate) fn unnecessary_literal_within_list_call(checker: &mut Checker, call: let mut call_start_edits = delete_around_comments( call.start(), argument.start(), - &checker.comment_ranges(), + checker.comment_ranges(), checker.locator().contents(), ); @@ -101,7 +101,7 @@ pub(crate) fn unnecessary_literal_within_list_call(checker: &mut Checker, call: let call_end_edits = delete_around_comments( argument.end(), call.end(), - &checker.comment_ranges(), + checker.comment_ranges(), checker.locator().contents(), ); diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.rs index 28a1b2092c3bd..8b96c59e6af3b 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.rs @@ -131,22 +131,18 @@ pub(crate) fn unnecessary_literal_within_tuple_call( // Replace `[` with `(`. let mut elt_start = replace_around_comments( - "(".into(), + "(", call.start(), argument.start() + TextSize::from(1), - &checker.comment_ranges(), + checker.comment_ranges(), checker.locator().contents(), ); // Replace `]` with `)` or `,)`. let elt_end = replace_around_comments( - if needs_trailing_comma { - ",)".into() - } else { - ")".into() - }, + if needs_trailing_comma { ",)" } else { ")" }, argument.end() - TextSize::from(1), call.end(), - &checker.comment_ranges(), + checker.comment_ranges(), checker.locator().contents(), ); elt_start.extend(elt_end); From 7a47c4da06cd6b4280c5522b8c65bb32852b15fd Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Mon, 9 Dec 2024 23:13:00 -0600 Subject: [PATCH 4/5] implement for up013 --- .../convert_typed_dict_functional_to_class.rs | 37 ++++++++++--------- ...er__rules__pyupgrade__tests__UP013.py.snap | 7 ++-- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs index ef3e3768e4913..e99b1dce9b783 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs @@ -1,4 +1,4 @@ -use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::is_dunder; use ruff_python_ast::{self as ast, Arguments, Expr, ExprContext, Identifier, Keyword, Stmt}; @@ -10,6 +10,7 @@ use ruff_source_file::LineRanges; use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; +use crate::fix::edits::replace_around_comments; /// ## What it does /// Checks for `TypedDict` declarations that use functional syntax. @@ -98,6 +99,7 @@ pub(crate) fn convert_typed_dict_functional_to_class( base_class, checker.generator(), checker.comment_ranges(), + checker.locator().contents(), )); } checker.diagnostics.push(diagnostic); @@ -273,21 +275,22 @@ fn convert_to_class( base_class: &Expr, generator: Generator, comment_ranges: &CommentRanges, + source: &str, ) -> Fix { - Fix::applicable_edit( - Edit::range_replacement( - generator.stmt(&create_class_def_stmt( - class_name, - body, - total_keyword, - base_class, - )), - stmt.range(), - ), - if comment_ranges.intersects(stmt.range()) { - Applicability::Unsafe - } else { - Applicability::Safe - }, - ) + let edits = replace_around_comments( + &generator.stmt(&create_class_def_stmt( + class_name, + body, + total_keyword, + base_class, + )), + stmt.start(), + stmt.end(), + comment_ranges, + source, + ); + let Some((edit, rest)) = edits.split_first() else { + panic!() + }; + Fix::safe_edits(edit.clone(), rest.to_owned()) } diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP013.py.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP013.py.snap index 0e03e843a1251..1796a57b1cf0f 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP013.py.snap +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP013.py.snap @@ -267,12 +267,13 @@ UP013.py:46:1: UP013 [*] Convert `X` from `TypedDict` functional to class syntax | = help: Convert `X` to class syntax -ℹ Unsafe fix +ℹ Safe fix 43 43 | MyType = TypedDict("MyType", dict()) 44 44 | 45 45 | # Unsafe fix if comments are present 46 |-X = TypedDict("X", { 47 |- "some_config": int, # important 48 |-}) - 46 |+class X(TypedDict): - 47 |+ some_config: int + 46 |+# important + 47 |+class X(TypedDict): + 48 |+ some_config: int From 8e2ab41288436e5542ea00815e8c71480ebd0238 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Mon, 9 Dec 2024 23:15:13 -0600 Subject: [PATCH 5/5] temporarily ignore clippy --- .../pyupgrade/rules/convert_typed_dict_functional_to_class.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs index e99b1dce9b783..1c57ba988085c 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/convert_typed_dict_functional_to_class.rs @@ -267,6 +267,7 @@ fn match_fields_and_total(arguments: &Arguments) -> Option<(Vec, Option<&K } /// Generate a `Fix` to convert a `TypedDict` from functional to class. +#[allow(clippy::too_many_arguments)] fn convert_to_class( stmt: &Stmt, class_name: &str,