From 03fb62c1747feb158fa644e7880f0d9e76b2382f Mon Sep 17 00:00:00 2001 From: Jonathan Plasse <13716151+JonathanPlasse@users.noreply.github.com> Date: Fri, 19 May 2023 21:05:51 +0200 Subject: [PATCH] Fix RUF010 auto-fix with parenthesis (#4524) --- .../resources/test/fixtures/ruff/RUF010.py | 2 + crates/ruff/src/checkers/ast/mod.rs | 18 +-- crates/ruff/src/cst/matchers.rs | 35 +++- .../explicit_f_string_type_conversion.rs | 151 ++++++++++++------ ..._rules__ruff__tests__RUF010_RUF010.py.snap | 75 ++++++++- 5 files changed, 211 insertions(+), 70 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF010.py b/crates/ruff/resources/test/fixtures/ruff/RUF010.py index cc3e9c7831..2d2604f9dc 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF010.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF010.py @@ -10,6 +10,8 @@ f"{str(bla)}, {repr(bla)}, {ascii(bla)}" # RUF010 f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 +f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 + f"{foo(bla)}" # OK f"{str(bla, 'ascii')}, {str(bla, encoding='cp1255')}" # OK diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 15f6a24085..08d1012f0b 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -3360,6 +3360,13 @@ where if self.settings.rules.enabled(Rule::HardcodedSQLExpression) { flake8_bandit::rules::hardcoded_sql_expression(self, expr); } + if self + .settings + .rules + .enabled(Rule::ExplicitFStringTypeConversion) + { + ruff::rules::explicit_f_string_type_conversion(self, expr, values); + } } Expr::BinOp(ast::ExprBinOp { left, @@ -3859,17 +3866,6 @@ where flake8_simplify::rules::expr_and_false(self, expr); } } - Expr::FormattedValue(ast::ExprFormattedValue { - value, conversion, .. - }) => { - if self - .settings - .rules - .enabled(Rule::ExplicitFStringTypeConversion) - { - ruff::rules::explicit_f_string_type_conversion(self, value, *conversion); - } - } _ => {} }; diff --git a/crates/ruff/src/cst/matchers.rs b/crates/ruff/src/cst/matchers.rs index 00baadea98..d24c0580d8 100644 --- a/crates/ruff/src/cst/matchers.rs +++ b/crates/ruff/src/cst/matchers.rs @@ -1,7 +1,8 @@ use anyhow::{bail, Result}; use libcst_native::{ - Attribute, Call, Comparison, Dict, Expr, Expression, Import, ImportAlias, ImportFrom, - ImportNames, Module, SimpleString, SmallStatement, Statement, + Attribute, Call, Comparison, Dict, Expr, Expression, FormattedString, FormattedStringContent, + FormattedStringExpression, Import, ImportAlias, ImportFrom, ImportNames, Module, Name, + SimpleString, SmallStatement, Statement, }; pub(crate) fn match_module(module_text: &str) -> Result { @@ -111,3 +112,33 @@ pub(crate) fn match_simple_string<'a, 'b>( bail!("Expected Expression::SimpleString") } } + +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 mut Expression<'b>) -> Result<&'a mut Name<'b>> { + if let Expression::Name(name) = expression { + Ok(name) + } else { + bail!("Expected Expression::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 219176d49e..4fd1be8b4b 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,9 +1,16 @@ -use rustpython_parser::ast::{self, ConversionFlag, Expr, Ranged}; +use anyhow::{bail, Result}; +use libcst_native::{Codegen, CodegenState}; +use rustpython_parser::ast::{self, Expr, Ranged}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::source_code::{Locator, Stylist}; use crate::checkers::ast::Checker; +use crate::cst::matchers::{ + match_call, match_expression, match_formatted_string, match_formatted_string_expression, + match_name, +}; use crate::registry::AsRule; /// ## What it does @@ -39,59 +46,101 @@ 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 mut formatted_string_expression = + match_formatted_string_expression(&mut formatted_string.parts[index])?; + let call = match_call(&mut formatted_string_expression.expression)?; + let name = match_name(&mut 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(); + + let mut state = CodegenState { + default_newline: &stylist.line_ending(), + default_indent: stylist.indentation(), + ..CodegenState::default() + }; + expression.codegen(&mut state); + + Ok(Fix::automatic(Edit::range_replacement( + state.to_string(), + range, + ))) +} + /// RUF010 pub(crate) fn explicit_f_string_type_conversion( checker: &mut Checker, - formatted_value: &Expr, - conversion: ConversionFlag, + expr: &Expr, + values: &[Expr], ) { - // Skip if there's already a conversion flag. - if !conversion.is_none() { - return; + for (index, formatted_value) in values.iter().enumerate() { + let Expr::FormattedValue(ast::ExprFormattedValue { + conversion, + value, + .. + }) = &formatted_value else { + continue; + }; + // Skip if there's already a conversion flag. + if !conversion.is_none() { + return; + } + + let Expr::Call(ast::ExprCall { + func, + args, + keywords, + .. + }) = value.as_ref() else { + return; + }; + + // Can't be a conversion otherwise. + if args.len() != 1 || !keywords.is_empty() { + return; + } + + let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + return; + }; + + if !matches!(id.as_str(), "str" | "repr" | "ascii") { + return; + }; + + if !checker.ctx.is_builtin(id) { + return; + } + + let mut diagnostic = Diagnostic::new(ExplicitFStringTypeConversion, value.range()); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.try_set_fix(|| { + fix_explicit_f_string_type_conversion(expr, index, checker.locator, checker.stylist) + }); + } + checker.diagnostics.push(diagnostic); } - - let Expr::Call(ast::ExprCall { - func, - args, - keywords, - range: _, - }) = formatted_value else { - return; - }; - - // Can't be a conversion otherwise. - if args.len() != 1 || !keywords.is_empty() { - return; - } - - let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { - return; - }; - - let conversion = match id.as_str() { - "ascii" => 'a', - "str" => 's', - "repr" => 'r', - _ => return, - }; - - if !checker.ctx.is_builtin(id) { - return; - } - - let formatted_value_range = formatted_value.range(); - let mut diagnostic = Diagnostic::new(ExplicitFStringTypeConversion, formatted_value_range); - - if checker.patch(diagnostic.kind.rule()) { - let arg_range = args[0].range(); - let remove_call = Edit::deletion(formatted_value_range.start(), arg_range.start()); - let add_conversion = Edit::replacement( - format!("!{conversion}"), - arg_range.end(), - formatted_value_range.end(), - ); - diagnostic.set_fix(Fix::automatic_edits(remove_call, [add_conversion])); - } - - checker.diagnostics.push(diagnostic); } 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 26fc6ede22..df895039e7 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 @@ -65,7 +65,7 @@ RUF010.py:11:4: RUF010 [*] Use conversion in f-string 13 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 | ^^^^^^^^^^^ RUF010 14 | -15 | f"{foo(bla)}" # OK +15 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 | = help: Replace f-string function call with conversion @@ -76,7 +76,7 @@ RUF010.py:11:4: RUF010 [*] Use conversion in f-string 11 |-f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 11 |+f"{d['a']!s}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 12 12 | -13 13 | f"{foo(bla)}" # OK +13 13 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 14 14 | RUF010.py:11:19: RUF010 [*] Use conversion in f-string @@ -86,7 +86,7 @@ RUF010.py:11:19: RUF010 [*] Use conversion in f-string 13 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 | ^^^^^^^^^^^^ RUF010 14 | -15 | f"{foo(bla)}" # OK +15 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 | = help: Replace f-string function call with conversion @@ -97,7 +97,7 @@ RUF010.py:11:19: RUF010 [*] Use conversion in f-string 11 |-f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 11 |+f"{str(d['a'])}, {d['b']!r}, {ascii(d['c'])}" # RUF010 12 12 | -13 13 | f"{foo(bla)}" # OK +13 13 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 14 14 | RUF010.py:11:35: RUF010 [*] Use conversion in f-string @@ -107,7 +107,7 @@ RUF010.py:11:35: RUF010 [*] Use conversion in f-string 13 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 | ^^^^^^^^^^^^^ RUF010 14 | -15 | f"{foo(bla)}" # OK +15 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 | = help: Replace f-string function call with conversion @@ -118,7 +118,70 @@ RUF010.py:11:35: RUF010 [*] Use conversion in f-string 11 |-f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 11 |+f"{str(d['a'])}, {repr(d['b'])}, {d['c']!a}" # RUF010 12 12 | -13 13 | f"{foo(bla)}" # OK +13 13 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 14 14 | +RUF010.py:13:5: RUF010 [*] Use conversion in f-string + | +13 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 +14 | +15 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 + | ^^^^^^^^ RUF010 +16 | +17 | f"{foo(bla)}" # OK + | + = help: Replace f-string function call with conversion + +ℹ Fix +10 10 | +11 11 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 +12 12 | +13 |-f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 + 13 |+f"{bla!s}, {(repr(bla))}, {(ascii(bla))}" # RUF010 +14 14 | +15 15 | f"{foo(bla)}" # OK +16 16 | + +RUF010.py:13:19: RUF010 [*] Use conversion in f-string + | +13 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 +14 | +15 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 + | ^^^^^^^^^ RUF010 +16 | +17 | f"{foo(bla)}" # OK + | + = help: Replace f-string function call with conversion + +ℹ Fix +10 10 | +11 11 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 +12 12 | +13 |-f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 + 13 |+f"{(str(bla))}, {bla!r}, {(ascii(bla))}" # RUF010 +14 14 | +15 15 | f"{foo(bla)}" # OK +16 16 | + +RUF010.py:13:34: RUF010 [*] Use conversion in f-string + | +13 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 +14 | +15 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 + | ^^^^^^^^^^ RUF010 +16 | +17 | f"{foo(bla)}" # OK + | + = help: Replace f-string function call with conversion + +ℹ Fix +10 10 | +11 11 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 +12 12 | +13 |-f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 + 13 |+f"{(str(bla))}, {(repr(bla))}, {bla!a}" # RUF010 +14 14 | +15 15 | f"{foo(bla)}" # OK +16 16 | +