Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle implicit string concatenations in conversion-flag rewrites #4947

Merged
merged 1 commit into from
Jun 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions crates/ruff/resources/test/fixtures/ruff/RUF010.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
29 changes: 3 additions & 26 deletions crates/ruff/src/cst/matchers.rs
Original file line number Diff line number Diff line change
@@ -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<Module> {
Expand Down Expand Up @@ -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)
Expand Down
166 changes: 116 additions & 50 deletions crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -46,57 +46,26 @@ impl AlwaysAutofixableViolation for ExplicitFStringTypeConversion {
}
}

fn fix_explicit_f_string_type_conversion(
expr: &Expr,
index: usize,
locator: &Locator,
stylist: &Stylist,
) -> Result<Fix> {
// 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() {
Expand Down Expand Up @@ -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<Fix> {
// 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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 | )