From ac4a4da50ed524b45f162acf2726aea902b2026f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 7 Jun 2023 22:04:35 -0400 Subject: [PATCH] Handle implicit string concatenations in conversion-flag rewrites (#4947) --- .../resources/test/fixtures/ruff/RUF010.py | 6 + crates/ruff/src/cst/matchers.rs | 29 +-- .../explicit_f_string_type_conversion.rs | 166 ++++++++++++------ ..._rules__ruff__tests__RUF010_RUF010.py.snap | 18 ++ 4 files changed, 143 insertions(+), 76 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF010.py b/crates/ruff/resources/test/fixtures/ruff/RUF010.py index f5850115de9f5..77e459c21496a 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF010.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF010.py @@ -28,3 +28,9 @@ def ascii(arg): f"{ascii(bla)}" # OK + +( + f"Member of tuple mismatches type at index {i}. Expected {of_shape_i}. Got " + " intermediary content " + f" that flows {repr(obj)} of type {type(obj)}.{additional_message}" # RUF010 +) diff --git a/crates/ruff/src/cst/matchers.rs b/crates/ruff/src/cst/matchers.rs index 23b6382d14adf..dad1e689eb142 100644 --- a/crates/ruff/src/cst/matchers.rs +++ b/crates/ruff/src/cst/matchers.rs @@ -1,9 +1,8 @@ use anyhow::{bail, Result}; use libcst_native::{ - Arg, Attribute, Call, Comparison, CompoundStatement, Dict, Expression, FormattedString, - FormattedStringContent, FormattedStringExpression, FunctionDef, GeneratorExp, If, Import, - ImportAlias, ImportFrom, ImportNames, IndentedBlock, Lambda, ListComp, Module, Name, - SmallStatement, Statement, Suite, Tuple, With, + Arg, Attribute, Call, Comparison, CompoundStatement, Dict, Expression, FunctionDef, + GeneratorExp, If, Import, ImportAlias, ImportFrom, ImportNames, IndentedBlock, Lambda, + ListComp, Module, Name, SmallStatement, Statement, Suite, Tuple, With, }; pub(crate) fn match_module(module_text: &str) -> Result { @@ -109,28 +108,6 @@ pub(crate) fn match_attribute<'a, 'b>( } } -pub(crate) fn match_formatted_string<'a, 'b>( - expression: &'a mut Expression<'b>, -) -> Result<&'a mut FormattedString<'b>> { - if let Expression::FormattedString(formatted_string) = expression { - Ok(formatted_string) - } else { - bail!("Expected Expression::FormattedString") - } -} - -pub(crate) fn match_formatted_string_expression<'a, 'b>( - formatted_string_content: &'a mut FormattedStringContent<'b>, -) -> Result<&'a mut FormattedStringExpression<'b>> { - if let FormattedStringContent::Expression(formatted_string_expression) = - formatted_string_content - { - Ok(formatted_string_expression) - } else { - bail!("Expected FormattedStringContent::Expression") - } -} - pub(crate) fn match_name<'a, 'b>(expression: &'a Expression<'b>) -> Result<&'a Name<'b>> { if let Expression::Name(name) = expression { Ok(name) diff --git a/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs b/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs index 18e93ab3c6a07..b41e8ae77b7ae 100644 --- a/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs +++ b/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs @@ -1,16 +1,16 @@ use anyhow::{bail, Result}; +use libcst_native::{ + ConcatenatedString, Expression, FormattedStringContent, FormattedStringExpression, +}; use rustpython_parser::ast::{self, Expr, Ranged}; -use crate::autofix::codemods::CodegenStylist; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::source_code::{Locator, Stylist}; +use crate::autofix::codemods::CodegenStylist; use crate::checkers::ast::Checker; -use crate::cst::matchers::{ - match_call_mut, match_expression, match_formatted_string, match_formatted_string_expression, - match_name, -}; +use crate::cst::matchers::{match_call_mut, match_expression, match_name}; use crate::registry::AsRule; /// ## What it does @@ -46,57 +46,26 @@ impl AlwaysAutofixableViolation for ExplicitFStringTypeConversion { } } -fn fix_explicit_f_string_type_conversion( - expr: &Expr, - index: usize, - locator: &Locator, - stylist: &Stylist, -) -> Result { - // Replace the call node with its argument and a conversion flag. - let range = expr.range(); - let content = locator.slice(range); - let mut expression = match_expression(content)?; - let formatted_string = match_formatted_string(&mut expression)?; - - // Replace the formatted call expression at `index` with a conversion flag. - let formatted_string_expression = - match_formatted_string_expression(&mut formatted_string.parts[index])?; - let call = match_call_mut(&mut formatted_string_expression.expression)?; - let name = match_name(&call.func)?; - match name.value { - "str" => { - formatted_string_expression.conversion = Some("s"); - } - "repr" => { - formatted_string_expression.conversion = Some("r"); - } - "ascii" => { - formatted_string_expression.conversion = Some("a"); - } - _ => bail!("Unexpected function call: `{:?}`", name.value), - } - formatted_string_expression.expression = call.args[0].value.clone(); - - Ok(Fix::automatic(Edit::range_replacement( - expression.codegen_stylist(stylist), - range, - ))) -} - /// RUF010 pub(crate) fn explicit_f_string_type_conversion( checker: &mut Checker, expr: &Expr, values: &[Expr], ) { - for (index, formatted_value) in values.iter().enumerate() { - let Expr::FormattedValue(ast::ExprFormattedValue { - conversion, - value, - .. - }) = &formatted_value else { - continue; - }; + for (index, formatted_value) in values + .iter() + .filter_map(|expr| { + if let Expr::FormattedValue(expr) = &expr { + Some(expr) + } else { + None + } + }) + .enumerate() + { + let ast::ExprFormattedValue { + value, conversion, .. + } = formatted_value; // Skip if there's already a conversion flag. if !conversion.is_none() { @@ -138,3 +107,100 @@ pub(crate) fn explicit_f_string_type_conversion( checker.diagnostics.push(diagnostic); } } + +/// Generate a [`Fix`] to replace an explicit type conversion with a conversion flag. +fn fix_explicit_f_string_type_conversion( + expr: &Expr, + index: usize, + locator: &Locator, + stylist: &Stylist, +) -> Result { + // Parenthesize the expression, to support implicit concatenation. + let range = expr.range(); + let content = locator.slice(range); + let parenthesized_content = format!("({content})"); + let mut expression = match_expression(&parenthesized_content)?; + + // Replace the formatted call expression at `index` with a conversion flag. + let mut formatted_string_expression = match_part(index, &mut expression)?; + let call = match_call_mut(&mut formatted_string_expression.expression)?; + let name = match_name(&call.func)?; + match name.value { + "str" => { + formatted_string_expression.conversion = Some("s"); + } + "repr" => { + formatted_string_expression.conversion = Some("r"); + } + "ascii" => { + formatted_string_expression.conversion = Some("a"); + } + _ => bail!("Unexpected function call: `{:?}`", name.value), + } + formatted_string_expression.expression = call.args[0].value.clone(); + + // Remove the parentheses (first and last characters). + let mut content = expression.codegen_stylist(stylist); + content.remove(0); + content.pop(); + + Ok(Fix::automatic(Edit::range_replacement(content, range))) +} + +/// Return the [`FormattedStringContent`] at the given index in an f-string or implicit +/// string concatenation. +fn match_part<'a, 'b>( + index: usize, + expr: &'a mut Expression<'b>, +) -> Result<&'a mut FormattedStringExpression<'b>> { + match expr { + Expression::ConcatenatedString(expr) => Ok(collect_parts(expr).remove(index)), + Expression::FormattedString(expr) => { + // Find the formatted expression at the given index. The `parts` field contains a mix + // of string literals and expressions, but our `index` only counts expressions. All + // the boxing and mutability makes this difficult to write in a functional style. + let mut format_index = 0; + for part in &mut expr.parts { + if let FormattedStringContent::Expression(expr) = part { + if format_index == index { + return Ok(expr); + } + format_index += 1; + } + } + + bail!("Index out of bounds: `{index}`") + } + _ => bail!("Unexpected expression: `{:?}`", expr), + } +} + +/// Given an implicit string concatenation, return a list of all the formatted expressions. +fn collect_parts<'a, 'b>( + expr: &'a mut ConcatenatedString<'b>, +) -> Vec<&'a mut FormattedStringExpression<'b>> { + fn inner<'a, 'b>( + string: &'a mut libcst_native::String<'b>, + formatted_expressions: &mut Vec<&'a mut FormattedStringExpression<'b>>, + ) { + match string { + libcst_native::String::Formatted(expr) => { + for part in &mut expr.parts { + if let FormattedStringContent::Expression(expr) = part { + formatted_expressions.push(expr); + } + } + } + libcst_native::String::Concatenated(expr) => { + inner(&mut expr.left, formatted_expressions); + inner(&mut expr.right, formatted_expressions); + } + libcst_native::String::Simple(_) => {} + } + } + + let mut formatted_expressions = vec![]; + inner(&mut expr.left, &mut formatted_expressions); + inner(&mut expr.right, &mut formatted_expressions); + formatted_expressions +} diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF010_RUF010.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF010_RUF010.py.snap index 57f269e8dcccb..4a82c0546383a 100644 --- a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF010_RUF010.py.snap +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF010_RUF010.py.snap @@ -226,4 +226,22 @@ RUF010.py:15:29: RUF010 [*] Use conversion in f-string 17 17 | f"{foo(bla)}" # OK 18 18 | +RUF010.py:35:20: RUF010 [*] Use conversion in f-string + | +35 | f"Member of tuple mismatches type at index {i}. Expected {of_shape_i}. Got " +36 | " intermediary content " +37 | f" that flows {repr(obj)} of type {type(obj)}.{additional_message}" # RUF010 + | ^^^^^^^^^ RUF010 +38 | ) + | + = help: Replace f-string function call with conversion + +ℹ Fix +32 32 | ( +33 33 | f"Member of tuple mismatches type at index {i}. Expected {of_shape_i}. Got " +34 34 | " intermediary content " +35 |- f" that flows {repr(obj)} of type {type(obj)}.{additional_message}" # RUF010 + 35 |+ f" that flows {obj!r} of type {type(obj)}.{additional_message}" # RUF010 +36 36 | ) +