From 02fcf83dea03a05d5dfca843c2bf779abd5c39b1 Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Thu, 4 May 2023 10:42:25 +0300 Subject: [PATCH 1/3] Implement FLY002 (convert static string joins to f-strings) --- .../resources/test/fixtures/flynt/FLY002.py | 18 +++ crates/ruff/src/checkers/ast/mod.rs | 14 +- crates/ruff/src/codes.rs | 4 + crates/ruff/src/registry.rs | 5 + crates/ruff/src/rules/flynt/helpers.rs | 64 +++++++++ crates/ruff/src/rules/flynt/mod.rs | 24 ++++ crates/ruff/src/rules/flynt/rules/mod.rs | 3 + .../flynt/rules/static_join_to_fstring.rs | 98 ++++++++++++++ ...rules__flynt__tests__FLY002_FLY002.py.snap | 128 ++++++++++++++++++ crates/ruff/src/rules/mod.rs | 1 + ruff.schema.json | 4 + 11 files changed, 360 insertions(+), 3 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/flynt/FLY002.py create mode 100644 crates/ruff/src/rules/flynt/helpers.rs create mode 100644 crates/ruff/src/rules/flynt/mod.rs create mode 100644 crates/ruff/src/rules/flynt/rules/mod.rs create mode 100644 crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs create mode 100644 crates/ruff/src/rules/flynt/snapshots/ruff__rules__flynt__tests__FLY002_FLY002.py.snap diff --git a/crates/ruff/resources/test/fixtures/flynt/FLY002.py b/crates/ruff/resources/test/fixtures/flynt/FLY002.py new file mode 100644 index 0000000000000..2067b12d5da92 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flynt/FLY002.py @@ -0,0 +1,18 @@ +import secrets +from random import random, choice + +a = "Hello" +ok1 = " ".join([a, " World"]) # OK +ok2 = "".join(["Finally, ", a, " World"]) # OK +ok3 = "x".join(("1", "2", "3")) # OK +ok4 = "y".join([1, 2, 3]) # Technically OK, though would've been an error originally +ok5 = "a".join([random(), random()]) # OK (simple calls) +ok6 = "a".join([secrets.token_urlsafe(), secrets.token_hex()]) # OK (attr calls) + +nok1 = "x".join({"4", "5", "yee"}) # Not OK (set) +nok2 = a.join(["1", "2", "3"]) # Not OK (not a static joiner) +nok3 = "a".join(a) # Not OK (not a static joinee) +nok4 = "a".join([a, a, *a]) # Not OK (not a static length) +nok5 = "a".join([choice("flarp")]) # Not OK (not a simple call) +nok6 = "a".join(x for x in "feefoofum") # Not OK (generator) +nok7 = "a".join([f"foo{8}", "bar"]) # Not OK (contains an f-string) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 14febab297f99..ee8faa5d0a5d3 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -43,9 +43,9 @@ use crate::rules::{ flake8_django, flake8_errmsg, flake8_gettext, flake8_implicit_str_concat, flake8_import_conventions, flake8_logging_format, flake8_pie, flake8_print, flake8_pyi, flake8_pytest_style, flake8_raise, flake8_return, flake8_self, flake8_simplify, - flake8_tidy_imports, flake8_type_checking, flake8_unused_arguments, flake8_use_pathlib, mccabe, - numpy, pandas_vet, pep8_naming, pycodestyle, pydocstyle, pyflakes, pygrep_hooks, pylint, - pyupgrade, ruff, tryceratops, + flake8_tidy_imports, flake8_type_checking, flake8_unused_arguments, flake8_use_pathlib, flynt, + mccabe, numpy, pandas_vet, pep8_naming, pycodestyle, pydocstyle, pyflakes, pygrep_hooks, + pylint, pyupgrade, ruff, tryceratops, }; use crate::settings::types::PythonVersion; use crate::settings::{flags, Settings}; @@ -2462,6 +2462,8 @@ where // pyupgrade Rule::FormatLiterals, Rule::FString, + // flynt + Rule::StaticJoinToFString, ]) { if let ExprKind::Attribute { value, attr, .. } = &func.node { if let ExprKind::Constant { @@ -2469,6 +2471,12 @@ where .. } = &value.node { + if attr == "join" { + // "...".join(...) call + if self.settings.rules.enabled(Rule::StaticJoinToFString) { + flynt::rules::static_join_to_fstring(self, expr, value); + } + } if attr == "format" { // "...".format(...) call let location = expr.range(); diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index cffdf12abe94e..4b32d04ccf62d 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -735,6 +735,10 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Flake8Django, "012") => Rule::DjangoUnorderedBodyContentInModel, (Flake8Django, "013") => Rule::DjangoNonLeadingReceiverDecorator, + // flynt + // Reserved: (Flynt, "001") => Rule::StringConcatenationToFString, + (Flynt, "002") => Rule::StaticJoinToFString, + _ => return None, }) } diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index cc342b9d0eab7..e797cc5a0c3b8 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -669,6 +669,8 @@ ruff_macros::register_rules!( rules::flake8_django::rules::DjangoModelWithoutDunderStr, rules::flake8_django::rules::DjangoUnorderedBodyContentInModel, rules::flake8_django::rules::DjangoNonLeadingReceiverDecorator, + // flynt + rules::flynt::rules::StaticJoinToFString, ); pub trait AsRule { @@ -824,6 +826,9 @@ pub enum Linter { /// [tryceratops](https://pypi.org/project/tryceratops/1.1.0/) #[prefix = "TRY"] Tryceratops, + /// [flynt](https://pypi.org/project/flynt/) + #[prefix = "FLY"] + Flynt, /// NumPy-specific rules #[prefix = "NPY"] Numpy, diff --git a/crates/ruff/src/rules/flynt/helpers.rs b/crates/ruff/src/rules/flynt/helpers.rs new file mode 100644 index 0000000000000..2979cd3c5c130 --- /dev/null +++ b/crates/ruff/src/rules/flynt/helpers.rs @@ -0,0 +1,64 @@ +use ruff_python_ast::helpers::create_expr; +use rustpython_parser::ast::{Constant, Expr, ExprKind}; + +fn to_formatted_value_expr(inner: Expr) -> Expr { + create_expr(ExprKind::FormattedValue { + value: Box::new(inner), + conversion: 0, + format_spec: None, + }) +} + +/// Figure out if `expr` represents a "simple" call +/// (i.e. one that can be safely converted to a formatted value). +fn is_simple_call(expr: &Expr) -> bool { + match &expr.node { + ExprKind::Call { + func, + args, + keywords, + } => args.is_empty() && keywords.is_empty() && is_simple_callee(func), + _ => false, + } +} + +/// Figure out if `func` represents a "simple" callee (a bare name, or a chain of simple +/// attribute accesses). +fn is_simple_callee(func: &Expr) -> bool { + match &func.node { + ExprKind::Name { .. } => true, + ExprKind::Attribute { value, .. } => is_simple_callee(value), + _ => false, + } +} + +/// Convert an expression to a f-string element (if it looks like a good idea). +pub fn to_fstring_elem(expr: Expr) -> Option { + match &expr.node { + // These are directly handled by `unparse_fstring_elem`: + ExprKind::Constant { + value: Constant::Str(_), + .. + } + | ExprKind::JoinedStr { .. } + | ExprKind::FormattedValue { .. } => Some(expr), + // These should be pretty safe to wrap in a formatted value. + ExprKind::Constant { + value: + Constant::Int(_) | Constant::Float(_) | Constant::Bool(_) | Constant::Complex { .. }, + .. + } + | ExprKind::Name { .. } + | ExprKind::Attribute { .. } => Some(to_formatted_value_expr(expr)), + ExprKind::Call { .. } if is_simple_call(&expr) => Some(to_formatted_value_expr(expr)), + _ => None, + } +} + +/// Convert a string to a constant string expression. +pub fn to_constant_string(s: &str) -> Expr { + create_expr(ExprKind::Constant { + value: Constant::Str(s.to_owned()), + kind: None, + }) +} diff --git a/crates/ruff/src/rules/flynt/mod.rs b/crates/ruff/src/rules/flynt/mod.rs new file mode 100644 index 0000000000000..2d5ca2114a03c --- /dev/null +++ b/crates/ruff/src/rules/flynt/mod.rs @@ -0,0 +1,24 @@ +//! Rules from [flynt](https://pypi.org/project/flynt/). +mod helpers; +pub(crate) mod rules; + +#[cfg(test)] +mod tests { + use crate::registry::Rule; + use crate::test::test_path; + use crate::{assert_messages, settings}; + use anyhow::Result; + use std::path::Path; + use test_case::test_case; + + #[test_case(Rule::StaticJoinToFString, Path::new("FLY002.py"); "FLY002")] + fn rules(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); + let diagnostics = test_path( + Path::new("flynt").join(path).as_path(), + &settings::Settings::for_rule(rule_code), + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } +} diff --git a/crates/ruff/src/rules/flynt/rules/mod.rs b/crates/ruff/src/rules/flynt/rules/mod.rs new file mode 100644 index 0000000000000..36bc617982081 --- /dev/null +++ b/crates/ruff/src/rules/flynt/rules/mod.rs @@ -0,0 +1,3 @@ +mod static_join_to_fstring; + +pub use static_join_to_fstring::{static_join_to_fstring, StaticJoinToFString}; 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 new file mode 100644 index 0000000000000..9c872718eb27d --- /dev/null +++ b/crates/ruff/src/rules/flynt/rules/static_join_to_fstring.rs @@ -0,0 +1,98 @@ +use rustpython_parser::ast::{Expr, ExprKind}; + +use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::{create_expr, unparse_expr}; + +use crate::checkers::ast::Checker; +use crate::registry::AsRule; +use crate::rules::flynt::helpers; + +#[violation] +pub struct StaticJoinToFString { + pub expr: String, + pub fixable: bool, +} + +impl Violation for StaticJoinToFString { + const AUTOFIX: AutofixKind = AutofixKind::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + let StaticJoinToFString { expr, .. } = self; + format!("Consider `{expr}` instead of string join") + } + + fn autofix_title_formatter(&self) -> Option String> { + self.fixable + .then_some(|StaticJoinToFString { expr, .. }| format!("Replace with `{expr}`")) + } +} + +fn is_static_length(elts: &[Expr]) -> bool { + elts.iter() + .all(|e| !matches!(e.node, ExprKind::Starred { .. })) +} + +fn build_fstring(joiner: &str, joinees: &Vec) -> Option { + let mut fstring_elems = Vec::with_capacity(joinees.len() * 2); + for (i, expr) in joinees.iter().enumerate() { + if matches!(expr.node, ExprKind::JoinedStr { .. }) { + // Oops, already an f-string. We don't know how to handle those + // gracefully right now. + return None; + } + let elem = helpers::to_fstring_elem(expr.clone())?; + if i != 0 { + fstring_elems.push(helpers::to_constant_string(joiner)); + } + fstring_elems.push(elem); + } + Some(create_expr(ExprKind::JoinedStr { + values: fstring_elems, + })) +} + +pub fn static_join_to_fstring(checker: &mut Checker, expr: &Expr, joiner: &str) { + let ExprKind::Call { + func: _, + args, + keywords, + } = &expr.node else { + return; + }; + + if !keywords.is_empty() || args.len() != 1 { + // If there are kwargs or more than one argument, + // this is some non-standard string join call. + return; + } + + // Get the elements to join; skip e.g. generators, sets, etc. + let joinees = match &args[0].node { + ExprKind::List { elts, .. } if is_static_length(elts) => elts, + ExprKind::Tuple { elts, .. } if is_static_length(elts) => elts, + _ => return, + }; + + // Try to build the fstring (internally checks whether e.g. the elements are + // convertible to f-string parts). + let Some(new_expr) = build_fstring(joiner, joinees) else { return }; + + let contents = unparse_expr(&new_expr, checker.stylist); + let fixable = true; // I'm not sure there is a case where this is not fixable..? + + let mut diagnostic = Diagnostic::new( + StaticJoinToFString { + expr: contents.clone(), + fixable, + }, + expr.range(), + ); + if checker.patch(diagnostic.kind.rule()) { + if fixable { + diagnostic.set_fix(Edit::range_replacement(contents, expr.range())); + } + } + checker.diagnostics.push(diagnostic); +} 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 new file mode 100644 index 0000000000000..d268b251ba6a9 --- /dev/null +++ b/crates/ruff/src/rules/flynt/snapshots/ruff__rules__flynt__tests__FLY002_FLY002.py.snap @@ -0,0 +1,128 @@ +--- +source: crates/ruff/src/rules/flynt/mod.rs +--- +FLY002.py:5:7: FLY002 [*] Consider `f"{a} World"` instead of string join + | +5 | a = "Hello" +6 | ok1 = " ".join([a, " World"]) # OK + | ^^^^^^^^^^^^^^^^^^^^^^^ FLY002 +7 | ok2 = "".join(["Finally, ", a, " World"]) # OK +8 | ok3 = "x".join(("1", "2", "3")) # OK + | + = help: Replace with `f"{a} World"` + +ℹ Suggested fix +2 2 | from random import random, choice +3 3 | +4 4 | a = "Hello" +5 |-ok1 = " ".join([a, " World"]) # OK + 5 |+ok1 = f"{a} World" # OK +6 6 | ok2 = "".join(["Finally, ", a, " World"]) # OK +7 7 | ok3 = "x".join(("1", "2", "3")) # OK +8 8 | ok4 = "y".join([1, 2, 3]) # Technically OK, though would've been an error originally + +FLY002.py:6:7: FLY002 [*] Consider `f"Finally, {a} World"` instead of string join + | + 6 | a = "Hello" + 7 | ok1 = " ".join([a, " World"]) # OK + 8 | ok2 = "".join(["Finally, ", a, " World"]) # OK + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FLY002 + 9 | ok3 = "x".join(("1", "2", "3")) # OK +10 | ok4 = "y".join([1, 2, 3]) # Technically OK, though would've been an error originally + | + = help: Replace with `f"Finally, {a} World"` + +ℹ Suggested fix +3 3 | +4 4 | a = "Hello" +5 5 | ok1 = " ".join([a, " World"]) # OK +6 |-ok2 = "".join(["Finally, ", a, " World"]) # OK + 6 |+ok2 = f"Finally, {a} World" # OK +7 7 | ok3 = "x".join(("1", "2", "3")) # 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) + +FLY002.py:7:7: FLY002 [*] Consider `f"1x2x3"` instead of string join + | + 7 | ok1 = " ".join([a, " World"]) # OK + 8 | ok2 = "".join(["Finally, ", a, " World"]) # OK + 9 | ok3 = "x".join(("1", "2", "3")) # OK + | ^^^^^^^^^^^^^^^^^^^^^^^^^ FLY002 +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"` + +ℹ 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 +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) + +FLY002.py:8:7: FLY002 [*] Consider `f"{1}y{2}y{3}"` instead of string join + | + 8 | ok2 = "".join(["Finally, ", a, " World"]) # OK + 9 | ok3 = "x".join(("1", "2", "3")) # OK +10 | ok4 = "y".join([1, 2, 3]) # Technically OK, though would've been an error originally + | ^^^^^^^^^^^^^^^^^^^ FLY002 +11 | ok5 = "a".join([random(), random()]) # OK (simple calls) +12 | ok6 = "a".join([secrets.token_urlsafe(), secrets.token_hex()]) # OK (attr calls) + | + = help: Replace with `f"{1}y{2}y{3}"` + +ℹ Suggested fix +5 5 | ok1 = " ".join([a, " World"]) # OK +6 6 | ok2 = "".join(["Finally, ", a, " World"]) # OK +7 7 | ok3 = "x".join(("1", "2", "3")) # OK +8 |-ok4 = "y".join([1, 2, 3]) # Technically OK, though would've been an error originally + 8 |+ok4 = f"{1}y{2}y{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) +11 11 | + +FLY002.py:9:7: FLY002 [*] Consider `f"{random()}a{random()}"` instead of string join + | + 9 | ok3 = "x".join(("1", "2", "3")) # OK +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) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FLY002 +12 | ok6 = "a".join([secrets.token_urlsafe(), secrets.token_hex()]) # OK (attr calls) + | + = help: Replace with `f"{random()}a{random()}"` + +ℹ Suggested fix +6 6 | ok2 = "".join(["Finally, ", a, " World"]) # OK +7 7 | ok3 = "x".join(("1", "2", "3")) # OK +8 8 | ok4 = "y".join([1, 2, 3]) # Technically OK, though would've been an error originally +9 |-ok5 = "a".join([random(), random()]) # OK (simple calls) + 9 |+ok5 = f"{random()}a{random()}" # OK (simple calls) +10 10 | ok6 = "a".join([secrets.token_urlsafe(), secrets.token_hex()]) # OK (attr calls) +11 11 | +12 12 | nok1 = "x".join({"4", "5", "yee"}) # Not OK (set) + +FLY002.py:10:7: FLY002 [*] Consider `f"{secrets.token_urlsafe()}a{secrets.token_hex()}"` 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) +12 | ok6 = "a".join([secrets.token_urlsafe(), secrets.token_hex()]) # OK (attr calls) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FLY002 +13 | +14 | nok1 = "x".join({"4", "5", "yee"}) # Not OK (set) + | + = help: Replace with `f"{secrets.token_urlsafe()}a{secrets.token_hex()}"` + +ℹ Suggested fix +7 7 | ok3 = "x".join(("1", "2", "3")) # 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 |-ok6 = "a".join([secrets.token_urlsafe(), secrets.token_hex()]) # OK (attr calls) + 10 |+ok6 = f"{secrets.token_urlsafe()}a{secrets.token_hex()}" # OK (attr calls) +11 11 | +12 12 | nok1 = "x".join({"4", "5", "yee"}) # Not OK (set) +13 13 | nok2 = a.join(["1", "2", "3"]) # Not OK (not a static joiner) + + diff --git a/crates/ruff/src/rules/mod.rs b/crates/ruff/src/rules/mod.rs index 418693c883687..bda3d832d481c 100644 --- a/crates/ruff/src/rules/mod.rs +++ b/crates/ruff/src/rules/mod.rs @@ -32,6 +32,7 @@ pub mod flake8_tidy_imports; pub mod flake8_type_checking; pub mod flake8_unused_arguments; pub mod flake8_use_pathlib; +pub mod flynt; pub mod isort; pub mod mccabe; pub mod numpy; diff --git a/ruff.schema.json b/ruff.schema.json index a39b5d603dcd4..c60a46740c7e5 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1829,6 +1829,10 @@ "FBT001", "FBT002", "FBT003", + "FLY", + "FLY0", + "FLY00", + "FLY002", "G", "G0", "G00", From 798b97ffa679d7371c06b3e09a26cf844c511033 Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Thu, 4 May 2023 14:09:36 +0300 Subject: [PATCH 2/3] Use a constant for ruleset size instead of a magic number --- crates/ruff/src/registry/rule_set.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/ruff/src/registry/rule_set.rs b/crates/ruff/src/registry/rule_set.rs index 4cb6df76b423a..6d38df4c0a217 100644 --- a/crates/ruff/src/registry/rule_set.rs +++ b/crates/ruff/src/registry/rule_set.rs @@ -3,14 +3,16 @@ use ruff_macros::CacheKey; use std::fmt::{Debug, Formatter}; use std::iter::FusedIterator; +const RULESET_SIZE: usize = 10; + /// A set of [`Rule`]s. /// /// Uses a bitset where a bit of one signals that the Rule with that [u16] is in this set. #[derive(Clone, Default, CacheKey, PartialEq, Eq)] -pub struct RuleSet([u64; 10]); +pub struct RuleSet([u64; RULESET_SIZE]); impl RuleSet { - const EMPTY: [u64; 10] = [0; 10]; + const EMPTY: [u64; RULESET_SIZE] = [0; RULESET_SIZE]; // 64 fits into a u16 without truncation #[allow(clippy::cast_possible_truncation)] From b988b594c6de5e81a0403b908e0a1b1351bcc2b8 Mon Sep 17 00:00:00 2001 From: Aarni Koskela Date: Thu, 4 May 2023 20:34:10 +0300 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Micha Reiser --- crates/ruff/src/checkers/ast/mod.rs | 3 +-- crates/ruff/src/rules/flynt/helpers.rs | 27 ++++++++++--------- .../flynt/rules/static_join_to_fstring.rs | 19 +++++++------ 3 files changed, 24 insertions(+), 25 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index ee8faa5d0a5d3..a3faf4c1b2fb1 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2476,8 +2476,7 @@ where if self.settings.rules.enabled(Rule::StaticJoinToFString) { flynt::rules::static_join_to_fstring(self, expr, value); } - } - if attr == "format" { + } else if attr == "format" { // "...".format(...) call let location = expr.range(); match pyflakes::format::FormatSummary::try_from(value.as_ref()) { diff --git a/crates/ruff/src/rules/flynt/helpers.rs b/crates/ruff/src/rules/flynt/helpers.rs index 2979cd3c5c130..e6a6f743a2907 100644 --- a/crates/ruff/src/rules/flynt/helpers.rs +++ b/crates/ruff/src/rules/flynt/helpers.rs @@ -1,14 +1,23 @@ use ruff_python_ast::helpers::create_expr; use rustpython_parser::ast::{Constant, Expr, ExprKind}; -fn to_formatted_value_expr(inner: Expr) -> Expr { +/// Wrap an expression in a `FormattedValue` with no special formatting. +fn to_formatted_value_expr(inner: &Expr) -> Expr { create_expr(ExprKind::FormattedValue { - value: Box::new(inner), + value: Box::new(inner.clone()), conversion: 0, format_spec: None, }) } +/// Convert a string to a constant string expression. +pub(crate) fn to_constant_string(s: &str) -> Expr { + create_expr(ExprKind::Constant { + value: Constant::Str(s.to_owned()), + kind: None, + }) +} + /// Figure out if `expr` represents a "simple" call /// (i.e. one that can be safely converted to a formatted value). fn is_simple_call(expr: &Expr) -> bool { @@ -33,7 +42,7 @@ fn is_simple_callee(func: &Expr) -> bool { } /// Convert an expression to a f-string element (if it looks like a good idea). -pub fn to_fstring_elem(expr: Expr) -> Option { +pub(crate) fn to_fstring_elem(expr: &Expr) -> Option { match &expr.node { // These are directly handled by `unparse_fstring_elem`: ExprKind::Constant { @@ -41,7 +50,7 @@ pub fn to_fstring_elem(expr: Expr) -> Option { .. } | ExprKind::JoinedStr { .. } - | ExprKind::FormattedValue { .. } => Some(expr), + | ExprKind::FormattedValue { .. } => Some(expr.clone()), // These should be pretty safe to wrap in a formatted value. ExprKind::Constant { value: @@ -50,15 +59,7 @@ pub fn to_fstring_elem(expr: Expr) -> Option { } | ExprKind::Name { .. } | ExprKind::Attribute { .. } => Some(to_formatted_value_expr(expr)), - ExprKind::Call { .. } if is_simple_call(&expr) => Some(to_formatted_value_expr(expr)), + ExprKind::Call { .. } if is_simple_call(expr) => Some(to_formatted_value_expr(expr)), _ => None, } } - -/// Convert a string to a constant string expression. -pub fn to_constant_string(s: &str) -> Expr { - create_expr(ExprKind::Constant { - value: Constant::Str(s.to_owned()), - kind: None, - }) -} 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 9c872718eb27d..826604616203c 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,19 +34,21 @@ fn is_static_length(elts: &[Expr]) -> bool { .all(|e| !matches!(e.node, ExprKind::Starred { .. })) } -fn build_fstring(joiner: &str, joinees: &Vec) -> Option { +fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option { let mut fstring_elems = Vec::with_capacity(joinees.len() * 2); - for (i, expr) in joinees.iter().enumerate() { + let mut first = true; + + for expr in joinees { if matches!(expr.node, ExprKind::JoinedStr { .. }) { // Oops, already an f-string. We don't know how to handle those // gracefully right now. return None; } - let elem = helpers::to_fstring_elem(expr.clone())?; - if i != 0 { + if !first { fstring_elems.push(helpers::to_constant_string(joiner)); } - fstring_elems.push(elem); + fstring_elems.push(helpers::to_fstring_elem(expr)?); + first = false; } Some(create_expr(ExprKind::JoinedStr { values: fstring_elems, @@ -80,19 +82,16 @@ pub fn static_join_to_fstring(checker: &mut Checker, expr: &Expr, joiner: &str) let Some(new_expr) = build_fstring(joiner, joinees) else { return }; let contents = unparse_expr(&new_expr, checker.stylist); - let fixable = true; // I'm not sure there is a case where this is not fixable..? let mut diagnostic = Diagnostic::new( StaticJoinToFString { expr: contents.clone(), - fixable, + fixable: true, }, expr.range(), ); if checker.patch(diagnostic.kind.rule()) { - if fixable { - diagnostic.set_fix(Edit::range_replacement(contents, expr.range())); - } + diagnostic.set_fix(Edit::range_replacement(contents, expr.range())); } checker.diagnostics.push(diagnostic); }