From 83319f12c5921ed81e8d81a6fcf1a15c1290ed7a Mon Sep 17 00:00:00 2001 From: colin99d Date: Sun, 15 Jan 2023 16:45:59 -0500 Subject: [PATCH 01/26] Layed a foundation --- resources/test/fixtures/pyupgrade/UP032.py | 1 + src/checkers/ast.rs | 4 ++++ src/registry.rs | 1 + src/rules/pyupgrade/rules/f_strings.rs | 7 +++++++ src/rules/pyupgrade/rules/mod.rs | 2 ++ src/violations.rs | 17 +++++++++++++++++ 6 files changed, 32 insertions(+) create mode 100644 resources/test/fixtures/pyupgrade/UP032.py create mode 100644 src/rules/pyupgrade/rules/f_strings.rs diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py new file mode 100644 index 00000000000000..4d919336a0223c --- /dev/null +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -0,0 +1 @@ +'{foo} {bar}'.format(foo=foo, bar=bar) diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index ce6feaf9f87347..c95ca00b968563 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -1909,6 +1909,10 @@ where if self.settings.enabled.contains(&RuleCode::UP030) { pyupgrade::rules::format_literals(self, &summary, expr); } + + if self.settings.enabled.contains(&RuleCode::UP032) { + pyupgrade::rules::f_strings(self, &summary, expr); + } } } } diff --git a/src/registry.rs b/src/registry.rs index f1ab10f61b0ce8..2737340abeb784 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -254,6 +254,7 @@ ruff_macros::define_rule_mapping!( UP028 => violations::RewriteYieldFrom, UP029 => violations::UnnecessaryBuiltinImport, UP030 => violations::FormatLiterals, + UP032 => violations::FString, // pydocstyle D100 => violations::PublicModule, D101 => violations::PublicClass, diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs new file mode 100644 index 00000000000000..acea980b4f6369 --- /dev/null +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -0,0 +1,7 @@ +use crate::rules::pyflakes::format::FormatSummary; +use crate::checkers::ast::Checker; +use rustpython_ast::Expr; + +/// UP032 +pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) { +} diff --git a/src/rules/pyupgrade/rules/mod.rs b/src/rules/pyupgrade/rules/mod.rs index e3af3e53377afc..e63a7513337774 100644 --- a/src/rules/pyupgrade/rules/mod.rs +++ b/src/rules/pyupgrade/rules/mod.rs @@ -30,6 +30,7 @@ pub(crate) use use_pep585_annotation::use_pep585_annotation; pub(crate) use use_pep604_annotation::use_pep604_annotation; pub(crate) use useless_metaclass_type::useless_metaclass_type; pub(crate) use useless_object_inheritance::useless_object_inheritance; +pub(crate) use f_strings::f_strings; use crate::ast::helpers::{self}; use crate::ast::types::{Range, Scope, ScopeKind}; @@ -65,6 +66,7 @@ mod use_pep585_annotation; mod use_pep604_annotation; mod useless_metaclass_type; mod useless_object_inheritance; +mod f_strings; /// UP008 pub fn super_args( diff --git a/src/violations.rs b/src/violations.rs index 379f73abcd3261..1b56fcd80f6591 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -3837,6 +3837,23 @@ impl AlwaysAutofixableViolation for FormatLiterals { } } +define_violation!( + pub struct FString; +); +impl AlwaysAutofixableViolation for FString { + fn message(&self) -> String { + "Use f-strings instead of positional format fields".to_string() + } + + fn autofix_title(&self) -> String { + "Convert strings with positional format fields to strings".to_string() + } + + fn placeholder() -> Self { + Self + } +} + // pydocstyle define_violation!( From b10ad54e87b90f286bd9d9453b97d2da6c2ef3df Mon Sep 17 00:00:00 2001 From: colin99d Date: Sun, 15 Jan 2023 18:31:22 -0500 Subject: [PATCH 02/26] Made progress --- resources/test/fixtures/pyupgrade/UP032.py | 2 +- src/rules/pyupgrade/rules/f_strings.rs | 75 +++++++++++++++++++++- 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index 4d919336a0223c..8b137891791fe9 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -1 +1 @@ -'{foo} {bar}'.format(foo=foo, bar=bar) + diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index acea980b4f6369..d38dcf39f6caca 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -1,7 +1,78 @@ -use crate::rules::pyflakes::format::FormatSummary; +use crate::ast::types::Range; use crate::checkers::ast::Checker; -use rustpython_ast::Expr; +use crate::fix::Fix; +use crate::registry::Diagnostic; +use crate::rules::pyflakes::format::FormatSummary; +use crate::violations; +use rustpython_ast::{Expr, ExprKind}; +use std::collections::HashMap; + +#[derive(Debug)] +struct FormatFunction { + args: Vec, + kwargs: HashMap, +} + +impl FormatFunction { + fn new(expr: &Expr) -> Result { + println!("expr: {:?}", expr); + if let ExprKind::Call{ func, args, keywords } = &expr.node { + println!("{:?}", args); + println!("{:?}", keywords); + } + Ok(Self { + args: vec![], + kwargs: HashMap::new(), + }) + } + + /// Returns true if args and kwargs are empty + fn is_empty(&self) -> bool { + self.args.is_empty() && self.kwargs.is_empty() + } + + fn add_arg(&mut self, arg: String) { + self.args.push(arg); + } + + fn add_kwarg(&mut self, key: String, value: String) { + self.kwargs.insert(key, value); + } + + /// Returns true if the statement and function call match, and false if not + fn check_with_summary(&self, summary: &FormatSummary) -> bool { + summary.autos.len() == self.args.len() && summary.keywords.len() == self.kwargs.len() + } +} + +fn generate_f_string(summary: &FormatSummary, expr: &Expr) -> String { + let mut original_call = FormatFunction::new(expr); + println!("{:?}", original_call); + String::new() +} /// UP032 pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) { + if summary.has_nested_parts { + return; + } + // UP030 already removes the indexes, so we should not need to worry about the complexity + if !summary.indexes.is_empty() { + return; + } + println!("Checkpoint Charlie"); + let mut diagnostic = Diagnostic::new(violations::FString, Range::from_located(expr)); + println!("{:?}", diagnostic.kind.code()); + println!("{:?}", checker.patch(diagnostic.kind.code())); + // Currently, the only issue we know of is in LibCST: + // https://github.com/Instagram/LibCST/issues/846 + let contents = generate_f_string(summary, expr); + if checker.patch(diagnostic.kind.code()) { + diagnostic.amend(Fix::replacement( + contents, + expr.location, + expr.end_location.unwrap(), + )); + }; + checker.diagnostics.push(diagnostic); } From c7c5056ea0437617fc83535115b6a5907751a3d2 Mon Sep 17 00:00:00 2001 From: colin99d Date: Sun, 15 Jan 2023 19:28:02 -0500 Subject: [PATCH 03/26] Added unit tests --- resources/test/fixtures/pyupgrade/UP032.py | 2 +- src/rules/pyupgrade/rules/f_strings.rs | 138 ++++++++++++++++++--- 2 files changed, 120 insertions(+), 20 deletions(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index 8b137891791fe9..a9706db17a5310 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -1 +1 @@ - +'{}{}{}{x}{foo} {bar}'.format(x, y, z, foo=hello, bar=world) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index d38dcf39f6caca..177b5f3be9d5b3 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -4,7 +4,7 @@ use crate::fix::Fix; use crate::registry::Diagnostic; use crate::rules::pyflakes::format::FormatSummary; use crate::violations; -use rustpython_ast::{Expr, ExprKind}; +use rustpython_ast::{Expr, ExprKind, KeywordData}; use std::collections::HashMap; #[derive(Debug)] @@ -14,16 +14,29 @@ struct FormatFunction { } impl FormatFunction { - fn new(expr: &Expr) -> Result { - println!("expr: {:?}", expr); - if let ExprKind::Call{ func, args, keywords } = &expr.node { - println!("{:?}", args); - println!("{:?}", keywords); + fn from_expr(expr: &Expr) -> Self { + let mut final_args: Vec = Vec::new(); + let mut final_kwargs: HashMap = HashMap::new(); + if let ExprKind::Call { args, keywords, .. } = &expr.node { + for arg in args { + if let ExprKind::Name { id, .. } = &arg.node { + final_args.push(id.to_string()) + } + } + + for keyword in keywords { + let KeywordData { arg, value } = &keyword.node; + if let ExprKind::Name { id, .. } = &value.node { + if let Some(key) = arg { + final_kwargs.insert(key.to_string(), id.to_string()); + } + } + } + } + Self { + args: final_args, + kwargs: final_kwargs, } - Ok(Self { - args: vec![], - kwargs: HashMap::new(), - }) } /// Returns true if args and kwargs are empty @@ -41,14 +54,18 @@ impl FormatFunction { /// Returns true if the statement and function call match, and false if not fn check_with_summary(&self, summary: &FormatSummary) -> bool { - summary.autos.len() == self.args.len() && summary.keywords.len() == self.kwargs.len() + let mut self_keys = self.kwargs.clone().into_keys().collect::>(); + self_keys.sort(); + let mut summary_keys = summary.keywords.clone(); + summary_keys.sort(); + summary.autos.len() == self.args.len() && self_keys == summary_keys } } -fn generate_f_string(summary: &FormatSummary, expr: &Expr) -> String { - let mut original_call = FormatFunction::new(expr); +fn generate_f_string(summary: &FormatSummary, expr: &Expr) -> Option { + let mut original_call = FormatFunction::from_expr(expr); println!("{:?}", original_call); - String::new() + Some(String::new()) } /// UP032 @@ -60,13 +77,14 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E if !summary.indexes.is_empty() { return; } - println!("Checkpoint Charlie"); - let mut diagnostic = Diagnostic::new(violations::FString, Range::from_located(expr)); - println!("{:?}", diagnostic.kind.code()); - println!("{:?}", checker.patch(diagnostic.kind.code())); // Currently, the only issue we know of is in LibCST: // https://github.com/Instagram/LibCST/issues/846 - let contents = generate_f_string(summary, expr); + let contents = match generate_f_string(summary, expr) { + None => return, + Some(items) => items, + }; + println!("WE HERE"); + let mut diagnostic = Diagnostic::new(violations::FString, Range::from_located(expr)); if checker.patch(diagnostic.kind.code()) { diagnostic.amend(Fix::replacement( contents, @@ -76,3 +94,85 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E }; checker.diagnostics.push(diagnostic); } + + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_check_with_summary() { + let summary = FormatSummary { + autos: vec![0, 1], + keywords: vec!["c".to_string(), "d".to_string()], + has_nested_parts: false, + indexes: vec![], + }; + let form_func = FormatFunction { + args: vec!["a".to_string(), "b".to_string()], + kwargs: [("c".to_string(), "e".to_string()), ("d".to_string(), "f".to_string())] + .iter() + .cloned() + .collect(), + }; + let checks_out = form_func.check_with_summary(&summary); + assert!(checks_out); + } + + #[test] + fn test_check_with_summary_unuequal_args() { + let summary = FormatSummary { + autos: vec![0, 1], + keywords: vec!["c".to_string()], + has_nested_parts: false, + indexes: vec![], + }; + let form_func = FormatFunction { + args: vec!["a".to_string(), "b".to_string()], + kwargs: [("c".to_string(), "e".to_string()), ("d".to_string(), "f".to_string())] + .iter() + .cloned() + .collect(), + }; + let checks_out = form_func.check_with_summary(&summary); + assert!(!checks_out); + } + + #[test] + fn test_check_with_summary_different_kwargs() { + let summary = FormatSummary { + autos: vec![0, 1], + keywords: vec!["c".to_string(), "d".to_string()], + has_nested_parts: false, + indexes: vec![], + }; + let form_func = FormatFunction { + args: vec!["a".to_string(), "b".to_string()], + kwargs: [("c".to_string(), "e".to_string()), ("e".to_string(), "f".to_string())] + .iter() + .cloned() + .collect(), + }; + let checks_out = form_func.check_with_summary(&summary); + assert!(!checks_out); + } + + #[test] + fn test_check_with_summary_kwargs_same_diff_order() { + let summary = FormatSummary { + autos: vec![0, 1], + keywords: vec!["c".to_string(), "d".to_string()], + has_nested_parts: false, + indexes: vec![], + }; + let form_func = FormatFunction { + args: vec!["a".to_string(), "b".to_string()], + kwargs: [("d".to_string(), "e".to_string()), ("c".to_string(), "f".to_string())] + .iter() + .cloned() + .collect(), + }; + let checks_out = form_func.check_with_summary(&summary); + assert!(checks_out); + } +} From 6b3430b76943cc063fa681cc7e4e89f85949eb2f Mon Sep 17 00:00:00 2001 From: colin99d Date: Sun, 15 Jan 2023 21:50:19 -0500 Subject: [PATCH 04/26] Got something working --- resources/test/fixtures/pyupgrade/UP032.py | 2 +- src/rules/pyupgrade/rules/f_strings.rs | 67 +++++++++++++++++++--- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index a9706db17a5310..a3f4378967ca8d 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -1 +1 @@ -'{}{}{}{x}{foo} {bar}'.format(x, y, z, foo=hello, bar=world) +'{}{}{}{foo} {bar}'.format(x, y, z, foo=hello, bar=world) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index 177b5f3be9d5b3..a534db47878786 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -1,12 +1,19 @@ +use once_cell::sync::Lazy; +use regex::{Regex, Captures}; use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::fix::Fix; use crate::registry::Diagnostic; use crate::rules::pyflakes::format::FormatSummary; use crate::violations; -use rustpython_ast::{Expr, ExprKind, KeywordData}; +use rustpython_ast::{Expr, ExprKind, KeywordData, Constant}; use std::collections::HashMap; +// Checks for curly brackets. Inside these brackets this checks for an optional valid python name +// and any format specifiers afterwards +static NAME_SPECIFIER: Lazy = + Lazy::new(|| Regex::new(r"\{(?P[^\W0-9]\w*)?(?P.*?)}").unwrap()); + #[derive(Debug)] struct FormatFunction { args: Vec, @@ -44,12 +51,16 @@ impl FormatFunction { self.args.is_empty() && self.kwargs.is_empty() } - fn add_arg(&mut self, arg: String) { - self.args.push(arg); + fn consume_arg(&mut self) -> Option { + if self.args.len() > 0 { + Some(self.args.remove(0)) + } else { + None + } } - fn add_kwarg(&mut self, key: String, value: String) { - self.kwargs.insert(key, value); + fn get_kwarg(&self, key: &str) -> Option { + self.kwargs.get(key).cloned() } /// Returns true if the statement and function call match, and false if not @@ -62,10 +73,50 @@ impl FormatFunction { } } +fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option { + let mut new_string = String::new(); + if let ExprKind::Call { func, .. } = &expr.node { + if let ExprKind::Attribute { value, .. } = &func.node { + if let ExprKind::Constant { value, .. } = & value.node { + if let Constant::Str(string) = value { + new_string = string.to_string(); + } + } + } + } + // If we didn't get a valid string, return an empty string + if new_string.is_empty() { + return None; + } + let clean_string = NAME_SPECIFIER.replace_all(&new_string, |caps: &Captures| { + if let Some(name) = caps.name("name") { + // I believe we do sufficient checks to make sure this unwrap is safe + let kwarg = function.get_kwarg(name.as_str()).unwrap(); + let second_part = caps.name("fmt").unwrap().as_str(); + format!("{{{}{}}}", kwarg, second_part) + } else { + // I believe we do sufficient checks to make sure this unwrap is safe + let arg = function.consume_arg().unwrap(); + let second_part = caps.name("fmt").unwrap().as_str(); + format!("{{{}{}}}", arg, second_part) + } + }); + Some(clean_string.to_string()) +} + fn generate_f_string(summary: &FormatSummary, expr: &Expr) -> Option { let mut original_call = FormatFunction::from_expr(expr); - println!("{:?}", original_call); - Some(String::new()) + // We do not need to make changes if there are no arguments (let me know if you want me to + // change this to removing the .format() and doing nothing else, but that seems like it could + // cause issues) + if original_call.is_empty() { + return None; + } + // If the actual string and the format function do not match, we cannot operate + if !original_call.check_with_summary(summary) { + return None; + } + create_new_string(expr, &mut original_call) } /// UP032 @@ -83,7 +134,7 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E None => return, Some(items) => items, }; - println!("WE HERE"); + println!("{}", contents); let mut diagnostic = Diagnostic::new(violations::FString, Range::from_located(expr)); if checker.patch(diagnostic.kind.code()) { diagnostic.amend(Fix::replacement( From 0da754313472c5b1e210ec48ebacc7f259094e7e Mon Sep 17 00:00:00 2001 From: colin99d Date: Sun, 15 Jan 2023 21:53:22 -0500 Subject: [PATCH 05/26] Got all positive tests in --- resources/test/fixtures/pyupgrade/UP032.py | 28 +++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index a3f4378967ca8d..4e2e0514e81ffb 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -1 +1,27 @@ -'{}{}{}{foo} {bar}'.format(x, y, z, foo=hello, bar=world) +# These SHOULD change +"{} {}".format(a, b) + +"{1} {0}".format(a, b) + +"{x.y}".format(x=z) + +"{.x} {.y}".format(a, b) + +"{} {}".format(a.b, c.d) + +"{}".format(a()) + +"{}".format(a.b()) + +"{}".format(a.b().c()) + +"hello {}!".format(name) + +"{}{{}}{}".format(escaped, y) + +"{}{b}{}".format(a, c, b=b) + +"{}".format(0x0) + +# Waiting on Charlie solution to solve this +# r'"\N{snowman} {}".format(a)' From 47454c78c7d91f5ca2de573975f91c7718696e23 Mon Sep 17 00:00:00 2001 From: colin99d Date: Sun, 15 Jan 2023 21:55:09 -0500 Subject: [PATCH 06/26] Got all positive tests in --- resources/test/fixtures/pyupgrade/UP032.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index 4e2e0514e81ffb..a3cfbb7527c6cb 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -23,5 +23,7 @@ "{}".format(0x0) +u"foo{}".format(1) + # Waiting on Charlie solution to solve this # r'"\N{snowman} {}".format(a)' From d516a637c9226eed69be956d2e25af04d99f46c6 Mon Sep 17 00:00:00 2001 From: colin99d Date: Sun, 15 Jan 2023 22:10:06 -0500 Subject: [PATCH 07/26] Added error handling --- src/rules/pyupgrade/rules/f_strings.rs | 37 +++++++++++++++++++++----- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index a534db47878786..ef60527cdab3a3 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -88,16 +88,40 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option { + had_error = true; + return String::new(); + } + Some(item) => item + }; + let second_part = match caps.name("fmt") { + None => { + had_error = true; + return String::new(); + } + Some(item) => item.as_str() + }; format!("{{{}{}}}", kwarg, second_part) } else { - // I believe we do sufficient checks to make sure this unwrap is safe - let arg = function.consume_arg().unwrap(); - let second_part = caps.name("fmt").unwrap().as_str(); + let arg = match function.consume_arg() { + None => { + had_error = true; + return String::new(); + } + Some(item) => item + }; + let second_part = match caps.name("fmt") { + None => { + had_error = true; + return String::new(); + } + Some(item) => item.as_str() + }; format!("{{{}{}}}", arg, second_part) } }); @@ -134,7 +158,6 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E None => return, Some(items) => items, }; - println!("{}", contents); let mut diagnostic = Diagnostic::new(violations::FString, Range::from_located(expr)); if checker.patch(diagnostic.kind.code()) { diagnostic.amend(Fix::replacement( From f56f6828868c8f47b1a4e22a1ffb1094305c10c3 Mon Sep 17 00:00:00 2001 From: colin99d Date: Sun, 15 Jan 2023 22:10:36 -0500 Subject: [PATCH 08/26] Improved error handling --- src/rules/pyupgrade/rules/f_strings.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index ef60527cdab3a3..e503c58583515c 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -125,6 +125,9 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option Date: Sun, 15 Jan 2023 22:17:54 -0500 Subject: [PATCH 09/26] Actually make a string --- src/rules/pyupgrade/rules/f_strings.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index e503c58583515c..d232f58eaa906d 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -128,7 +128,7 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option Option { From 0e955885e0bcc71a7f68ccb2013fe41a2fa7d855 Mon Sep 17 00:00:00 2001 From: colin99d Date: Sun, 15 Jan 2023 22:20:27 -0500 Subject: [PATCH 10/26] Last little change for tonight --- resources/test/fixtures/pyupgrade/UP032.py | 1 + 1 file changed, 1 insertion(+) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index a3cfbb7527c6cb..c3a44349f798d5 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -25,5 +25,6 @@ u"foo{}".format(1) +# Don't forget assigned, call, and multiline # Waiting on Charlie solution to solve this # r'"\N{snowman} {}".format(a)' From c518410952017e80abbb07e8c9b5cc18bcd321c2 Mon Sep 17 00:00:00 2001 From: colin99d Date: Mon, 16 Jan 2023 13:42:23 -0500 Subject: [PATCH 11/26] Made a lot of progress --- resources/test/fixtures/pyupgrade/UP032.py | 5 +++-- src/rules/pyupgrade/mod.rs | 1 + src/rules/pyupgrade/rules/f_strings.rs | 22 +++++++++++----------- src/rules/pyupgrade/rules/mod.rs | 2 +- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index c3a44349f798d5..92cd477918d072 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -17,14 +17,15 @@ "hello {}!".format(name) -"{}{{}}{}".format(escaped, y) - "{}{b}{}".format(a, c, b=b) "{}".format(0x0) u"foo{}".format(1) +# Waiting for a response to solve this one +# "{}{{}}{}".format(escaped, y) + # Don't forget assigned, call, and multiline # Waiting on Charlie solution to solve this # r'"\N{snowman} {}".format(a)' diff --git a/src/rules/pyupgrade/mod.rs b/src/rules/pyupgrade/mod.rs index 16c3eb721bfa6e..493f112640ee21 100644 --- a/src/rules/pyupgrade/mod.rs +++ b/src/rules/pyupgrade/mod.rs @@ -53,6 +53,7 @@ mod tests { #[test_case(RuleCode::UP029, Path::new("UP029.py"); "UP029")] #[test_case(RuleCode::UP030, Path::new("UP030_0.py"); "UP030_0")] #[test_case(RuleCode::UP030, Path::new("UP030_1.py"); "UP030_1")] + #[test_case(RuleCode::UP032, Path::new("UP032.py"); "UP032")] fn rules(rule_code: RuleCode, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index d232f58eaa906d..ce49c2dda0f2bb 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -21,22 +21,22 @@ struct FormatFunction { } impl FormatFunction { - fn from_expr(expr: &Expr) -> Self { + fn from_expr(checker: &mut Checker, expr: &Expr) -> Self { let mut final_args: Vec = Vec::new(); let mut final_kwargs: HashMap = HashMap::new(); if let ExprKind::Call { args, keywords, .. } = &expr.node { for arg in args { - if let ExprKind::Name { id, .. } = &arg.node { - final_args.push(id.to_string()) - } + let arg_range = Range::from_located(&arg); + let arg_string = checker.locator.slice_source_code_range(&arg_range); + final_args.push(arg_string.to_string()); } for keyword in keywords { let KeywordData { arg, value } = &keyword.node; - if let ExprKind::Name { id, .. } = &value.node { - if let Some(key) = arg { - final_kwargs.insert(key.to_string(), id.to_string()); - } + if let Some(key) = arg { + let kwarg_range = Range::from_located(&value); + let kwarg_string = checker.locator.slice_source_code_range(&kwarg_range); + final_kwargs.insert(key.to_string(), kwarg_string.to_string()); } } } @@ -131,8 +131,8 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option Option { - let mut original_call = FormatFunction::from_expr(expr); +fn generate_f_string(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) -> Option { + let mut original_call = FormatFunction::from_expr(checker, expr); // We do not need to make changes if there are no arguments (let me know if you want me to // change this to removing the .format() and doing nothing else, but that seems like it could // cause issues) @@ -157,7 +157,7 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E } // Currently, the only issue we know of is in LibCST: // https://github.com/Instagram/LibCST/issues/846 - let contents = match generate_f_string(summary, expr) { + let contents = match generate_f_string(checker, summary, expr) { None => return, Some(items) => items, }; diff --git a/src/rules/pyupgrade/rules/mod.rs b/src/rules/pyupgrade/rules/mod.rs index e63a7513337774..5ebc53920fea9b 100644 --- a/src/rules/pyupgrade/rules/mod.rs +++ b/src/rules/pyupgrade/rules/mod.rs @@ -32,7 +32,7 @@ pub(crate) use useless_metaclass_type::useless_metaclass_type; pub(crate) use useless_object_inheritance::useless_object_inheritance; pub(crate) use f_strings::f_strings; -use crate::ast::helpers::{self}; +use crate::ast::helpers; use crate::ast::types::{Range, Scope, ScopeKind}; use crate::fix::Fix; use crate::registry::Diagnostic; From 62e8a41e05f74836972330fa45ed350c60503135 Mon Sep 17 00:00:00 2001 From: colin99d Date: Mon, 16 Jan 2023 14:10:39 -0500 Subject: [PATCH 12/26] Added handling for a lot of negative cases --- resources/test/fixtures/pyupgrade/UP032.py | 39 ++++++++++++++++++++++ src/rules/pyupgrade/rules/f_strings.rs | 36 +++++++++++++++++++- 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index 92cd477918d072..82f29435c153b9 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -29,3 +29,42 @@ # Don't forget assigned, call, and multiline # Waiting on Charlie solution to solve this # r'"\N{snowman} {}".format(a)' + + +# These should NOT change + +"{".format(a) + +"}".format(a) + +"{}" . format(x) + +"{}".format( + a +) + +"{} {}".format(*a) + +"{0} {0}".format(arg) + +"{x} {x}".format(arg) + +"{x.y} {x.z}".format(arg) + +b"{} {}".format(a, b) + +"{:{}}".format(x, y) + +"{a[b]}".format(a=a) + +"{a.a[b]}".format(a=a) + +"{}{}".format(a) + +''"{}".format(a['\\'])'' + +'{}'.format(a['b']) + +async def c(): return '{}'.format(await 3) + +async def c(): return '{}'.format(1 + await 3) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index ce49c2dda0f2bb..2f64cd5fe60206 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -18,16 +18,31 @@ static NAME_SPECIFIER: Lazy = struct FormatFunction { args: Vec, kwargs: HashMap, + // Whether or not something invalid was found, if it was return IMMIDEATELY + invalid: bool, +} + +/// Whether the given string contains characters that are FORBIDDEN in args and kwargs +fn contains_invalids(string: &str) -> bool { + let invalids = vec!['*', '\'','"']; + for invalid in invalids { + if string.contains(invalid) { + return true; + } + } + false } impl FormatFunction { fn from_expr(checker: &mut Checker, expr: &Expr) -> Self { let mut final_args: Vec = Vec::new(); let mut final_kwargs: HashMap = HashMap::new(); + let mut invalid = false; if let ExprKind::Call { args, keywords, .. } = &expr.node { for arg in args { let arg_range = Range::from_located(&arg); let arg_string = checker.locator.slice_source_code_range(&arg_range); + invalid = contains_invalids(&arg_string); final_args.push(arg_string.to_string()); } @@ -36,6 +51,7 @@ impl FormatFunction { if let Some(key) = arg { let kwarg_range = Range::from_located(&value); let kwarg_string = checker.locator.slice_source_code_range(&kwarg_range); + invalid = contains_invalids(&kwarg_string); final_kwargs.insert(key.to_string(), kwarg_string.to_string()); } } @@ -43,6 +59,7 @@ impl FormatFunction { Self { args: final_args, kwargs: final_kwargs, + invalid } } @@ -77,7 +94,13 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option Option Option { let mut original_call = FormatFunction::from_expr(checker, expr); + + // If there were any invalid characters we should return immediately + if original_call.invalid { + return None; + } // We do not need to make changes if there are no arguments (let me know if you want me to // change this to removing the .format() and doing nothing else, but that seems like it could // cause issues) @@ -148,6 +176,12 @@ fn generate_f_string(checker: &mut Checker, summary: &FormatSummary, expr: &Expr /// UP032 pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) { + let expr_range = Range::from_located(&expr); + let expr_string = checker.locator.slice_source_code_range(&expr_range); + // Pyupgrade says we should not try and refactor multi-line statements + if expr_string.contains('\n') { + return; + } if summary.has_nested_parts { return; } From 3f16a2a4008dfbb080e1ef797b6a38351734495e Mon Sep 17 00:00:00 2001 From: colin99d Date: Mon, 16 Jan 2023 14:20:56 -0500 Subject: [PATCH 13/26] Typos and fmt --- src/rules/pyupgrade/rules/f_strings.rs | 121 +++++++++++++++---------- src/rules/pyupgrade/rules/mod.rs | 4 +- 2 files changed, 76 insertions(+), 49 deletions(-) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index 2f64cd5fe60206..1355a04d6dd176 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -1,16 +1,18 @@ +use std::collections::HashMap; + use once_cell::sync::Lazy; -use regex::{Regex, Captures}; +use regex::{Captures, Regex}; +use rustpython_ast::{Constant, Expr, ExprKind, KeywordData}; + use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::fix::Fix; use crate::registry::Diagnostic; use crate::rules::pyflakes::format::FormatSummary; use crate::violations; -use rustpython_ast::{Expr, ExprKind, KeywordData, Constant}; -use std::collections::HashMap; -// Checks for curly brackets. Inside these brackets this checks for an optional valid python name -// and any format specifiers afterwards +// Checks for curly brackets. Inside these brackets this checks for an optional +// valid python name and any format specifiers afterwards static NAME_SPECIFIER: Lazy = Lazy::new(|| Regex::new(r"\{(?P[^\W0-9]\w*)?(?P.*?)}").unwrap()); @@ -18,13 +20,14 @@ static NAME_SPECIFIER: Lazy = struct FormatFunction { args: Vec, kwargs: HashMap, - // Whether or not something invalid was found, if it was return IMMIDEATELY + // Whether or not something invalid was found, if it was return immediately invalid: bool, } -/// Whether the given string contains characters that are FORBIDDEN in args and kwargs +/// Whether the given string contains characters that are FORBIDDEN in args and +/// kwargs fn contains_invalids(string: &str) -> bool { - let invalids = vec!['*', '\'','"']; + let invalids = vec!['*', '\'', '"']; for invalid in invalids { if string.contains(invalid) { return true; @@ -59,7 +62,7 @@ impl FormatFunction { Self { args: final_args, kwargs: final_kwargs, - invalid + invalid, } } @@ -93,8 +96,8 @@ impl FormatFunction { fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option { let mut new_string = String::new(); if let ExprKind::Call { func, .. } = &expr.node { - if let ExprKind::Attribute { value, .. } = &func.node { - if let ExprKind::Constant { value, kind } = & value.node { + if let ExprKind::Attribute { value, .. } = &func.node { + if let ExprKind::Constant { value, kind } = &value.node { // Do NOT refactor byte strings if let Some(kind_str) = kind { if kind_str == "b" { @@ -111,7 +114,8 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option Option item + Some(item) => item, }; let second_part = match caps.name("fmt") { None => { had_error = true; return String::new(); } - Some(item) => item.as_str() + Some(item) => item.as_str(), }; format!("{{{}{}}}", kwarg, second_part) } else { @@ -136,34 +140,38 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option item + Some(item) => item, }; let second_part = match caps.name("fmt") { None => { had_error = true; return String::new(); } - Some(item) => item.as_str() + Some(item) => item.as_str(), }; format!("{{{}{}}}", arg, second_part) } }); if had_error { - return None + return None; } Some(format!("f\"{}\"", clean_string)) } -fn generate_f_string(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) -> Option { +fn generate_f_string( + checker: &mut Checker, + summary: &FormatSummary, + expr: &Expr, +) -> Option { let mut original_call = FormatFunction::from_expr(checker, expr); // If there were any invalid characters we should return immediately if original_call.invalid { return None; } - // We do not need to make changes if there are no arguments (let me know if you want me to - // change this to removing the .format() and doing nothing else, but that seems like it could - // cause issues) + // We do not need to make changes if there are no arguments (let me know if you + // want me to change this to removing the .format() and doing nothing else, + // but that seems like it could cause issues) if original_call.is_empty() { return None; } @@ -176,16 +184,20 @@ fn generate_f_string(checker: &mut Checker, summary: &FormatSummary, expr: &Expr /// UP032 pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) { + println!("Checkpoint 0"); let expr_range = Range::from_located(&expr); let expr_string = checker.locator.slice_source_code_range(&expr_range); // Pyupgrade says we should not try and refactor multi-line statements + println!("Checkpoint 1"); if expr_string.contains('\n') { return; } + println!("Checkpoint 2"); if summary.has_nested_parts { return; } - // UP030 already removes the indexes, so we should not need to worry about the complexity + // UP030 already removes the indexes, so we should not need to worry about the + // complexity if !summary.indexes.is_empty() { return; } @@ -195,6 +207,10 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E None => return, Some(items) => items, }; + // Don't refactor if it will make the string longer + if contents.len() > expr_string.len() { + return; + } let mut diagnostic = Diagnostic::new(violations::FString, Range::from_located(expr)); if checker.patch(diagnostic.kind.code()) { diagnostic.amend(Fix::replacement( @@ -206,7 +222,6 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E checker.diagnostics.push(diagnostic); } - #[cfg(test)] mod tests { use super::*; @@ -221,13 +236,16 @@ mod tests { }; let form_func = FormatFunction { args: vec!["a".to_string(), "b".to_string()], - kwargs: [("c".to_string(), "e".to_string()), ("d".to_string(), "f".to_string())] - .iter() - .cloned() - .collect(), + kwargs: [ + ("c".to_string(), "e".to_string()), + ("d".to_string(), "f".to_string()), + ] + .iter() + .cloned() + .collect(), }; - let checks_out = form_func.check_with_summary(&summary); - assert!(checks_out); + let checks_out = form_func.check_with_summary(&summary); + assert!(checks_out); } #[test] @@ -240,13 +258,16 @@ mod tests { }; let form_func = FormatFunction { args: vec!["a".to_string(), "b".to_string()], - kwargs: [("c".to_string(), "e".to_string()), ("d".to_string(), "f".to_string())] - .iter() - .cloned() - .collect(), + kwargs: [ + ("c".to_string(), "e".to_string()), + ("d".to_string(), "f".to_string()), + ] + .iter() + .cloned() + .collect(), }; - let checks_out = form_func.check_with_summary(&summary); - assert!(!checks_out); + let checks_out = form_func.check_with_summary(&summary); + assert!(!checks_out); } #[test] @@ -259,13 +280,16 @@ mod tests { }; let form_func = FormatFunction { args: vec!["a".to_string(), "b".to_string()], - kwargs: [("c".to_string(), "e".to_string()), ("e".to_string(), "f".to_string())] - .iter() - .cloned() - .collect(), + kwargs: [ + ("c".to_string(), "e".to_string()), + ("e".to_string(), "f".to_string()), + ] + .iter() + .cloned() + .collect(), }; - let checks_out = form_func.check_with_summary(&summary); - assert!(!checks_out); + let checks_out = form_func.check_with_summary(&summary); + assert!(!checks_out); } #[test] @@ -278,12 +302,15 @@ mod tests { }; let form_func = FormatFunction { args: vec!["a".to_string(), "b".to_string()], - kwargs: [("d".to_string(), "e".to_string()), ("c".to_string(), "f".to_string())] - .iter() - .cloned() - .collect(), + kwargs: [ + ("d".to_string(), "e".to_string()), + ("c".to_string(), "f".to_string()), + ] + .iter() + .cloned() + .collect(), }; - let checks_out = form_func.check_with_summary(&summary); - assert!(checks_out); + let checks_out = form_func.check_with_summary(&summary); + assert!(checks_out); } } diff --git a/src/rules/pyupgrade/rules/mod.rs b/src/rules/pyupgrade/rules/mod.rs index 5ebc53920fea9b..7c9576d720b43e 100644 --- a/src/rules/pyupgrade/rules/mod.rs +++ b/src/rules/pyupgrade/rules/mod.rs @@ -2,6 +2,7 @@ pub(crate) use convert_named_tuple_functional_to_class::convert_named_tuple_func pub(crate) use convert_typed_dict_functional_to_class::convert_typed_dict_functional_to_class; pub(crate) use datetime_utc_alias::datetime_utc_alias; pub(crate) use deprecated_unittest_alias::deprecated_unittest_alias; +pub(crate) use f_strings::f_strings; pub(crate) use format_literals::format_literals; pub(crate) use native_literals::native_literals; use once_cell::sync::Lazy; @@ -30,7 +31,6 @@ pub(crate) use use_pep585_annotation::use_pep585_annotation; pub(crate) use use_pep604_annotation::use_pep604_annotation; pub(crate) use useless_metaclass_type::useless_metaclass_type; pub(crate) use useless_object_inheritance::useless_object_inheritance; -pub(crate) use f_strings::f_strings; use crate::ast::helpers; use crate::ast::types::{Range, Scope, ScopeKind}; @@ -42,6 +42,7 @@ mod convert_named_tuple_functional_to_class; mod convert_typed_dict_functional_to_class; mod datetime_utc_alias; mod deprecated_unittest_alias; +mod f_strings; mod format_literals; mod native_literals; mod open_alias; @@ -66,7 +67,6 @@ mod use_pep585_annotation; mod use_pep604_annotation; mod useless_metaclass_type; mod useless_object_inheritance; -mod f_strings; /// UP008 pub fn super_args( From e48cf151e2e4c3faa26d4cc290a4ee6266a3f6bb Mon Sep 17 00:00:00 2001 From: colin99d Date: Mon, 16 Jan 2023 15:00:57 -0500 Subject: [PATCH 14/26] Fixed some small things --- resources/test/fixtures/pyupgrade/UP032.py | 2 +- src/rules/pyupgrade/rules/f_strings.rs | 51 ++++++++++++++-------- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index 82f29435c153b9..b2598d6b33bf18 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -61,7 +61,7 @@ "{}{}".format(a) -''"{}".format(a['\\'])'' +''"{}".format(a['\\']) '{}'.format(a['b']) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index 1355a04d6dd176..f1b3ef5f5bebc2 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -16,6 +16,8 @@ use crate::violations; static NAME_SPECIFIER: Lazy = Lazy::new(|| Regex::new(r"\{(?P[^\W0-9]\w*)?(?P.*?)}").unwrap()); +static HAS_BRACKETS: Lazy = Lazy::new(|| Regex::new(r"[.*]").unwrap()); + #[derive(Debug)] struct FormatFunction { args: Vec, @@ -93,6 +95,20 @@ impl FormatFunction { } } + +fn extract_caps(caps: &Captures, target: &str) -> Result { + let new_string = match caps.name(target) { + None => { + return Err(()) + } + Some(item) => item.as_str(), + }; + if HAS_BRACKETS.is_match(new_string) { + return Err(()) + } + Ok(new_string.to_string()) +} + fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option { let mut new_string = String::new(); if let ExprKind::Call { func, .. } = &expr.node { @@ -126,14 +142,12 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option item, }; - let second_part = match caps.name("fmt") { - None => { - had_error = true; - return String::new(); - } - Some(item) => item.as_str(), - }; - format!("{{{}{}}}", kwarg, second_part) + if let Ok(second_part) = extract_caps(&caps, "fmt") { + format!("{{{}{}}}", kwarg, second_part) + } else { + had_error = true; + "badstring".to_string() + } } else { let arg = match function.consume_arg() { None => { @@ -142,14 +156,12 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option item, }; - let second_part = match caps.name("fmt") { - None => { - had_error = true; - return String::new(); - } - Some(item) => item.as_str(), - }; - format!("{{{}{}}}", arg, second_part) + if let Ok(second_part) = extract_caps(&caps, "fmt") { + format!("{{{}{}}}", arg, second_part) + } else { + had_error = true; + "badstring".to_string() + } } }); if had_error { @@ -184,15 +196,12 @@ fn generate_f_string( /// UP032 pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) { - println!("Checkpoint 0"); let expr_range = Range::from_located(&expr); let expr_string = checker.locator.slice_source_code_range(&expr_range); // Pyupgrade says we should not try and refactor multi-line statements - println!("Checkpoint 1"); if expr_string.contains('\n') { return; } - println!("Checkpoint 2"); if summary.has_nested_parts { return; } @@ -243,6 +252,7 @@ mod tests { .iter() .cloned() .collect(), + invalid: false }; let checks_out = form_func.check_with_summary(&summary); assert!(checks_out); @@ -265,6 +275,7 @@ mod tests { .iter() .cloned() .collect(), + invalid: false }; let checks_out = form_func.check_with_summary(&summary); assert!(!checks_out); @@ -287,6 +298,7 @@ mod tests { .iter() .cloned() .collect(), + invalid: false }; let checks_out = form_func.check_with_summary(&summary); assert!(!checks_out); @@ -309,6 +321,7 @@ mod tests { .iter() .cloned() .collect(), + invalid: false }; let checks_out = form_func.check_with_summary(&summary); assert!(checks_out); From d30ebc9bb29e9f01f9df9c3f7381bce4eb00bc18 Mon Sep 17 00:00:00 2001 From: colin99d Date: Mon, 16 Jan 2023 15:30:26 -0500 Subject: [PATCH 15/26] Fixed some small things --- README.md | 1 + resources/test/fixtures/pyupgrade/UP032.py | 4 +--- ruff.schema.json | 1 + src/rules/pyupgrade/rules/f_strings.rs | 19 ++++++++----------- 4 files changed, 11 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 3f131e3e35a12e..976d3ac0ee6123 100644 --- a/README.md +++ b/README.md @@ -712,6 +712,7 @@ For more, see [pyupgrade](https://pypi.org/project/pyupgrade/3.2.0/) on PyPI. | UP028 | RewriteYieldFrom | Replace `yield` over `for` loop with `yield from` | 🛠 | | UP029 | UnnecessaryBuiltinImport | Unnecessary builtin import: `...` | 🛠 | | UP030 | FormatLiterals | Use implicit references for positional format fields | 🛠 | +| UP032 | FString | Use f-strings instead of positional format fields | 🛠 | ### pep8-naming (N) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index b2598d6b33bf18..5d268e1d7297c3 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -26,7 +26,7 @@ # Waiting for a response to solve this one # "{}{{}}{}".format(escaped, y) -# Don't forget assigned, call, and multiline +# Don't forget assigned, call # Waiting on Charlie solution to solve this # r'"\N{snowman} {}".format(a)' @@ -37,8 +37,6 @@ "}".format(a) -"{}" . format(x) - "{}".format( a ) diff --git a/ruff.schema.json b/ruff.schema.json index fbfbe1fb9cc437..440de34e968b16 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1686,6 +1686,7 @@ "UP029", "UP03", "UP030", + "UP032", "W", "W2", "W29", diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index f1b3ef5f5bebc2..e9a3f951801e3c 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -16,7 +16,7 @@ use crate::violations; static NAME_SPECIFIER: Lazy = Lazy::new(|| Regex::new(r"\{(?P[^\W0-9]\w*)?(?P.*?)}").unwrap()); -static HAS_BRACKETS: Lazy = Lazy::new(|| Regex::new(r"[.*]").unwrap()); +static HAS_BRACKETS: Lazy = Lazy::new(|| Regex::new(r"\[.*\]").unwrap()); #[derive(Debug)] struct FormatFunction { @@ -29,7 +29,7 @@ struct FormatFunction { /// Whether the given string contains characters that are FORBIDDEN in args and /// kwargs fn contains_invalids(string: &str) -> bool { - let invalids = vec!['*', '\'', '"']; + let invalids = vec!["*", "'", "\"", "await"]; for invalid in invalids { if string.contains(invalid) { return true; @@ -95,16 +95,13 @@ impl FormatFunction { } } - fn extract_caps(caps: &Captures, target: &str) -> Result { let new_string = match caps.name(target) { - None => { - return Err(()) - } + None => return Err(()), Some(item) => item.as_str(), }; if HAS_BRACKETS.is_match(new_string) { - return Err(()) + return Err(()); } Ok(new_string.to_string()) } @@ -252,7 +249,7 @@ mod tests { .iter() .cloned() .collect(), - invalid: false + invalid: false, }; let checks_out = form_func.check_with_summary(&summary); assert!(checks_out); @@ -275,7 +272,7 @@ mod tests { .iter() .cloned() .collect(), - invalid: false + invalid: false, }; let checks_out = form_func.check_with_summary(&summary); assert!(!checks_out); @@ -298,7 +295,7 @@ mod tests { .iter() .cloned() .collect(), - invalid: false + invalid: false, }; let checks_out = form_func.check_with_summary(&summary); assert!(!checks_out); @@ -321,7 +318,7 @@ mod tests { .iter() .cloned() .collect(), - invalid: false + invalid: false, }; let checks_out = form_func.check_with_summary(&summary); assert!(checks_out); From b4624ef7426f4a40ca4fd84b627bcf99c2f2da8d Mon Sep 17 00:00:00 2001 From: colin99d Date: Mon, 16 Jan 2023 15:35:46 -0500 Subject: [PATCH 16/26] two more rules --- resources/test/fixtures/pyupgrade/UP032.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index 5d268e1d7297c3..42ece0e55a3b96 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -23,14 +23,16 @@ u"foo{}".format(1) +x = "{a}".format(a=1) + +print("foo {} ".format(x)) + # Waiting for a response to solve this one # "{}{{}}{}".format(escaped, y) -# Don't forget assigned, call # Waiting on Charlie solution to solve this # r'"\N{snowman} {}".format(a)' - # These should NOT change "{".format(a) From aadf36489335215498279e56e7839abe8fc618bb Mon Sep 17 00:00:00 2001 From: colin99d Date: Mon, 16 Jan 2023 15:39:45 -0500 Subject: [PATCH 17/26] two more rules --- src/rules/pyupgrade/rules/f_strings.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index e9a3f951801e3c..100bd608b754dd 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -45,7 +45,7 @@ impl FormatFunction { let mut invalid = false; if let ExprKind::Call { args, keywords, .. } = &expr.node { for arg in args { - let arg_range = Range::from_located(&arg); + let arg_range = Range::from_located(arg); let arg_string = checker.locator.slice_source_code_range(&arg_range); invalid = contains_invalids(&arg_string); final_args.push(arg_string.to_string()); @@ -54,7 +54,7 @@ impl FormatFunction { for keyword in keywords { let KeywordData { arg, value } = &keyword.node; if let Some(key) = arg { - let kwarg_range = Range::from_located(&value); + let kwarg_range = Range::from_located(value); let kwarg_string = checker.locator.slice_source_code_range(&kwarg_range); invalid = contains_invalids(&kwarg_string); final_kwargs.insert(key.to_string(), kwarg_string.to_string()); @@ -74,7 +74,7 @@ impl FormatFunction { } fn consume_arg(&mut self) -> Option { - if self.args.len() > 0 { + if !self.args.is_empty() { Some(self.args.remove(0)) } else { None @@ -139,7 +139,7 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option item, }; - if let Ok(second_part) = extract_caps(&caps, "fmt") { + if let Ok(second_part) = extract_caps(caps, "fmt") { format!("{{{}{}}}", kwarg, second_part) } else { had_error = true; @@ -153,7 +153,7 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option item, }; - if let Ok(second_part) = extract_caps(&caps, "fmt") { + if let Ok(second_part) = extract_caps(caps, "fmt") { format!("{{{}{}}}", arg, second_part) } else { had_error = true; @@ -193,7 +193,7 @@ fn generate_f_string( /// UP032 pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) { - let expr_range = Range::from_located(&expr); + let expr_range = Range::from_located(expr); let expr_string = checker.locator.slice_source_code_range(&expr_range); // Pyupgrade says we should not try and refactor multi-line statements if expr_string.contains('\n') { From ed871cca67ac59fbebbaada0ebf21d02aa608fd5 Mon Sep 17 00:00:00 2001 From: colin99d Date: Mon, 16 Jan 2023 15:47:11 -0500 Subject: [PATCH 18/26] Fixed clippy rule --- src/rules/pyupgrade/rules/f_strings.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index 100bd608b754dd..36998cc8665c09 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -74,10 +74,10 @@ impl FormatFunction { } fn consume_arg(&mut self) -> Option { - if !self.args.is_empty() { - Some(self.args.remove(0)) - } else { + if self.args.is_empty() { None + } else { + Some(self.args.remove(0)) } } From 267f506af7c6bb630084867c2a8fb947d3bb7bdb Mon Sep 17 00:00:00 2001 From: colin99d Date: Mon, 16 Jan 2023 15:54:23 -0500 Subject: [PATCH 19/26] Fixed more clippy issues: --- src/rules/pyupgrade/rules/f_strings.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index 36998cc8665c09..694a529b450787 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -140,7 +140,7 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option item, }; if let Ok(second_part) = extract_caps(caps, "fmt") { - format!("{{{}{}}}", kwarg, second_part) + format!("{{{kwarg}{second_part}}}") } else { had_error = true; "badstring".to_string() @@ -154,7 +154,7 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option item, }; if let Ok(second_part) = extract_caps(caps, "fmt") { - format!("{{{}{}}}", arg, second_part) + format!("{{{arg}{second_part}}}") } else { had_error = true; "badstring".to_string() @@ -164,7 +164,7 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option Date: Mon, 16 Jan 2023 20:30:06 -0500 Subject: [PATCH 20/26] Fix fixtures --- resources/test/fixtures/pyupgrade/UP032.py | 26 +- src/checkers/ast.rs | 1 + src/rules/pyupgrade/rules/f_strings.rs | 23 +- ...les__pyupgrade__tests__UP032_UP032.py.snap | 226 ++++++++++++++++++ 4 files changed, 248 insertions(+), 28 deletions(-) create mode 100644 src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index 42ece0e55a3b96..794464eece0ea9 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -1,31 +1,31 @@ # These SHOULD change -"{} {}".format(a, b) +f"{a} {b}" "{1} {0}".format(a, b) -"{x.y}".format(x=z) +f"{z.y}" -"{.x} {.y}".format(a, b) +f"{a.x} {b.y}" -"{} {}".format(a.b, c.d) +f"{a.b} {c.d}" -"{}".format(a()) +f"{a()}" -"{}".format(a.b()) +f"{a.b()}" -"{}".format(a.b().c()) +f"{a.b().c()}" -"hello {}!".format(name) +f"hello {name}!" -"{}{b}{}".format(a, c, b=b) +f"{a}{b}{c}" -"{}".format(0x0) +f"{0x0}" -u"foo{}".format(1) +f"foo{1}" -x = "{a}".format(a=1) +x = f"{1}" -print("foo {} ".format(x)) +print(f"foo {x} ") # Waiting for a response to solve this one # "{}{{}}{}".format(escaped, y) diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 603c30f91aa151..18a192227535a0 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -1867,6 +1867,7 @@ where || self.settings.enabled.contains(&RuleCode::F525) // pyupgrade || self.settings.enabled.contains(&RuleCode::UP030) + || self.settings.enabled.contains(&RuleCode::UP032) { if let ExprKind::Attribute { value, attr, .. } = &func.node { if let ExprKind::Constant { diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index 694a529b450787..e5b1622be6f5cb 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -132,12 +132,9 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option { - had_error = true; - return String::new(); - } - Some(item) => item, + let Some(kwarg) = function.get_kwarg(name.as_str()) else { + had_error = true; + return String::new(); }; if let Ok(second_part) = extract_caps(caps, "fmt") { format!("{{{kwarg}{second_part}}}") @@ -146,12 +143,9 @@ fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option { - had_error = true; - return String::new(); - } - Some(item) => item, + let Some(arg) = function.consume_arg() else { + had_error = true; + return String::new(); }; if let Ok(second_part) = extract_caps(caps, "fmt") { format!("{{{arg}{second_part}}}") @@ -209,9 +203,8 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E } // Currently, the only issue we know of is in LibCST: // https://github.com/Instagram/LibCST/issues/846 - let contents = match generate_f_string(checker, summary, expr) { - None => return, - Some(items) => items, + let Some(contents) = generate_f_string(checker, summary, expr) else { + return; }; // Don't refactor if it will make the string longer if contents.len() > expr_string.len() { diff --git a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap new file mode 100644 index 00000000000000..fc88e966a62134 --- /dev/null +++ b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap @@ -0,0 +1,226 @@ +--- +source: src/rules/pyupgrade/mod.rs +expression: diagnostics +--- +- kind: + FString: ~ + location: + row: 2 + column: 0 + end_location: + row: 2 + column: 20 + fix: + content: "f\"{a} {b}\"" + location: + row: 2 + column: 0 + end_location: + row: 2 + column: 20 + parent: ~ +- kind: + FString: ~ + location: + row: 6 + column: 0 + end_location: + row: 6 + column: 19 + fix: + content: "f\"{z.y}\"" + location: + row: 6 + column: 0 + end_location: + row: 6 + column: 19 + parent: ~ +- kind: + FString: ~ + location: + row: 8 + column: 0 + end_location: + row: 8 + column: 24 + fix: + content: "f\"{a.x} {b.y}\"" + location: + row: 8 + column: 0 + end_location: + row: 8 + column: 24 + parent: ~ +- kind: + FString: ~ + location: + row: 10 + column: 0 + end_location: + row: 10 + column: 24 + fix: + content: "f\"{a.b} {c.d}\"" + location: + row: 10 + column: 0 + end_location: + row: 10 + column: 24 + parent: ~ +- kind: + FString: ~ + location: + row: 12 + column: 0 + end_location: + row: 12 + column: 16 + fix: + content: "f\"{a()}\"" + location: + row: 12 + column: 0 + end_location: + row: 12 + column: 16 + parent: ~ +- kind: + FString: ~ + location: + row: 14 + column: 0 + end_location: + row: 14 + column: 18 + fix: + content: "f\"{a.b()}\"" + location: + row: 14 + column: 0 + end_location: + row: 14 + column: 18 + parent: ~ +- kind: + FString: ~ + location: + row: 16 + column: 0 + end_location: + row: 16 + column: 22 + fix: + content: "f\"{a.b().c()}\"" + location: + row: 16 + column: 0 + end_location: + row: 16 + column: 22 + parent: ~ +- kind: + FString: ~ + location: + row: 18 + column: 0 + end_location: + row: 18 + column: 24 + fix: + content: "f\"hello {name}!\"" + location: + row: 18 + column: 0 + end_location: + row: 18 + column: 24 + parent: ~ +- kind: + FString: ~ + location: + row: 20 + column: 0 + end_location: + row: 20 + column: 27 + fix: + content: "f\"{a}{b}{c}\"" + location: + row: 20 + column: 0 + end_location: + row: 20 + column: 27 + parent: ~ +- kind: + FString: ~ + location: + row: 22 + column: 0 + end_location: + row: 22 + column: 16 + fix: + content: "f\"{0x0}\"" + location: + row: 22 + column: 0 + end_location: + row: 22 + column: 16 + parent: ~ +- kind: + FString: ~ + location: + row: 24 + column: 0 + end_location: + row: 24 + column: 18 + fix: + content: "f\"foo{1}\"" + location: + row: 24 + column: 0 + end_location: + row: 24 + column: 18 + parent: ~ +- kind: + FString: ~ + location: + row: 26 + column: 4 + end_location: + row: 26 + column: 21 + fix: + content: "f\"{1}\"" + location: + row: 26 + column: 4 + end_location: + row: 26 + column: 21 + parent: ~ +- kind: + FString: ~ + location: + row: 28 + column: 6 + end_location: + row: 28 + column: 25 + fix: + content: "f\"foo {x} \"" + location: + row: 28 + column: 6 + end_location: + row: 28 + column: 25 + parent: ~ + From be5029b33981136d4ea003c7cdbefaa02500c127 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 16 Jan 2023 21:21:08 -0500 Subject: [PATCH 21/26] Minor refactors --- resources/test/fixtures/pyupgrade/UP032.py | 26 +- src/checkers/ast.rs | 4 +- src/rules/pyupgrade/rules/f_strings.rs | 337 ++++++++++----------- 3 files changed, 173 insertions(+), 194 deletions(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index 794464eece0ea9..42ece0e55a3b96 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -1,31 +1,31 @@ # These SHOULD change -f"{a} {b}" +"{} {}".format(a, b) "{1} {0}".format(a, b) -f"{z.y}" +"{x.y}".format(x=z) -f"{a.x} {b.y}" +"{.x} {.y}".format(a, b) -f"{a.b} {c.d}" +"{} {}".format(a.b, c.d) -f"{a()}" +"{}".format(a()) -f"{a.b()}" +"{}".format(a.b()) -f"{a.b().c()}" +"{}".format(a.b().c()) -f"hello {name}!" +"hello {}!".format(name) -f"{a}{b}{c}" +"{}{b}{}".format(a, c, b=b) -f"{0x0}" +"{}".format(0x0) -f"foo{1}" +u"foo{}".format(1) -x = f"{1}" +x = "{a}".format(a=1) -print(f"foo {x} ") +print("foo {} ".format(x)) # Waiting for a response to solve this one # "{}{{}}{}".format(escaped, y) diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 18a192227535a0..cf642b9f5e65c0 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -1891,8 +1891,8 @@ where } Ok(summary) => { if self.settings.enabled.contains(&RuleCode::F522) { - pyflakes::rules::string_dot_format_extra_named_arguments(self, - &summary, keywords, location, + pyflakes::rules::string_dot_format_extra_named_arguments( + self, &summary, keywords, location, ); } diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index e5b1622be6f5cb..a877ca6ece0540 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -1,76 +1,67 @@ -use std::collections::HashMap; - +use anyhow::{anyhow, Result}; use once_cell::sync::Lazy; use regex::{Captures, Regex}; +use rustc_hash::FxHashMap; use rustpython_ast::{Constant, Expr, ExprKind, KeywordData}; use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::fix::Fix; -use crate::registry::Diagnostic; +use crate::registry::{Diagnostic, RuleCode}; use crate::rules::pyflakes::format::FormatSummary; use crate::violations; -// Checks for curly brackets. Inside these brackets this checks for an optional -// valid python name and any format specifiers afterwards static NAME_SPECIFIER: Lazy = Lazy::new(|| Regex::new(r"\{(?P[^\W0-9]\w*)?(?P.*?)}").unwrap()); -static HAS_BRACKETS: Lazy = Lazy::new(|| Regex::new(r"\[.*\]").unwrap()); +static HAS_BRACKETS: Lazy = Lazy::new(|| Regex::new(r"\[.*]").unwrap()); +/// Like [`FormatSummary`], but maps positional and keyword arguments to their +/// values. For example, given `{a} {b}".format(a=1, b=2)`, `FormatFunction` +/// would include `"a"` and `'b'` in `kwargs`, mapped to `1` and `2` +/// respectively. #[derive(Debug)] -struct FormatFunction { +struct FormatSummaryValues<'a> { args: Vec, - kwargs: HashMap, - // Whether or not something invalid was found, if it was return immediately - invalid: bool, -} - -/// Whether the given string contains characters that are FORBIDDEN in args and -/// kwargs -fn contains_invalids(string: &str) -> bool { - let invalids = vec!["*", "'", "\"", "await"]; - for invalid in invalids { - if string.contains(invalid) { - return true; - } - } - false + kwargs: FxHashMap<&'a str, String>, } -impl FormatFunction { - fn from_expr(checker: &mut Checker, expr: &Expr) -> Self { - let mut final_args: Vec = Vec::new(); - let mut final_kwargs: HashMap = HashMap::new(); - let mut invalid = false; +impl<'a> FormatSummaryValues<'a> { + fn try_from_expr(checker: &'a Checker, expr: &'a Expr) -> Option { + let mut extracted_args: Vec = Vec::new(); + let mut extracted_kwargs: FxHashMap<&str, String> = FxHashMap::default(); if let ExprKind::Call { args, keywords, .. } = &expr.node { for arg in args { - let arg_range = Range::from_located(arg); - let arg_string = checker.locator.slice_source_code_range(&arg_range); - invalid = contains_invalids(&arg_string); - final_args.push(arg_string.to_string()); + let arg = checker + .locator + .slice_source_code_range(&Range::from_located(arg)); + if contains_invalids(&arg) { + return None; + } + extracted_args.push(arg.to_string()); } - for keyword in keywords { let KeywordData { arg, value } = &keyword.node; if let Some(key) = arg { - let kwarg_range = Range::from_located(value); - let kwarg_string = checker.locator.slice_source_code_range(&kwarg_range); - invalid = contains_invalids(&kwarg_string); - final_kwargs.insert(key.to_string(), kwarg_string.to_string()); + let kwarg = checker + .locator + .slice_source_code_range(&Range::from_located(value)); + if contains_invalids(&kwarg) { + return None; + } + extracted_kwargs.insert(key, kwarg.to_string()); } } } - Self { - args: final_args, - kwargs: final_kwargs, - invalid, + + if extracted_args.is_empty() && extracted_kwargs.is_empty() { + return None; } - } - /// Returns true if args and kwargs are empty - fn is_empty(&self) -> bool { - self.args.is_empty() && self.kwargs.is_empty() + Some(Self { + args: extracted_args, + kwargs: extracted_kwargs, + }) } fn consume_arg(&mut self) -> Option { @@ -81,137 +72,141 @@ impl FormatFunction { } } - fn get_kwarg(&self, key: &str) -> Option { - self.kwargs.get(key).cloned() + fn consume_kwarg(&mut self, key: &str) -> Option { + self.kwargs.remove(key) } - /// Returns true if the statement and function call match, and false if not - fn check_with_summary(&self, summary: &FormatSummary) -> bool { + /// Return `true` if the statement and function call match. + fn validate(&self, summary: &FormatSummary) -> bool { let mut self_keys = self.kwargs.clone().into_keys().collect::>(); - self_keys.sort(); + self_keys.sort_unstable(); + let mut summary_keys = summary.keywords.clone(); summary_keys.sort(); + summary.autos.len() == self.args.len() && self_keys == summary_keys } } -fn extract_caps(caps: &Captures, target: &str) -> Result { - let new_string = match caps.name(target) { - None => return Err(()), - Some(item) => item.as_str(), +/// Return `true` if the string contains characters that are forbidden in +/// argument identifier. +fn contains_invalids(string: &str) -> bool { + string.contains('*') + || string.contains('\'') + || string.contains('"') + || string.contains("await") +} + +/// Extract the format spec from a regex [`Captures`] object. +fn extract_format_spec(caps: &Captures, target: &str) -> Result { + let Some(match_) = caps.name(target) else { + return Err(anyhow!("No match for target: {}", target)); }; - if HAS_BRACKETS.is_match(new_string) { - return Err(()); + let match_ = match_.as_str(); + if HAS_BRACKETS.is_match(match_) { + return Err(anyhow!("Invalid match for target: {}", target)); } - Ok(new_string.to_string()) + Ok(match_.to_string()) } -fn create_new_string(expr: &Expr, function: &mut FormatFunction) -> Option { - let mut new_string = String::new(); - if let ExprKind::Call { func, .. } = &expr.node { - if let ExprKind::Attribute { value, .. } = &func.node { - if let ExprKind::Constant { value, kind } = &value.node { - // Do NOT refactor byte strings - if let Some(kind_str) = kind { - if kind_str == "b" { - return None; - } - } - if let Constant::Str(string) = value { - new_string = string.to_string(); - } - } - } +// See: https://github.com/rust-lang/regex/issues/648 +fn replace_all( + re: &Regex, + haystack: &str, + mut replacement: impl FnMut(&Captures) -> Result, +) -> Result { + let mut new = String::with_capacity(haystack.len()); + let mut last_match = 0; + for caps in re.captures_iter(haystack) { + let m = caps.get(0).unwrap(); + new.push_str(&haystack[last_match..m.start()]); + new.push_str(&replacement(&caps)?); + last_match = m.end(); } - // If we didn't get a valid string, return an empty string - if new_string.is_empty() { + new.push_str(&haystack[last_match..]); + Ok(new) +} + +/// Generate an f-string from an [`Expr`]. +fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { + let ExprKind::Call { func, .. } = &expr.node else { + return None; + }; + let ExprKind::Attribute { value, .. } = &func.node else { + return None; + }; + let ExprKind::Constant { value, .. } = &value.node else { + return None; + }; + let Constant::Str(string) = value else { + return None; + }; + + let contents = string.to_string(); + if contents.is_empty() { return None; } + + let Some(mut summary) = FormatSummaryValues::try_from_expr(checker, expr) else { + return None; + }; + // You can't return a function from inside a closure, so we just record that - // there was an error - let mut had_error = false; - let clean_string = NAME_SPECIFIER.replace_all(&new_string, |caps: &Captures| { + // there was an error. + let clean_string = replace_all(&NAME_SPECIFIER, &contents, |caps: &Captures| { if let Some(name) = caps.name("name") { - let Some(kwarg) = function.get_kwarg(name.as_str()) else { - had_error = true; - return String::new(); + let Some(value) = summary.consume_kwarg(name.as_str()) else { + return Err(anyhow!("Missing kwarg")); }; - if let Ok(second_part) = extract_caps(caps, "fmt") { - format!("{{{kwarg}{second_part}}}") - } else { - had_error = true; - "badstring".to_string() - } + let Ok(format_spec) = extract_format_spec(caps, "fmt") else { + return Err(anyhow!("Missing caps")); + }; + Ok(format!("{{{value}{format_spec}}}")) } else { - let Some(arg) = function.consume_arg() else { - had_error = true; - return String::new(); + let Some(value) = summary.consume_arg() else { + return Err(anyhow!("Missing arg")); }; - if let Ok(second_part) = extract_caps(caps, "fmt") { - format!("{{{arg}{second_part}}}") - } else { - had_error = true; - "badstring".to_string() - } + let Ok(format_spec) = extract_format_spec(caps, "fmt") else { + return Err(anyhow!("Missing caps")); + }; + Ok(format!("{{{value}{format_spec}}}")) } - }); - if had_error { - return None; - } + }) + .ok()?; Some(format!("f\"{clean_string}\"")) } -fn generate_f_string( - checker: &mut Checker, - summary: &FormatSummary, - expr: &Expr, -) -> Option { - let mut original_call = FormatFunction::from_expr(checker, expr); - - // If there were any invalid characters we should return immediately - if original_call.invalid { - return None; - } - // We do not need to make changes if there are no arguments (let me know if you - // want me to change this to removing the .format() and doing nothing else, - // but that seems like it could cause issues) - if original_call.is_empty() { - return None; - } - // If the actual string and the format function do not match, we cannot operate - if !original_call.check_with_summary(summary) { - return None; - } - create_new_string(expr, &mut original_call) -} - /// UP032 pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) { - let expr_range = Range::from_located(expr); - let expr_string = checker.locator.slice_source_code_range(&expr_range); - // Pyupgrade says we should not try and refactor multi-line statements - if expr_string.contains('\n') { - return; - } if summary.has_nested_parts { return; } - // UP030 already removes the indexes, so we should not need to worry about the - // complexity if !summary.indexes.is_empty() { return; } + + let existing = checker + .locator + .slice_source_code_range(&Range::from_located(expr)); + + // Avoid refactoring multi-line strings. + if existing.contains('\n') { + return; + } + // Currently, the only issue we know of is in LibCST: // https://github.com/Instagram/LibCST/issues/846 - let Some(contents) = generate_f_string(checker, summary, expr) else { + let Some(contents) = try_convert_to_f_string(checker, expr) else { return; }; - // Don't refactor if it will make the string longer - if contents.len() > expr_string.len() { + + // Avoid refactors that increase the resulting string length. + if contents.len() > existing.len() { return; } + let mut diagnostic = Diagnostic::new(violations::FString, Range::from_located(expr)); - if checker.patch(diagnostic.kind.code()) { + if checker.patch(&RuleCode::UP032) { diagnostic.amend(Fix::replacement( contents, expr.location, @@ -226,94 +221,78 @@ mod tests { use super::*; #[test] - fn test_check_with_summary() { + fn test_validate() { let summary = FormatSummary { autos: vec![0, 1], keywords: vec!["c".to_string(), "d".to_string()], has_nested_parts: false, indexes: vec![], }; - let form_func = FormatFunction { + let form_func = FormatSummaryValues { args: vec!["a".to_string(), "b".to_string()], - kwargs: [ - ("c".to_string(), "e".to_string()), - ("d".to_string(), "f".to_string()), - ] - .iter() - .cloned() - .collect(), - invalid: false, + kwargs: [("c", "e".to_string()), ("d", "f".to_string())] + .iter() + .cloned() + .collect(), }; - let checks_out = form_func.check_with_summary(&summary); + let checks_out = form_func.validate(&summary); assert!(checks_out); } #[test] - fn test_check_with_summary_unuequal_args() { + fn test_validate_unequal_args() { let summary = FormatSummary { autos: vec![0, 1], keywords: vec!["c".to_string()], has_nested_parts: false, indexes: vec![], }; - let form_func = FormatFunction { + let form_func = FormatSummaryValues { args: vec!["a".to_string(), "b".to_string()], - kwargs: [ - ("c".to_string(), "e".to_string()), - ("d".to_string(), "f".to_string()), - ] - .iter() - .cloned() - .collect(), - invalid: false, + kwargs: [("c", "e".to_string()), ("d", "f".to_string())] + .iter() + .cloned() + .collect(), }; - let checks_out = form_func.check_with_summary(&summary); + let checks_out = form_func.validate(&summary); assert!(!checks_out); } #[test] - fn test_check_with_summary_different_kwargs() { + fn test_validate_different_kwargs() { let summary = FormatSummary { autos: vec![0, 1], keywords: vec!["c".to_string(), "d".to_string()], has_nested_parts: false, indexes: vec![], }; - let form_func = FormatFunction { + let form_func = FormatSummaryValues { args: vec!["a".to_string(), "b".to_string()], - kwargs: [ - ("c".to_string(), "e".to_string()), - ("e".to_string(), "f".to_string()), - ] - .iter() - .cloned() - .collect(), - invalid: false, + kwargs: [("c", "e".to_string()), ("e", "f".to_string())] + .iter() + .cloned() + .collect(), }; - let checks_out = form_func.check_with_summary(&summary); + let checks_out = form_func.validate(&summary); assert!(!checks_out); } #[test] - fn test_check_with_summary_kwargs_same_diff_order() { + fn test_validate_kwargs_same_diff_order() { let summary = FormatSummary { autos: vec![0, 1], keywords: vec!["c".to_string(), "d".to_string()], has_nested_parts: false, indexes: vec![], }; - let form_func = FormatFunction { + let form_func = FormatSummaryValues { args: vec!["a".to_string(), "b".to_string()], - kwargs: [ - ("d".to_string(), "e".to_string()), - ("c".to_string(), "f".to_string()), - ] - .iter() - .cloned() - .collect(), - invalid: false, + kwargs: [("d", "e".to_string()), ("c", "f".to_string())] + .iter() + .cloned() + .collect(), }; - let checks_out = form_func.check_with_summary(&summary); + let checks_out = form_func.validate(&summary); assert!(checks_out); } } From 0e7fcd8bd3ba686e9ea934dd15110caaff93cd52 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 16 Jan 2023 21:49:47 -0500 Subject: [PATCH 22/26] Minor refactors --- resources/test/fixtures/pyupgrade/UP032.py | 6 + src/rules/pyupgrade/rules/f_strings.rs | 132 ++++-------------- ...les__pyupgrade__tests__UP032_UP032.py.snap | 71 ++++++++-- src/source_code/stylist.rs | 9 ++ 4 files changed, 102 insertions(+), 116 deletions(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index 42ece0e55a3b96..639f7056070cfa 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -21,8 +21,14 @@ "{}".format(0x0) +'{} {}'.format(a, b) + +'''{} {}'''.format(a, b) + u"foo{}".format(1) +r"foo{}".format(1) + x = "{a}".format(a=1) print("foo {} ".format(x)) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index a877ca6ece0540..898e1125809f62 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -75,17 +75,6 @@ impl<'a> FormatSummaryValues<'a> { fn consume_kwarg(&mut self, key: &str) -> Option { self.kwargs.remove(key) } - - /// Return `true` if the statement and function call match. - fn validate(&self, summary: &FormatSummary) -> bool { - let mut self_keys = self.kwargs.clone().into_keys().collect::>(); - self_keys.sort_unstable(); - - let mut summary_keys = summary.keywords.clone(); - summary_keys.sort(); - - summary.autos.len() == self.args.len() && self_keys == summary_keys - } } /// Return `true` if the string contains characters that are forbidden in @@ -135,14 +124,24 @@ fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { let ExprKind::Attribute { value, .. } = &func.node else { return None; }; - let ExprKind::Constant { value, .. } = &value.node else { - return None; - }; - let Constant::Str(string) = value else { + if !matches!( + &value.node, + ExprKind::Constant { + value: Constant::Str(..), + .. + }, + ) { return None; }; - let contents = string.to_string(); + let contents = checker + .locator + .slice_source_code_range(&Range::from_located(value)); + let contents = if contents.starts_with('U') || contents.starts_with('u') { + &contents[1..] + } else { + &contents + }; if contents.is_empty() { return None; } @@ -151,9 +150,7 @@ fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { return None; }; - // You can't return a function from inside a closure, so we just record that - // there was an error. - let clean_string = replace_all(&NAME_SPECIFIER, &contents, |caps: &Captures| { + let converted = replace_all(&NAME_SPECIFIER, contents, |caps: &Captures| { if let Some(name) = caps.name("name") { let Some(value) = summary.consume_kwarg(name.as_str()) else { return Err(anyhow!("Missing kwarg")); @@ -173,7 +170,12 @@ fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { } }) .ok()?; - Some(format!("f\"{clean_string}\"")) + + // Construct the format string. + let mut contents = String::with_capacity(1 + converted.len()); + contents.push('f'); + contents.push_str(&converted); + Some(contents) } /// UP032 @@ -185,12 +187,8 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E return; } - let existing = checker - .locator - .slice_source_code_range(&Range::from_located(expr)); - // Avoid refactoring multi-line strings. - if existing.contains('\n') { + if expr.location.row() != expr.end_location.unwrap().row() { return; } @@ -201,6 +199,9 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E }; // Avoid refactors that increase the resulting string length. + let existing = checker + .locator + .slice_source_code_range(&Range::from_located(expr)); if contents.len() > existing.len() { return; } @@ -215,84 +216,3 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E }; checker.diagnostics.push(diagnostic); } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_validate() { - let summary = FormatSummary { - autos: vec![0, 1], - keywords: vec!["c".to_string(), "d".to_string()], - has_nested_parts: false, - indexes: vec![], - }; - let form_func = FormatSummaryValues { - args: vec!["a".to_string(), "b".to_string()], - kwargs: [("c", "e".to_string()), ("d", "f".to_string())] - .iter() - .cloned() - .collect(), - }; - let checks_out = form_func.validate(&summary); - assert!(checks_out); - } - - #[test] - fn test_validate_unequal_args() { - let summary = FormatSummary { - autos: vec![0, 1], - keywords: vec!["c".to_string()], - has_nested_parts: false, - indexes: vec![], - }; - let form_func = FormatSummaryValues { - args: vec!["a".to_string(), "b".to_string()], - kwargs: [("c", "e".to_string()), ("d", "f".to_string())] - .iter() - .cloned() - .collect(), - }; - let checks_out = form_func.validate(&summary); - assert!(!checks_out); - } - - #[test] - fn test_validate_different_kwargs() { - let summary = FormatSummary { - autos: vec![0, 1], - keywords: vec!["c".to_string(), "d".to_string()], - has_nested_parts: false, - indexes: vec![], - }; - let form_func = FormatSummaryValues { - args: vec!["a".to_string(), "b".to_string()], - kwargs: [("c", "e".to_string()), ("e", "f".to_string())] - .iter() - .cloned() - .collect(), - }; - let checks_out = form_func.validate(&summary); - assert!(!checks_out); - } - - #[test] - fn test_validate_kwargs_same_diff_order() { - let summary = FormatSummary { - autos: vec![0, 1], - keywords: vec!["c".to_string(), "d".to_string()], - has_nested_parts: false, - indexes: vec![], - }; - let form_func = FormatSummaryValues { - args: vec!["a".to_string(), "b".to_string()], - kwargs: [("d", "e".to_string()), ("c", "f".to_string())] - .iter() - .cloned() - .collect(), - }; - let checks_out = form_func.validate(&summary); - assert!(checks_out); - } -} diff --git a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap index fc88e966a62134..83c71c78707800 100644 --- a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap +++ b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap @@ -179,48 +179,99 @@ expression: diagnostics column: 0 end_location: row: 24 - column: 18 + column: 20 fix: - content: "f\"foo{1}\"" + content: "f'{a} {b}'" location: row: 24 column: 0 end_location: row: 24 - column: 18 + column: 20 parent: ~ - kind: FString: ~ location: row: 26 - column: 4 + column: 0 end_location: row: 26 + column: 24 + fix: + content: "f'''{a} {b}'''" + location: + row: 26 + column: 0 + end_location: + row: 26 + column: 24 + parent: ~ +- kind: + FString: ~ + location: + row: 28 + column: 0 + end_location: + row: 28 + column: 18 + fix: + content: "f\"foo{1}\"" + location: + row: 28 + column: 0 + end_location: + row: 28 + column: 18 + parent: ~ +- kind: + FString: ~ + location: + row: 30 + column: 0 + end_location: + row: 30 + column: 18 + fix: + content: "fr\"foo{1}\"" + location: + row: 30 + column: 0 + end_location: + row: 30 + column: 18 + parent: ~ +- kind: + FString: ~ + location: + row: 32 + column: 4 + end_location: + row: 32 column: 21 fix: content: "f\"{1}\"" location: - row: 26 + row: 32 column: 4 end_location: - row: 26 + row: 32 column: 21 parent: ~ - kind: FString: ~ location: - row: 28 + row: 34 column: 6 end_location: - row: 28 + row: 34 column: 25 fix: content: "f\"foo {x} \"" location: - row: 28 + row: 34 column: 6 end_location: - row: 28 + row: 34 column: 25 parent: ~ diff --git a/src/source_code/stylist.rs b/src/source_code/stylist.rs index 8d20da4ea73b41..073b3d6042eeba 100644 --- a/src/source_code/stylist.rs +++ b/src/source_code/stylist.rs @@ -61,6 +61,15 @@ impl Default for Quote { } } +impl From for char { + fn from(val: Quote) -> Self { + match val { + Quote::Single => '\'', + Quote::Double => '"', + } + } +} + impl From<&Quote> for vendor::str::Quote { fn from(val: &Quote) -> Self { match val { From bf83d2eb21780e7425432b511435d26a21265c5b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 16 Jan 2023 22:33:32 -0500 Subject: [PATCH 23/26] Use RustPython parser --- foo.py | 7 + resources/test/fixtures/pyupgrade/UP032.py | 37 +- src/docstrings/constants.rs | 4 + src/rules/pydocstyle/helpers.rs | 13 + src/rules/pyupgrade/rules/f_strings.rs | 133 +++++-- ..._pyupgrade__tests__UP032_UP032.py.snap.new | 363 ++++++++++++++++++ 6 files changed, 516 insertions(+), 41 deletions(-) create mode 100644 foo.py create mode 100644 src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap.new diff --git a/foo.py b/foo.py new file mode 100644 index 00000000000000..e73b09d391e816 --- /dev/null +++ b/foo.py @@ -0,0 +1,7 @@ +z = 1 + +# "{.1}".format(z) + +"{} {0} {}".format(z, z, z) + +# "{x}".format(x=z) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index 639f7056070cfa..b195b085a5c3d3 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -21,11 +21,11 @@ "{}".format(0x0) -'{} {}'.format(a, b) +"{} {}".format(a, b) -'''{} {}'''.format(a, b) +"""{} {}""".format(a, b) -u"foo{}".format(1) +"foo{}".format(1) r"foo{}".format(1) @@ -33,11 +33,14 @@ print("foo {} ".format(x)) -# Waiting for a response to solve this one -# "{}{{}}{}".format(escaped, y) +"{a[b]}".format(a=a) + +"{a.a[b]}".format(a=a) + + +"{}{{}}{}".format(escaped, y) -# Waiting on Charlie solution to solve this -# r'"\N{snowman} {}".format(a)' +"\N{snowman} {}".format(a) # These should NOT change @@ -45,9 +48,7 @@ "}".format(a) -"{}".format( - a -) +"{}".format(a) "{} {}".format(*a) @@ -61,16 +62,18 @@ "{:{}}".format(x, y) -"{a[b]}".format(a=a) +"{}{}".format(a) -"{a.a[b]}".format(a=a) +"" "{}".format(a["\\"]) -"{}{}".format(a) +"{}".format(a["b"]) + +r'"\N{snowman} {}".format(a)' -''"{}".format(a['\\']) -'{}'.format(a['b']) +async def c(): + return "{}".format(await 3) -async def c(): return '{}'.format(await 3) -async def c(): return '{}'.format(1 + await 3) +async def c(): + return "{}".format(1 + await 3) diff --git a/src/docstrings/constants.rs b/src/docstrings/constants.rs index efdd0a1602fd23..3473b0c841cc58 100644 --- a/src/docstrings/constants.rs +++ b/src/docstrings/constants.rs @@ -7,3 +7,7 @@ pub const TRIPLE_QUOTE_PREFIXES: &[&str] = &[ pub const SINGLE_QUOTE_PREFIXES: &[&str] = &[ "u\"", "u'", "r\"", "r'", "u\"", "u'", "r\"", "r'", "U\"", "U'", "R\"", "R'", "\"", "'", ]; + +pub const TRIPLE_QUOTE_SUFFIXES: &[&str] = &["\"\"\"", "'''"]; + +pub const SINGLE_QUOTE_SUFFIXES: &[&str] = &["\"", "'"]; diff --git a/src/rules/pydocstyle/helpers.rs b/src/rules/pydocstyle/helpers.rs index 49138be22a6709..0d04b09b36b00c 100644 --- a/src/rules/pydocstyle/helpers.rs +++ b/src/rules/pydocstyle/helpers.rs @@ -30,6 +30,19 @@ pub fn leading_quote(content: &str) -> Option<&str> { None } +/// Return the trailing quote string for a docstring (e.g., `"""`). +pub fn trailing_quote(content: &str) -> Option<&str> { + for pattern in constants::SINGLE_QUOTE_SUFFIXES + .iter() + .chain(constants::SINGLE_QUOTE_SUFFIXES) + { + if content.ends_with(pattern) { + return Some(pattern); + } + } + None +} + /// Return the index of the first logical line in a string. pub fn logical_line(content: &str) -> Option { // Find the first logical line. diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index 898e1125809f62..994b437f49671c 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -3,11 +3,15 @@ use once_cell::sync::Lazy; use regex::{Captures, Regex}; use rustc_hash::FxHashMap; use rustpython_ast::{Constant, Expr, ExprKind, KeywordData}; +use rustpython_common::format::{ + FieldName, FieldNamePart, FieldType, FormatPart, FormatString, FromTemplate, +}; use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::fix::Fix; use crate::registry::{Diagnostic, RuleCode}; +use crate::rules::pydocstyle::helpers::{leading_quote, trailing_quote}; use crate::rules::pyflakes::format::FormatSummary; use crate::violations; @@ -64,7 +68,7 @@ impl<'a> FormatSummaryValues<'a> { }) } - fn consume_arg(&mut self) -> Option { + fn consume_next(&mut self) -> Option { if self.args.is_empty() { None } else { @@ -72,6 +76,14 @@ impl<'a> FormatSummaryValues<'a> { } } + fn consume_arg(&mut self, index: usize) -> Option { + if self.args.len() > index { + Some(self.args.remove(index)) + } else { + None + } + } + fn consume_kwarg(&mut self, key: &str) -> Option { self.kwargs.remove(key) } @@ -134,9 +146,16 @@ fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { return None; }; + let Some(mut summary) = FormatSummaryValues::try_from_expr(checker, expr) else { + return None; + }; + let contents = checker .locator .slice_source_code_range(&Range::from_located(value)); + + // Strip the unicode prefix. It's redundant in Python 3, and invalid when used + // with f-strings. let contents = if contents.starts_with('U') || contents.starts_with('u') { &contents[1..] } else { @@ -146,35 +165,104 @@ fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { return None; } - let Some(mut summary) = FormatSummaryValues::try_from_expr(checker, expr) else { + // Remove the leading and trailing quotes. + let Some(leading_quote) = leading_quote(contents) else { + return None; + }; + let Some(trailing_quote) = trailing_quote(contents) else { return None; }; + let contents = &contents[leading_quote.len()..contents.len() - trailing_quote.len()]; - let converted = replace_all(&NAME_SPECIFIER, contents, |caps: &Captures| { - if let Some(name) = caps.name("name") { - let Some(value) = summary.consume_kwarg(name.as_str()) else { - return Err(anyhow!("Missing kwarg")); - }; - let Ok(format_spec) = extract_format_spec(caps, "fmt") else { - return Err(anyhow!("Missing caps")); - }; - Ok(format!("{{{value}{format_spec}}}")) - } else { - let Some(value) = summary.consume_arg() else { - return Err(anyhow!("Missing arg")); - }; - let Ok(format_spec) = extract_format_spec(caps, "fmt") else { - return Err(anyhow!("Missing caps")); - }; - Ok(format!("{{{value}{format_spec}}}")) + // Parse the format string. + let Ok(format_string) = FormatString::from_str(contents) else { + println!("Failed to parse format string: {}", contents); + return None; + }; + + println!("format_string: {:?}", format_string); + + let mut converted = String::with_capacity(contents.len()); + for part in format_string.format_parts { + match part { + FormatPart::Field { + field_name, + preconversion_spec, + format_spec, + } => { + converted.push('{'); + + let field = FieldName::parse(&field_name).ok()?; + match field.field_type { + FieldType::Auto => { + let Some(arg) = summary.consume_next() else { + return None; + }; + converted.push_str(&arg); + } + FieldType::Index(index) => { + let Some(arg) = summary.consume_arg(index) else { + return None; + }; + converted.push_str(&arg); + } + FieldType::Keyword(name) => { + let Some(arg) = summary.consume_kwarg(&name) else { + return None; + }; + converted.push_str(&arg); + } + } + + for part in field.parts { + match part { + FieldNamePart::Attribute(name) => { + converted.push('.'); + converted.push_str(&name); + } + FieldNamePart::Index(index) => { + converted.push('['); + converted.push_str(index.to_string().as_str()); + converted.push(']'); + } + FieldNamePart::StringIndex(index) => { + converted.push('['); + converted.push_str(&index); + converted.push(']'); + } + } + } + + if let Some(preconversion_spec) = preconversion_spec { + converted.push('!'); + converted.push(preconversion_spec); + } + + if !format_spec.is_empty() { + converted.push(':'); + converted.push_str(&format_spec); + } + + converted.push('}'); + } + FormatPart::Literal(value) => { + if value.starts_with('{') { + converted.push_str("{{"); + } + converted.push_str(&value); + if value.ends_with('}') { + converted.push_str("}}"); + } + } } - }) - .ok()?; + } // Construct the format string. let mut contents = String::with_capacity(1 + converted.len()); contents.push('f'); + contents.push_str(&leading_quote); contents.push_str(&converted); + contents.push_str(&trailing_quote); Some(contents) } @@ -183,9 +271,6 @@ pub(crate) fn f_strings(checker: &mut Checker, summary: &FormatSummary, expr: &E if summary.has_nested_parts { return; } - if !summary.indexes.is_empty() { - return; - } // Avoid refactoring multi-line strings. if expr.location.row() != expr.end_location.unwrap().row() { diff --git a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap.new b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap.new new file mode 100644 index 00000000000000..f9d70b38cbd31e --- /dev/null +++ b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap.new @@ -0,0 +1,363 @@ +--- +source: src/rules/pyupgrade/mod.rs +assertion_line: 65 +expression: diagnostics +--- +- kind: + FString: ~ + location: + row: 2 + column: 0 + end_location: + row: 2 + column: 20 + fix: + content: "f\"{a} {b}\"" + location: + row: 2 + column: 0 + end_location: + row: 2 + column: 20 + parent: ~ +- kind: + FString: ~ + location: + row: 4 + column: 0 + end_location: + row: 4 + column: 22 + fix: + content: "f\"{b} {a}\"" + location: + row: 4 + column: 0 + end_location: + row: 4 + column: 22 + parent: ~ +- kind: + FString: ~ + location: + row: 6 + column: 0 + end_location: + row: 6 + column: 19 + fix: + content: "f\"{z.y}\"" + location: + row: 6 + column: 0 + end_location: + row: 6 + column: 19 + parent: ~ +- kind: + FString: ~ + location: + row: 8 + column: 0 + end_location: + row: 8 + column: 24 + fix: + content: "f\"{a.x} {b.y}\"" + location: + row: 8 + column: 0 + end_location: + row: 8 + column: 24 + parent: ~ +- kind: + FString: ~ + location: + row: 10 + column: 0 + end_location: + row: 10 + column: 24 + fix: + content: "f\"{a.b} {c.d}\"" + location: + row: 10 + column: 0 + end_location: + row: 10 + column: 24 + parent: ~ +- kind: + FString: ~ + location: + row: 12 + column: 0 + end_location: + row: 12 + column: 16 + fix: + content: "f\"{a()}\"" + location: + row: 12 + column: 0 + end_location: + row: 12 + column: 16 + parent: ~ +- kind: + FString: ~ + location: + row: 14 + column: 0 + end_location: + row: 14 + column: 18 + fix: + content: "f\"{a.b()}\"" + location: + row: 14 + column: 0 + end_location: + row: 14 + column: 18 + parent: ~ +- kind: + FString: ~ + location: + row: 16 + column: 0 + end_location: + row: 16 + column: 22 + fix: + content: "f\"{a.b().c()}\"" + location: + row: 16 + column: 0 + end_location: + row: 16 + column: 22 + parent: ~ +- kind: + FString: ~ + location: + row: 18 + column: 0 + end_location: + row: 18 + column: 24 + fix: + content: "f\"hello {name}!\"" + location: + row: 18 + column: 0 + end_location: + row: 18 + column: 24 + parent: ~ +- kind: + FString: ~ + location: + row: 20 + column: 0 + end_location: + row: 20 + column: 27 + fix: + content: "f\"{a}{b}{c}\"" + location: + row: 20 + column: 0 + end_location: + row: 20 + column: 27 + parent: ~ +- kind: + FString: ~ + location: + row: 22 + column: 0 + end_location: + row: 22 + column: 16 + fix: + content: "f\"{0x0}\"" + location: + row: 22 + column: 0 + end_location: + row: 22 + column: 16 + parent: ~ +- kind: + FString: ~ + location: + row: 24 + column: 0 + end_location: + row: 24 + column: 20 + fix: + content: "f\"{a} {b}\"" + location: + row: 24 + column: 0 + end_location: + row: 24 + column: 20 + parent: ~ +- kind: + FString: ~ + location: + row: 26 + column: 0 + end_location: + row: 26 + column: 24 + fix: + content: "f\"\"\"{a} {b}\"\"\"" + location: + row: 26 + column: 0 + end_location: + row: 26 + column: 24 + parent: ~ +- kind: + FString: ~ + location: + row: 28 + column: 0 + end_location: + row: 28 + column: 17 + fix: + content: "f\"foo{1}\"" + location: + row: 28 + column: 0 + end_location: + row: 28 + column: 17 + parent: ~ +- kind: + FString: ~ + location: + row: 30 + column: 0 + end_location: + row: 30 + column: 18 + fix: + content: "fr\"foo{1}\"" + location: + row: 30 + column: 0 + end_location: + row: 30 + column: 18 + parent: ~ +- kind: + FString: ~ + location: + row: 32 + column: 4 + end_location: + row: 32 + column: 21 + fix: + content: "f\"{1}\"" + location: + row: 32 + column: 4 + end_location: + row: 32 + column: 21 + parent: ~ +- kind: + FString: ~ + location: + row: 34 + column: 6 + end_location: + row: 34 + column: 25 + fix: + content: "f\"foo {x} \"" + location: + row: 34 + column: 6 + end_location: + row: 34 + column: 25 + parent: ~ +- kind: + FString: ~ + location: + row: 36 + column: 0 + end_location: + row: 36 + column: 20 + fix: + content: "f\"{a[b]}\"" + location: + row: 36 + column: 0 + end_location: + row: 36 + column: 20 + parent: ~ +- kind: + FString: ~ + location: + row: 38 + column: 0 + end_location: + row: 38 + column: 22 + fix: + content: "f\"{a.a[b]}\"" + location: + row: 38 + column: 0 + end_location: + row: 38 + column: 22 + parent: ~ +- kind: + FString: ~ + location: + row: 41 + column: 0 + end_location: + row: 41 + column: 29 + fix: + content: "f\"{escaped}{{{}}}{y}\"" + location: + row: 41 + column: 0 + end_location: + row: 41 + column: 29 + parent: ~ +- kind: + FString: ~ + location: + row: 51 + column: 0 + end_location: + row: 51 + column: 14 + fix: + content: "f\"{a}\"" + location: + row: 51 + column: 0 + end_location: + row: 51 + column: 14 + parent: ~ + From d87da2e9f6f3a0adcda98be13683941a07c7b328 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 16 Jan 2023 22:41:50 -0500 Subject: [PATCH 24/26] Use RustPython parser --- foo.py | 7 - src/rules/pydocstyle/helpers.rs | 11 +- src/rules/pyupgrade/rules/f_strings.rs | 50 +-- ...les__pyupgrade__tests__UP032_UP032.py.snap | 93 ++++- ..._pyupgrade__tests__UP032_UP032.py.snap.new | 363 ------------------ 5 files changed, 97 insertions(+), 427 deletions(-) delete mode 100644 foo.py delete mode 100644 src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap.new diff --git a/foo.py b/foo.py deleted file mode 100644 index e73b09d391e816..00000000000000 --- a/foo.py +++ /dev/null @@ -1,7 +0,0 @@ -z = 1 - -# "{.1}".format(z) - -"{} {0} {}".format(z, z, z) - -# "{x}".format(x=z) diff --git a/src/rules/pydocstyle/helpers.rs b/src/rules/pydocstyle/helpers.rs index 0d04b09b36b00c..6164b1c8ce463e 100644 --- a/src/rules/pydocstyle/helpers.rs +++ b/src/rules/pydocstyle/helpers.rs @@ -31,16 +31,11 @@ pub fn leading_quote(content: &str) -> Option<&str> { } /// Return the trailing quote string for a docstring (e.g., `"""`). -pub fn trailing_quote(content: &str) -> Option<&str> { - for pattern in constants::SINGLE_QUOTE_SUFFIXES +pub fn trailing_quote(content: &str) -> Option<&&str> { + constants::TRIPLE_QUOTE_SUFFIXES .iter() .chain(constants::SINGLE_QUOTE_SUFFIXES) - { - if content.ends_with(pattern) { - return Some(pattern); - } - } - None + .find(|&pattern| content.ends_with(pattern)) } /// Return the index of the first logical line in a string. diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index 994b437f49671c..642ebd4493c23a 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -1,6 +1,3 @@ -use anyhow::{anyhow, Result}; -use once_cell::sync::Lazy; -use regex::{Captures, Regex}; use rustc_hash::FxHashMap; use rustpython_ast::{Constant, Expr, ExprKind, KeywordData}; use rustpython_common::format::{ @@ -15,11 +12,6 @@ use crate::rules::pydocstyle::helpers::{leading_quote, trailing_quote}; use crate::rules::pyflakes::format::FormatSummary; use crate::violations; -static NAME_SPECIFIER: Lazy = - Lazy::new(|| Regex::new(r"\{(?P[^\W0-9]\w*)?(?P.*?)}").unwrap()); - -static HAS_BRACKETS: Lazy = Lazy::new(|| Regex::new(r"\[.*]").unwrap()); - /// Like [`FormatSummary`], but maps positional and keyword arguments to their /// values. For example, given `{a} {b}".format(a=1, b=2)`, `FormatFunction` /// would include `"a"` and `'b'` in `kwargs`, mapped to `1` and `2` @@ -98,36 +90,6 @@ fn contains_invalids(string: &str) -> bool { || string.contains("await") } -/// Extract the format spec from a regex [`Captures`] object. -fn extract_format_spec(caps: &Captures, target: &str) -> Result { - let Some(match_) = caps.name(target) else { - return Err(anyhow!("No match for target: {}", target)); - }; - let match_ = match_.as_str(); - if HAS_BRACKETS.is_match(match_) { - return Err(anyhow!("Invalid match for target: {}", target)); - } - Ok(match_.to_string()) -} - -// See: https://github.com/rust-lang/regex/issues/648 -fn replace_all( - re: &Regex, - haystack: &str, - mut replacement: impl FnMut(&Captures) -> Result, -) -> Result { - let mut new = String::with_capacity(haystack.len()); - let mut last_match = 0; - for caps in re.captures_iter(haystack) { - let m = caps.get(0).unwrap(); - new.push_str(&haystack[last_match..m.start()]); - new.push_str(&replacement(&caps)?); - last_match = m.end(); - } - new.push_str(&haystack[last_match..]); - Ok(new) -} - /// Generate an f-string from an [`Expr`]. fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { let ExprKind::Call { func, .. } = &expr.node else { @@ -176,12 +138,10 @@ fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { // Parse the format string. let Ok(format_string) = FormatString::from_str(contents) else { - println!("Failed to parse format string: {}", contents); + return None; }; - println!("format_string: {:?}", format_string); - let mut converted = String::with_capacity(contents.len()); for part in format_string.format_parts { match part { @@ -247,11 +207,11 @@ fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { } FormatPart::Literal(value) => { if value.starts_with('{') { - converted.push_str("{{"); + converted.push('{'); } converted.push_str(&value); if value.ends_with('}') { - converted.push_str("}}"); + converted.push('}'); } } } @@ -260,9 +220,9 @@ fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { // Construct the format string. let mut contents = String::with_capacity(1 + converted.len()); contents.push('f'); - contents.push_str(&leading_quote); + contents.push_str(leading_quote); contents.push_str(&converted); - contents.push_str(&trailing_quote); + contents.push_str(trailing_quote); Some(contents) } diff --git a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap index 83c71c78707800..485b66b0492277 100644 --- a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap +++ b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap @@ -19,6 +19,23 @@ expression: diagnostics row: 2 column: 20 parent: ~ +- kind: + FString: ~ + location: + row: 4 + column: 0 + end_location: + row: 4 + column: 22 + fix: + content: "f\"{b} {a}\"" + location: + row: 4 + column: 0 + end_location: + row: 4 + column: 22 + parent: ~ - kind: FString: ~ location: @@ -181,7 +198,7 @@ expression: diagnostics row: 24 column: 20 fix: - content: "f'{a} {b}'" + content: "f\"{a} {b}\"" location: row: 24 column: 0 @@ -198,7 +215,7 @@ expression: diagnostics row: 26 column: 24 fix: - content: "f'''{a} {b}'''" + content: "f\"\"\"{a} {b}\"\"\"" location: row: 26 column: 0 @@ -213,7 +230,7 @@ expression: diagnostics column: 0 end_location: row: 28 - column: 18 + column: 17 fix: content: "f\"foo{1}\"" location: @@ -221,7 +238,7 @@ expression: diagnostics column: 0 end_location: row: 28 - column: 18 + column: 17 parent: ~ - kind: FString: ~ @@ -274,4 +291,72 @@ expression: diagnostics row: 34 column: 25 parent: ~ +- kind: + FString: ~ + location: + row: 36 + column: 0 + end_location: + row: 36 + column: 20 + fix: + content: "f\"{a[b]}\"" + location: + row: 36 + column: 0 + end_location: + row: 36 + column: 20 + parent: ~ +- kind: + FString: ~ + location: + row: 38 + column: 0 + end_location: + row: 38 + column: 22 + fix: + content: "f\"{a.a[b]}\"" + location: + row: 38 + column: 0 + end_location: + row: 38 + column: 22 + parent: ~ +- kind: + FString: ~ + location: + row: 41 + column: 0 + end_location: + row: 41 + column: 29 + fix: + content: "f\"{escaped}{{}}{y}\"" + location: + row: 41 + column: 0 + end_location: + row: 41 + column: 29 + parent: ~ +- kind: + FString: ~ + location: + row: 51 + column: 0 + end_location: + row: 51 + column: 14 + fix: + content: "f\"{a}\"" + location: + row: 51 + column: 0 + end_location: + row: 51 + column: 14 + parent: ~ diff --git a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap.new b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap.new deleted file mode 100644 index f9d70b38cbd31e..00000000000000 --- a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap.new +++ /dev/null @@ -1,363 +0,0 @@ ---- -source: src/rules/pyupgrade/mod.rs -assertion_line: 65 -expression: diagnostics ---- -- kind: - FString: ~ - location: - row: 2 - column: 0 - end_location: - row: 2 - column: 20 - fix: - content: "f\"{a} {b}\"" - location: - row: 2 - column: 0 - end_location: - row: 2 - column: 20 - parent: ~ -- kind: - FString: ~ - location: - row: 4 - column: 0 - end_location: - row: 4 - column: 22 - fix: - content: "f\"{b} {a}\"" - location: - row: 4 - column: 0 - end_location: - row: 4 - column: 22 - parent: ~ -- kind: - FString: ~ - location: - row: 6 - column: 0 - end_location: - row: 6 - column: 19 - fix: - content: "f\"{z.y}\"" - location: - row: 6 - column: 0 - end_location: - row: 6 - column: 19 - parent: ~ -- kind: - FString: ~ - location: - row: 8 - column: 0 - end_location: - row: 8 - column: 24 - fix: - content: "f\"{a.x} {b.y}\"" - location: - row: 8 - column: 0 - end_location: - row: 8 - column: 24 - parent: ~ -- kind: - FString: ~ - location: - row: 10 - column: 0 - end_location: - row: 10 - column: 24 - fix: - content: "f\"{a.b} {c.d}\"" - location: - row: 10 - column: 0 - end_location: - row: 10 - column: 24 - parent: ~ -- kind: - FString: ~ - location: - row: 12 - column: 0 - end_location: - row: 12 - column: 16 - fix: - content: "f\"{a()}\"" - location: - row: 12 - column: 0 - end_location: - row: 12 - column: 16 - parent: ~ -- kind: - FString: ~ - location: - row: 14 - column: 0 - end_location: - row: 14 - column: 18 - fix: - content: "f\"{a.b()}\"" - location: - row: 14 - column: 0 - end_location: - row: 14 - column: 18 - parent: ~ -- kind: - FString: ~ - location: - row: 16 - column: 0 - end_location: - row: 16 - column: 22 - fix: - content: "f\"{a.b().c()}\"" - location: - row: 16 - column: 0 - end_location: - row: 16 - column: 22 - parent: ~ -- kind: - FString: ~ - location: - row: 18 - column: 0 - end_location: - row: 18 - column: 24 - fix: - content: "f\"hello {name}!\"" - location: - row: 18 - column: 0 - end_location: - row: 18 - column: 24 - parent: ~ -- kind: - FString: ~ - location: - row: 20 - column: 0 - end_location: - row: 20 - column: 27 - fix: - content: "f\"{a}{b}{c}\"" - location: - row: 20 - column: 0 - end_location: - row: 20 - column: 27 - parent: ~ -- kind: - FString: ~ - location: - row: 22 - column: 0 - end_location: - row: 22 - column: 16 - fix: - content: "f\"{0x0}\"" - location: - row: 22 - column: 0 - end_location: - row: 22 - column: 16 - parent: ~ -- kind: - FString: ~ - location: - row: 24 - column: 0 - end_location: - row: 24 - column: 20 - fix: - content: "f\"{a} {b}\"" - location: - row: 24 - column: 0 - end_location: - row: 24 - column: 20 - parent: ~ -- kind: - FString: ~ - location: - row: 26 - column: 0 - end_location: - row: 26 - column: 24 - fix: - content: "f\"\"\"{a} {b}\"\"\"" - location: - row: 26 - column: 0 - end_location: - row: 26 - column: 24 - parent: ~ -- kind: - FString: ~ - location: - row: 28 - column: 0 - end_location: - row: 28 - column: 17 - fix: - content: "f\"foo{1}\"" - location: - row: 28 - column: 0 - end_location: - row: 28 - column: 17 - parent: ~ -- kind: - FString: ~ - location: - row: 30 - column: 0 - end_location: - row: 30 - column: 18 - fix: - content: "fr\"foo{1}\"" - location: - row: 30 - column: 0 - end_location: - row: 30 - column: 18 - parent: ~ -- kind: - FString: ~ - location: - row: 32 - column: 4 - end_location: - row: 32 - column: 21 - fix: - content: "f\"{1}\"" - location: - row: 32 - column: 4 - end_location: - row: 32 - column: 21 - parent: ~ -- kind: - FString: ~ - location: - row: 34 - column: 6 - end_location: - row: 34 - column: 25 - fix: - content: "f\"foo {x} \"" - location: - row: 34 - column: 6 - end_location: - row: 34 - column: 25 - parent: ~ -- kind: - FString: ~ - location: - row: 36 - column: 0 - end_location: - row: 36 - column: 20 - fix: - content: "f\"{a[b]}\"" - location: - row: 36 - column: 0 - end_location: - row: 36 - column: 20 - parent: ~ -- kind: - FString: ~ - location: - row: 38 - column: 0 - end_location: - row: 38 - column: 22 - fix: - content: "f\"{a.a[b]}\"" - location: - row: 38 - column: 0 - end_location: - row: 38 - column: 22 - parent: ~ -- kind: - FString: ~ - location: - row: 41 - column: 0 - end_location: - row: 41 - column: 29 - fix: - content: "f\"{escaped}{{{}}}{y}\"" - location: - row: 41 - column: 0 - end_location: - row: 41 - column: 29 - parent: ~ -- kind: - FString: ~ - location: - row: 51 - column: 0 - end_location: - row: 51 - column: 14 - fix: - content: "f\"{a}\"" - location: - row: 51 - column: 0 - end_location: - row: 51 - column: 14 - parent: ~ - From 2692121905581fabd3fea17e9d90fb41ddab73cb Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 16 Jan 2023 22:51:27 -0500 Subject: [PATCH 25/26] Avoid implicit string concats --- resources/test/fixtures/pyupgrade/UP032.py | 12 +- src/rules/pyupgrade/rules/f_strings.rs | 13 +- ...les__pyupgrade__tests__UP032_UP032.py.snap | 168 +++++++++--------- 3 files changed, 105 insertions(+), 88 deletions(-) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index b195b085a5c3d3..fb96dfc7562add 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -1,4 +1,7 @@ -# These SHOULD change +### +# Errors +### + "{} {}".format(a, b) "{1} {0}".format(a, b) @@ -37,12 +40,13 @@ "{a.a[b]}".format(a=a) - "{}{{}}{}".format(escaped, y) "\N{snowman} {}".format(a) -# These should NOT change +### +# Non-errors +### "{".format(a) @@ -70,6 +74,8 @@ r'"\N{snowman} {}".format(a)' +"{a}" "{b}".format(a=1, b=1) + async def c(): return "{}".format(await 3) diff --git a/src/rules/pyupgrade/rules/f_strings.rs b/src/rules/pyupgrade/rules/f_strings.rs index 642ebd4493c23a..76c5c33956d7f1 100644 --- a/src/rules/pyupgrade/rules/f_strings.rs +++ b/src/rules/pyupgrade/rules/f_strings.rs @@ -3,6 +3,8 @@ use rustpython_ast::{Constant, Expr, ExprKind, KeywordData}; use rustpython_common::format::{ FieldName, FieldNamePart, FieldType, FormatPart, FormatString, FromTemplate, }; +use rustpython_parser::lexer; +use rustpython_parser::lexer::Tok; use crate::ast::types::Range; use crate::checkers::ast::Checker; @@ -116,6 +118,16 @@ fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { .locator .slice_source_code_range(&Range::from_located(value)); + // Tokenize: we need to avoid trying to fix implicit string concatenations. + if lexer::make_tokenizer(&contents) + .flatten() + .filter(|(_, tok, _)| matches!(tok, Tok::String { .. })) + .count() + > 1 + { + return None; + } + // Strip the unicode prefix. It's redundant in Python 3, and invalid when used // with f-strings. let contents = if contents.starts_with('U') || contents.starts_with('u') { @@ -138,7 +150,6 @@ fn try_convert_to_f_string(checker: &Checker, expr: &Expr) -> Option { // Parse the format string. let Ok(format_string) = FormatString::from_str(contents) else { - return None; }; diff --git a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap index 485b66b0492277..3e2639f2d7781a 100644 --- a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap +++ b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap @@ -5,358 +5,358 @@ expression: diagnostics - kind: FString: ~ location: - row: 2 + row: 5 column: 0 end_location: - row: 2 + row: 5 column: 20 fix: content: "f\"{a} {b}\"" location: - row: 2 + row: 5 column: 0 end_location: - row: 2 + row: 5 column: 20 parent: ~ - kind: FString: ~ location: - row: 4 + row: 7 column: 0 end_location: - row: 4 + row: 7 column: 22 fix: content: "f\"{b} {a}\"" location: - row: 4 + row: 7 column: 0 end_location: - row: 4 + row: 7 column: 22 parent: ~ - kind: FString: ~ location: - row: 6 + row: 9 column: 0 end_location: - row: 6 + row: 9 column: 19 fix: content: "f\"{z.y}\"" location: - row: 6 + row: 9 column: 0 end_location: - row: 6 + row: 9 column: 19 parent: ~ - kind: FString: ~ location: - row: 8 + row: 11 column: 0 end_location: - row: 8 + row: 11 column: 24 fix: content: "f\"{a.x} {b.y}\"" location: - row: 8 + row: 11 column: 0 end_location: - row: 8 + row: 11 column: 24 parent: ~ - kind: FString: ~ location: - row: 10 + row: 13 column: 0 end_location: - row: 10 + row: 13 column: 24 fix: content: "f\"{a.b} {c.d}\"" location: - row: 10 + row: 13 column: 0 end_location: - row: 10 + row: 13 column: 24 parent: ~ - kind: FString: ~ location: - row: 12 + row: 15 column: 0 end_location: - row: 12 + row: 15 column: 16 fix: content: "f\"{a()}\"" location: - row: 12 + row: 15 column: 0 end_location: - row: 12 + row: 15 column: 16 parent: ~ - kind: FString: ~ location: - row: 14 + row: 17 column: 0 end_location: - row: 14 + row: 17 column: 18 fix: content: "f\"{a.b()}\"" location: - row: 14 + row: 17 column: 0 end_location: - row: 14 + row: 17 column: 18 parent: ~ - kind: FString: ~ location: - row: 16 + row: 19 column: 0 end_location: - row: 16 + row: 19 column: 22 fix: content: "f\"{a.b().c()}\"" location: - row: 16 + row: 19 column: 0 end_location: - row: 16 + row: 19 column: 22 parent: ~ - kind: FString: ~ location: - row: 18 + row: 21 column: 0 end_location: - row: 18 + row: 21 column: 24 fix: content: "f\"hello {name}!\"" location: - row: 18 + row: 21 column: 0 end_location: - row: 18 + row: 21 column: 24 parent: ~ - kind: FString: ~ location: - row: 20 + row: 23 column: 0 end_location: - row: 20 + row: 23 column: 27 fix: content: "f\"{a}{b}{c}\"" location: - row: 20 + row: 23 column: 0 end_location: - row: 20 + row: 23 column: 27 parent: ~ - kind: FString: ~ location: - row: 22 + row: 25 column: 0 end_location: - row: 22 + row: 25 column: 16 fix: content: "f\"{0x0}\"" location: - row: 22 + row: 25 column: 0 end_location: - row: 22 + row: 25 column: 16 parent: ~ - kind: FString: ~ location: - row: 24 + row: 27 column: 0 end_location: - row: 24 + row: 27 column: 20 fix: content: "f\"{a} {b}\"" location: - row: 24 + row: 27 column: 0 end_location: - row: 24 + row: 27 column: 20 parent: ~ - kind: FString: ~ location: - row: 26 + row: 29 column: 0 end_location: - row: 26 + row: 29 column: 24 fix: content: "f\"\"\"{a} {b}\"\"\"" location: - row: 26 + row: 29 column: 0 end_location: - row: 26 + row: 29 column: 24 parent: ~ - kind: FString: ~ location: - row: 28 + row: 31 column: 0 end_location: - row: 28 + row: 31 column: 17 fix: content: "f\"foo{1}\"" location: - row: 28 + row: 31 column: 0 end_location: - row: 28 + row: 31 column: 17 parent: ~ - kind: FString: ~ location: - row: 30 + row: 33 column: 0 end_location: - row: 30 + row: 33 column: 18 fix: content: "fr\"foo{1}\"" location: - row: 30 + row: 33 column: 0 end_location: - row: 30 + row: 33 column: 18 parent: ~ - kind: FString: ~ location: - row: 32 + row: 35 column: 4 end_location: - row: 32 + row: 35 column: 21 fix: content: "f\"{1}\"" location: - row: 32 + row: 35 column: 4 end_location: - row: 32 + row: 35 column: 21 parent: ~ - kind: FString: ~ location: - row: 34 + row: 37 column: 6 end_location: - row: 34 + row: 37 column: 25 fix: content: "f\"foo {x} \"" location: - row: 34 + row: 37 column: 6 end_location: - row: 34 + row: 37 column: 25 parent: ~ - kind: FString: ~ location: - row: 36 + row: 39 column: 0 end_location: - row: 36 + row: 39 column: 20 fix: content: "f\"{a[b]}\"" location: - row: 36 + row: 39 column: 0 end_location: - row: 36 + row: 39 column: 20 parent: ~ - kind: FString: ~ location: - row: 38 + row: 41 column: 0 end_location: - row: 38 + row: 41 column: 22 fix: content: "f\"{a.a[b]}\"" location: - row: 38 + row: 41 column: 0 end_location: - row: 38 + row: 41 column: 22 parent: ~ - kind: FString: ~ location: - row: 41 + row: 43 column: 0 end_location: - row: 41 + row: 43 column: 29 fix: content: "f\"{escaped}{{}}{y}\"" location: - row: 41 + row: 43 column: 0 end_location: - row: 41 + row: 43 column: 29 parent: ~ - kind: FString: ~ location: - row: 51 + row: 55 column: 0 end_location: - row: 51 + row: 55 column: 14 fix: content: "f\"{a}\"" location: - row: 51 + row: 55 column: 0 end_location: - row: 51 + row: 55 column: 14 parent: ~ From 87c31e622efecbfb674823749b56c7e6a55ead5c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 16 Jan 2023 22:58:51 -0500 Subject: [PATCH 26/26] Change format --- README.md | 2 +- resources/test/fixtures/pyupgrade/UP032.py | 7 ++++--- .../ruff__rules__pyupgrade__tests__UP032_UP032.py.snap | 8 ++++---- src/violations.rs | 6 +++--- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 4ac54efbf40d34..73ee73104aa9a5 100644 --- a/README.md +++ b/README.md @@ -725,7 +725,7 @@ For more, see [pyupgrade](https://pypi.org/project/pyupgrade/3.2.0/) on PyPI. | UP028 | RewriteYieldFrom | Replace `yield` over `for` loop with `yield from` | 🛠 | | UP029 | UnnecessaryBuiltinImport | Unnecessary builtin import: `...` | 🛠 | | UP030 | FormatLiterals | Use implicit references for positional format fields | 🛠 | -| UP032 | FString | Use f-strings instead of positional format fields | 🛠 | +| UP032 | FString | Use f-string instead of `format` call | 🛠 | ### pep8-naming (N) diff --git a/resources/test/fixtures/pyupgrade/UP032.py b/resources/test/fixtures/pyupgrade/UP032.py index fb96dfc7562add..3964f6daa6cc27 100644 --- a/resources/test/fixtures/pyupgrade/UP032.py +++ b/resources/test/fixtures/pyupgrade/UP032.py @@ -42,18 +42,19 @@ "{}{{}}{}".format(escaped, y) -"\N{snowman} {}".format(a) +"{}".format(a) ### # Non-errors ### +# False-negative: RustPython doesn't parse the `\N{snowman}`. +"\N{snowman} {}".format(a) + "{".format(a) "}".format(a) -"{}".format(a) - "{} {}".format(*a) "{0} {0}".format(arg) diff --git a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap index 3e2639f2d7781a..e3e29cb71c4c40 100644 --- a/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap +++ b/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP032_UP032.py.snap @@ -345,18 +345,18 @@ expression: diagnostics - kind: FString: ~ location: - row: 55 + row: 45 column: 0 end_location: - row: 55 + row: 45 column: 14 fix: content: "f\"{a}\"" location: - row: 55 + row: 45 column: 0 end_location: - row: 55 + row: 45 column: 14 parent: ~ diff --git a/src/violations.rs b/src/violations.rs index 4b08cc543ac701..a329df0bcbe004 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -3802,15 +3802,15 @@ define_violation!( ); impl AlwaysAutofixableViolation for FString { fn message(&self) -> String { - "Use f-strings instead of positional format fields".to_string() + "Use f-string instead of `format` call".to_string() } fn autofix_title(&self) -> String { - "Convert strings with positional format fields to strings".to_string() + "Convert to f-string".to_string() } fn placeholder() -> Self { - Self + FString } }