From 841714b233888c7031f86ba75351bb12df260b65 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Sat, 3 Jun 2023 08:58:59 -0500 Subject: [PATCH 1/3] Make FLY002 emit a string (instead of an f-string) if all join() arguments are strings --- .../flynt/rules/static_join_to_fstring.rs | 32 +++++++++++++++---- ...rules__flynt__tests__FLY002_FLY002.py.snap | 6 ++-- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs index cd05bace7cfe6..db7cc6ae22c69 100644 --- a/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs +++ b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs @@ -1,5 +1,6 @@ +use itertools::Itertools; use ruff_text_size::TextRange; -use rustpython_parser::ast::{self, Expr, Ranged}; +use rustpython_parser::ast::{self, Constant, Expr, Ranged}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; @@ -33,6 +34,7 @@ fn is_static_length(elts: &[Expr]) -> bool { fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option { let mut fstring_elems = Vec::with_capacity(joinees.len() * 2); let mut first = true; + let mut strings: Vec<&String> = vec![]; for expr in joinees { if matches!(expr, Expr::JoinedStr(_)) { @@ -44,13 +46,31 @@ fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option { fstring_elems.push(helpers::to_constant_string(joiner)); } fstring_elems.push(helpers::to_fstring_elem(expr)?); + + if let Expr::Constant(ast::ExprConstant { + value: Constant::Str(value), + .. + }) = expr + { + strings.push(value); + } } - let node = ast::ExprJoinedStr { - values: fstring_elems, - range: TextRange::default(), + let node = if strings.len() == joinees.len() { + ast::Expr::Constant(ast::ExprConstant { + value: Constant::Str(strings.iter().join(joiner)), + range: TextRange::default(), + kind: None, + }) + } else { + ast::ExprJoinedStr { + values: fstring_elems, + range: TextRange::default(), + } + .into() }; - Some(node.into()) + + Some(node) } pub(crate) fn static_join_to_fstring(checker: &mut Checker, expr: &Expr, joiner: &str) { @@ -58,7 +78,7 @@ pub(crate) fn static_join_to_fstring(checker: &mut Checker, expr: &Expr, joiner: args, keywords, .. - })= expr else { + }) = expr else { return; }; diff --git a/crates/ruff/src/rules/flynt/snapshots/ruff__rules__flynt__tests__FLY002_FLY002.py.snap b/crates/ruff/src/rules/flynt/snapshots/ruff__rules__flynt__tests__FLY002_FLY002.py.snap index d268b251ba6a9..e987f86206f84 100644 --- a/crates/ruff/src/rules/flynt/snapshots/ruff__rules__flynt__tests__FLY002_FLY002.py.snap +++ b/crates/ruff/src/rules/flynt/snapshots/ruff__rules__flynt__tests__FLY002_FLY002.py.snap @@ -42,7 +42,7 @@ FLY002.py:6:7: FLY002 [*] Consider `f"Finally, {a} World"` instead of string joi 8 8 | ok4 = "y".join([1, 2, 3]) # Technically OK, though would've been an error originally 9 9 | ok5 = "a".join([random(), random()]) # OK (simple calls) -FLY002.py:7:7: FLY002 [*] Consider `f"1x2x3"` instead of string join +FLY002.py:7:7: FLY002 [*] Consider `"1x2x3"` instead of string join | 7 | ok1 = " ".join([a, " World"]) # OK 8 | ok2 = "".join(["Finally, ", a, " World"]) # OK @@ -51,14 +51,14 @@ FLY002.py:7:7: FLY002 [*] Consider `f"1x2x3"` instead of string join 10 | ok4 = "y".join([1, 2, 3]) # Technically OK, though would've been an error originally 11 | ok5 = "a".join([random(), random()]) # OK (simple calls) | - = help: Replace with `f"1x2x3"` + = help: Replace with `"1x2x3"` ℹ Suggested fix 4 4 | a = "Hello" 5 5 | ok1 = " ".join([a, " World"]) # OK 6 6 | ok2 = "".join(["Finally, ", a, " World"]) # OK 7 |-ok3 = "x".join(("1", "2", "3")) # OK - 7 |+ok3 = f"1x2x3" # OK + 7 |+ok3 = "1x2x3" # OK 8 8 | ok4 = "y".join([1, 2, 3]) # Technically OK, though would've been an error originally 9 9 | ok5 = "a".join([random(), random()]) # OK (simple calls) 10 10 | ok6 = "a".join([secrets.token_urlsafe(), secrets.token_hex()]) # OK (attr calls) From 3c2192183ac4e3dcab37082c20d3b20a92fbcd54 Mon Sep 17 00:00:00 2001 From: Evan Rittenhouse Date: Sat, 3 Jun 2023 10:30:37 -0500 Subject: [PATCH 2/3] More descriptive variable name --- .../ruff/src/rules/flynt/rules/static_join_to_fstring.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs index db7cc6ae22c69..292ec48bcf40c 100644 --- a/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs +++ b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs @@ -34,7 +34,7 @@ fn is_static_length(elts: &[Expr]) -> bool { fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option { let mut fstring_elems = Vec::with_capacity(joinees.len() * 2); let mut first = true; - let mut strings: Vec<&String> = vec![]; + let mut string_args: Vec<&String> = vec![]; for expr in joinees { if matches!(expr, Expr::JoinedStr(_)) { @@ -52,13 +52,13 @@ fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option { .. }) = expr { - strings.push(value); + string_args.push(value); } } - let node = if strings.len() == joinees.len() { + let node = if string_args.len() == joinees.len() { ast::Expr::Constant(ast::ExprConstant { - value: Constant::Str(strings.iter().join(joiner)), + value: Constant::Str(string_args.iter().join(joiner)), range: TextRange::default(), kind: None, }) From d61a5e69ddd46e92f7c2649807d3c577aff062bd Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 3 Jun 2023 16:24:52 -0400 Subject: [PATCH 3/3] Check upfront --- .../flynt/rules/static_join_to_fstring.rs | 64 +++++++++++-------- 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs index 292ec48bcf40c..2faab59911a9b 100644 --- a/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs +++ b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs @@ -28,16 +28,48 @@ impl AlwaysAutofixableViolation for StaticJoinToFString { } fn is_static_length(elts: &[Expr]) -> bool { - elts.iter().all(|e| !matches!(e, Expr::Starred(_))) + elts.iter().all(|e| !e.is_starred_expr()) } fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option { + // If all elements are string constants, join them into a single string. + if joinees.iter().all(|expr| { + matches!( + expr, + Expr::Constant(ast::ExprConstant { + value: Constant::Str(_), + .. + }) + ) + }) { + let node = ast::ExprConstant { + value: Constant::Str( + joinees + .iter() + .filter_map(|expr| { + if let Expr::Constant(ast::ExprConstant { + value: Constant::Str(string), + .. + }) = expr + { + Some(string.as_str()) + } else { + None + } + }) + .join(joiner), + ), + range: TextRange::default(), + kind: None, + }; + return Some(node.into()); + } + let mut fstring_elems = Vec::with_capacity(joinees.len() * 2); let mut first = true; - let mut string_args: Vec<&String> = vec![]; for expr in joinees { - if matches!(expr, Expr::JoinedStr(_)) { + if expr.is_joined_str_expr() { // Oops, already an f-string. We don't know how to handle those // gracefully right now. return None; @@ -46,31 +78,13 @@ fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option { fstring_elems.push(helpers::to_constant_string(joiner)); } fstring_elems.push(helpers::to_fstring_elem(expr)?); - - if let Expr::Constant(ast::ExprConstant { - value: Constant::Str(value), - .. - }) = expr - { - string_args.push(value); - } } - let node = if string_args.len() == joinees.len() { - ast::Expr::Constant(ast::ExprConstant { - value: Constant::Str(string_args.iter().join(joiner)), - range: TextRange::default(), - kind: None, - }) - } else { - ast::ExprJoinedStr { - values: fstring_elems, - range: TextRange::default(), - } - .into() + let node = ast::ExprJoinedStr { + values: fstring_elems, + range: TextRange::default(), }; - - Some(node) + Some(node.into()) } pub(crate) fn static_join_to_fstring(checker: &mut Checker, expr: &Expr, joiner: &str) {