From b5edc6dfc96e505c6b5079856166b5236fec9a87 Mon Sep 17 00:00:00 2001 From: Jonathan Plasse <13716151+JonathanPlasse@users.noreply.github.com> Date: Tue, 21 Mar 2023 04:55:23 +0100 Subject: [PATCH] Add autofix functionality for `F523` (#3613) --- .../resources/test/fixtures/pyflakes/F523.py | 1 + crates/ruff/src/cst/matchers.rs | 31 ++- crates/ruff/src/rules/pyflakes/fixes.rs | 179 ++++++++++++------ crates/ruff/src/rules/pyflakes/format.rs | 8 +- .../ruff/src/rules/pyflakes/rules/strings.rs | 28 ++- ..._rules__pyflakes__tests__F523_F523.py.snap | 123 +++++++++--- 6 files changed, 283 insertions(+), 87 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pyflakes/F523.py b/crates/ruff/resources/test/fixtures/pyflakes/F523.py index dcf031658a..75935e4a5f 100644 --- a/crates/ruff/resources/test/fixtures/pyflakes/F523.py +++ b/crates/ruff/resources/test/fixtures/pyflakes/F523.py @@ -4,6 +4,7 @@ "{1:{0}}".format(1, 2) # No issues "{1:{0}}".format(1, 2, 3) # F523 "{0}{2}".format(1, 2) # F523, # F524 +"{1.arg[1]!r:0{2['arg']}{1}}".format(1, 2, 3, 4) # F523 # With no indexes "{}".format(1, 2) # F523 diff --git a/crates/ruff/src/cst/matchers.rs b/crates/ruff/src/cst/matchers.rs index bcbcc742a2..9a5ea83a38 100644 --- a/crates/ruff/src/cst/matchers.rs +++ b/crates/ruff/src/cst/matchers.rs @@ -1,6 +1,7 @@ use anyhow::{bail, Result}; use libcst_native::{ - Call, Comparison, Expr, Expression, Import, ImportFrom, Module, SmallStatement, Statement, + Attribute, Call, Comparison, Dict, Expr, Expression, Import, ImportFrom, Module, SimpleString, + SmallStatement, Statement, }; pub fn match_module(module_text: &str) -> Result { @@ -70,3 +71,31 @@ pub fn match_comparison<'a, 'b>( bail!("Expected Expression::Comparison") } } + +pub fn match_dict<'a, 'b>(expression: &'a mut Expression<'b>) -> Result<&'a mut Dict<'b>> { + if let Expression::Dict(dict) = expression { + Ok(dict) + } else { + bail!("Expected Expression::Dict") + } +} + +pub fn match_attribute<'a, 'b>( + expression: &'a mut Expression<'b>, +) -> Result<&'a mut Attribute<'b>> { + if let Expression::Attribute(attribute) = expression { + Ok(attribute) + } else { + bail!("Expected Expression::Attribute") + } +} + +pub fn match_simple_string<'a, 'b>( + expression: &'a mut Expression<'b>, +) -> Result<&'a mut SimpleString<'b>> { + if let Expression::SimpleString(simple_string) = expression { + Ok(simple_string) + } else { + bail!("Expected Expression::SimpleString") + } +} diff --git a/crates/ruff/src/rules/pyflakes/fixes.rs b/crates/ruff/src/rules/pyflakes/fixes.rs index eaff5c6543..3d5d1cb1a8 100644 --- a/crates/ruff/src/rules/pyflakes/fixes.rs +++ b/crates/ruff/src/rules/pyflakes/fixes.rs @@ -1,5 +1,5 @@ -use anyhow::{bail, Result}; -use libcst_native::{Call, Codegen, CodegenState, Dict, DictElement, Expression}; +use anyhow::{bail, Ok, Result}; +use libcst_native::{Codegen, CodegenState, DictElement, Expression}; use rustpython_parser::ast::{Excepthandler, Expr}; use rustpython_parser::{lexer, Mode, Tok}; @@ -7,8 +7,13 @@ use ruff_diagnostics::Fix; use ruff_python_ast::source_code::{Locator, Stylist}; use ruff_python_ast::str::raw_contents; use ruff_python_ast::types::Range; +use rustpython_common::format::{ + FieldName, FieldNamePart, FieldType, FormatPart, FormatString, FromTemplate, +}; -use crate::cst::matchers::{match_expr, match_module}; +use crate::cst::matchers::{ + match_attribute, match_call, match_dict, match_expression, match_simple_string, +}; /// Generate a [`Fix`] to remove unused keys from format dict. pub fn remove_unused_format_arguments_from_dict( @@ -18,34 +23,15 @@ pub fn remove_unused_format_arguments_from_dict( stylist: &Stylist, ) -> Result { let module_text = locator.slice(stmt); - let mut tree = match_module(module_text)?; - let mut body = match_expr(&mut tree)?; + let mut tree = match_expression(module_text)?; + let dict = match_dict(&mut tree)?; - let new_dict = { - let Expression::Dict(dict) = &body.value else { - bail!("Expected Expression::Dict") - }; - - Dict { - lbrace: dict.lbrace.clone(), - lpar: dict.lpar.clone(), - rbrace: dict.rbrace.clone(), - rpar: dict.rpar.clone(), - elements: dict - .elements - .iter() - .filter_map(|e| match e { - DictElement::Simple { - key: Expression::SimpleString(name), - .. - } if unused_arguments.contains(&raw_contents(name.value)) => None, - e => Some(e.clone()), - }) - .collect(), - } - }; - - body.value = Expression::Dict(Box::new(new_dict)); + dict.elements.retain(|e| { + !matches!(e, DictElement::Simple { + key: Expression::SimpleString(name), + .. + } if unused_arguments.contains(&raw_contents(name.value))) + }); let mut state = CodegenState { default_newline: stylist.line_ending(), @@ -61,7 +47,7 @@ pub fn remove_unused_format_arguments_from_dict( )) } -/// Generate a [`Fix`] to remove unused keyword arguments from format call. +/// Generate a [`Fix`] to remove unused keyword arguments from a `format` call. pub fn remove_unused_keyword_arguments_from_format_call( unused_arguments: &[&str], location: Range, @@ -69,32 +55,119 @@ pub fn remove_unused_keyword_arguments_from_format_call( stylist: &Stylist, ) -> Result { let module_text = locator.slice(location); - let mut tree = match_module(module_text)?; - let mut body = match_expr(&mut tree)?; + let mut tree = match_expression(module_text)?; + let call = match_call(&mut tree)?; - let new_call = { - let Expression::Call(call) = &body.value else { - bail!("Expected Expression::Call") - }; + call.args + .retain(|e| !matches!(&e.keyword, Some(kw) if unused_arguments.contains(&kw.value))); - Call { - func: call.func.clone(), - lpar: call.lpar.clone(), - rpar: call.rpar.clone(), - whitespace_before_args: call.whitespace_before_args.clone(), - whitespace_after_func: call.whitespace_after_func.clone(), - args: call - .args - .iter() - .filter_map(|e| match &e.keyword { - Some(kw) if unused_arguments.contains(&kw.value) => None, - _ => Some(e.clone()), - }) - .collect(), - } + let mut state = CodegenState { + default_newline: stylist.line_ending(), + default_indent: stylist.indentation(), + ..CodegenState::default() }; + tree.codegen(&mut state); - body.value = Expression::Call(Box::new(new_call)); + Ok(Fix::replacement( + state.to_string(), + location.location, + location.end_location, + )) +} + +fn unparse_format_part(format_part: FormatPart) -> String { + match format_part { + FormatPart::Literal(literal) => literal, + FormatPart::Field { + field_name, + conversion_spec, + format_spec, + } => { + let mut field_name = field_name; + if let Some(conversion) = conversion_spec { + field_name.push_str(&format!("!{conversion}")); + } + if !format_spec.is_empty() { + field_name.push_str(&format!(":{format_spec}")); + } + format!("{{{field_name}}}") + } + } +} + +fn update_field_types(format_string: &FormatString, min_unused: usize) -> String { + format_string + .format_parts + .iter() + .map(|part| match part { + FormatPart::Literal(literal) => FormatPart::Literal(literal.to_string()), + FormatPart::Field { + field_name, + conversion_spec, + format_spec, + } => { + let new_field_name = FieldName::parse(field_name).unwrap(); // This should never fail because we parsed it before + let mut new_field_name_string = match new_field_name.field_type { + FieldType::Auto => String::new(), + FieldType::Index(i) => (i - min_unused).to_string(), + FieldType::Keyword(keyword) => keyword, + }; + for field_name_part in &new_field_name.parts { + let field_name_part_string = match field_name_part { + FieldNamePart::Attribute(attribute) => format!(".{attribute}"), + FieldNamePart::Index(i) => format!("[{i}]"), + FieldNamePart::StringIndex(s) => format!("[{s}]"), + }; + new_field_name_string.push_str(&field_name_part_string); + } + let new_format_spec = FormatString::from_str(format_spec).unwrap(); // This should never fail because we parsed it before + let new_format_spec_string = update_field_types(&new_format_spec, min_unused); + FormatPart::Field { + field_name: new_field_name_string, + conversion_spec: *conversion_spec, + format_spec: new_format_spec_string, + } + } + }) + .map(unparse_format_part) + .collect() +} + +/// Generate a [`Fix`] to remove unused positional arguments from a `format` call. +pub fn remove_unused_positional_arguments_from_format_call( + unused_arguments: &[usize], + location: Range, + locator: &Locator, + stylist: &Stylist, + format_string: &FormatString, +) -> Result { + let module_text = locator.slice(location); + let mut tree = match_expression(module_text)?; + let call = match_call(&mut tree)?; + + let mut index = 0; + call.args.retain(|_| { + index += 1; + !unused_arguments.contains(&(index - 1)) + }); + + let mut min_unused_index = 0; + for index in unused_arguments { + if *index == min_unused_index { + min_unused_index += 1; + } else { + break; + } + } + + let mut new_format_string; + if min_unused_index > 0 { + let func = match_attribute(&mut call.func)?; + let simple_string = match_simple_string(&mut func.value)?; + new_format_string = update_field_types(format_string, min_unused_index); + new_format_string = format!(r#""{new_format_string}""#); + simple_string.value = new_format_string.as_str(); + } let mut state = CodegenState { default_newline: stylist.line_ending(), diff --git a/crates/ruff/src/rules/pyflakes/format.rs b/crates/ruff/src/rules/pyflakes/format.rs index c2c49cdd49..766ca1b5be 100644 --- a/crates/ruff/src/rules/pyflakes/format.rs +++ b/crates/ruff/src/rules/pyflakes/format.rs @@ -26,6 +26,7 @@ pub(crate) struct FormatSummary { pub indexes: Vec, pub keywords: Vec, pub has_nested_parts: bool, + pub format_string: FormatString, } impl TryFrom<&str> for FormatSummary { @@ -39,7 +40,7 @@ impl TryFrom<&str> for FormatSummary { let mut keywords = Vec::new(); let mut has_nested_parts = false; - for format_part in format_string.format_parts { + for format_part in &format_string.format_parts { let FormatPart::Field { field_name, format_spec, @@ -47,14 +48,14 @@ impl TryFrom<&str> for FormatSummary { } = format_part else { continue; }; - let parsed = FieldName::parse(&field_name)?; + let parsed = FieldName::parse(field_name)?; match parsed.field_type { FieldType::Auto => autos.push(autos.len()), FieldType::Index(i) => indexes.push(i), FieldType::Keyword(k) => keywords.push(k), }; - let nested = FormatString::from_str(&format_spec)?; + let nested = FormatString::from_str(format_spec)?; for nested_part in nested.format_parts { let FormatPart::Field { field_name, .. } = nested_part else { continue; @@ -74,6 +75,7 @@ impl TryFrom<&str> for FormatSummary { indexes, keywords, has_nested_parts, + format_string, }) } } diff --git a/crates/ruff/src/rules/pyflakes/rules/strings.rs b/crates/ruff/src/rules/pyflakes/rules/strings.rs index b19ea083a5..2171ffe886 100644 --- a/crates/ruff/src/rules/pyflakes/rules/strings.rs +++ b/crates/ruff/src/rules/pyflakes/rules/strings.rs @@ -14,6 +14,7 @@ use crate::registry::AsRule; use super::super::cformat::CFormatSummary; use super::super::fixes::{ remove_unused_format_arguments_from_dict, remove_unused_keyword_arguments_from_format_call, + remove_unused_positional_arguments_from_format_call, }; use super::super::format::FormatSummary; @@ -169,13 +170,19 @@ pub struct StringDotFormatExtraPositionalArguments { pub missing: Vec, } -impl Violation for StringDotFormatExtraPositionalArguments { +impl AlwaysAutofixableViolation for StringDotFormatExtraPositionalArguments { #[derive_message_formats] fn message(&self) -> String { let StringDotFormatExtraPositionalArguments { missing } = self; let message = missing.join(", "); format!("`.format` call has unused arguments at position(s): {message}") } + + fn autofix_title(&self) -> String { + let StringDotFormatExtraPositionalArguments { missing } = self; + let message = missing.join(", "); + format!("Remove extra positional arguments at position(s): {message}") + } } #[violation] @@ -505,7 +512,7 @@ pub(crate) fn string_dot_format_extra_positional_arguments( return; } - checker.diagnostics.push(Diagnostic::new( + let mut diagnostic = Diagnostic::new( StringDotFormatExtraPositionalArguments { missing: missing .iter() @@ -513,7 +520,22 @@ pub(crate) fn string_dot_format_extra_positional_arguments( .collect::>(), }, location, - )); + ); + if checker.patch(diagnostic.kind.rule()) { + match remove_unused_positional_arguments_from_format_call( + &missing, + location, + checker.locator, + checker.stylist, + &summary.format_string, + ) { + Ok(fix) => { + diagnostic.amend(fix); + } + Err(e) => error!("Failed to remove unused positional arguments: {e}"), + } + } + checker.diagnostics.push(diagnostic); } /// F524 diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F523_F523.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F523_F523.py.snap index 8d4b0172e3..1d691a0df0 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F523_F523.py.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F523_F523.py.snap @@ -5,92 +5,161 @@ expression: diagnostics - kind: name: StringDotFormatExtraPositionalArguments body: "`.format` call has unused arguments at position(s): 1" - suggestion: ~ - fixable: false + suggestion: "Remove extra positional arguments at position(s): 1" + fixable: true location: row: 2 column: 0 end_location: row: 2 column: 18 - fix: ~ + fix: + content: "\"{0}\".format(1, )" + location: + row: 2 + column: 0 + end_location: + row: 2 + column: 18 parent: ~ - kind: name: StringDotFormatExtraPositionalArguments body: "`.format` call has unused arguments at position(s): 0, 2" - suggestion: ~ - fixable: false + suggestion: "Remove extra positional arguments at position(s): 0, 2" + fixable: true location: row: 3 column: 0 end_location: row: 3 column: 21 - fix: ~ + fix: + content: "\"{0}\".format(2, )" + location: + row: 3 + column: 0 + end_location: + row: 3 + column: 21 parent: ~ - kind: name: StringDotFormatExtraPositionalArguments body: "`.format` call has unused arguments at position(s): 2" - suggestion: ~ - fixable: false + suggestion: "Remove extra positional arguments at position(s): 2" + fixable: true location: row: 5 column: 0 end_location: row: 5 column: 25 - fix: ~ + fix: + content: "\"{1:{0}}\".format(1, 2, )" + location: + row: 5 + column: 0 + end_location: + row: 5 + column: 25 parent: ~ - kind: name: StringDotFormatExtraPositionalArguments body: "`.format` call has unused arguments at position(s): 1" - suggestion: ~ - fixable: false + suggestion: "Remove extra positional arguments at position(s): 1" + fixable: true location: row: 6 column: 0 end_location: row: 6 column: 21 - fix: ~ + fix: + content: "\"{0}{2}\".format(1, )" + location: + row: 6 + column: 0 + end_location: + row: 6 + column: 21 + parent: ~ +- kind: + name: StringDotFormatExtraPositionalArguments + body: "`.format` call has unused arguments at position(s): 0, 3" + suggestion: "Remove extra positional arguments at position(s): 0, 3" + fixable: true + location: + row: 7 + column: 0 + end_location: + row: 7 + column: 48 + fix: + content: "\"{0.arg[1]!r:0{1['arg']}{0}}\".format(2, 3, )" + location: + row: 7 + column: 0 + end_location: + row: 7 + column: 48 parent: ~ - kind: name: StringDotFormatExtraPositionalArguments body: "`.format` call has unused arguments at position(s): 1" - suggestion: ~ - fixable: false + suggestion: "Remove extra positional arguments at position(s): 1" + fixable: true location: - row: 9 + row: 10 column: 0 end_location: - row: 9 + row: 10 column: 17 - fix: ~ + fix: + content: "\"{}\".format(1, )" + location: + row: 10 + column: 0 + end_location: + row: 10 + column: 17 parent: ~ - kind: name: StringDotFormatExtraPositionalArguments body: "`.format` call has unused arguments at position(s): 1, 2" - suggestion: ~ - fixable: false + suggestion: "Remove extra positional arguments at position(s): 1, 2" + fixable: true location: - row: 10 + row: 11 column: 0 end_location: - row: 10 + row: 11 column: 20 - fix: ~ + fix: + content: "\"{}\".format(1, )" + location: + row: 11 + column: 0 + end_location: + row: 11 + column: 20 parent: ~ - kind: name: StringDotFormatExtraPositionalArguments body: "`.format` call has unused arguments at position(s): 2" - suggestion: ~ - fixable: false + suggestion: "Remove extra positional arguments at position(s): 2" + fixable: true location: - row: 12 + row: 13 column: 0 end_location: - row: 12 + row: 13 column: 23 - fix: ~ + fix: + content: "\"{:{}}\".format(1, 2, )" + location: + row: 13 + column: 0 + end_location: + row: 13 + column: 23 parent: ~