From d8c2a2a86dbb33f99b06506bf0a1bc2dbd33a05e Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Thu, 18 Apr 2024 08:50:55 -0700 Subject: [PATCH 01/12] [ruff UP031, issue #10187] change docs to clarify the fix-offered and that the "problem" is only that the fix is not offered in some cases; general readability --- .../rules/printf_string_formatting.rs | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs index 11b2aedbd86a0..52c8a503c7d42 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs @@ -19,7 +19,8 @@ use crate::checkers::ast::Checker; use crate::rules::pyupgrade::helpers::curly_escape; /// ## What it does -/// Checks for `printf`-style string formatting. +/// Checks for `printf`-style string formatting, and offers to replace it with +/// `str.format` calls. /// /// ## Why is this bad? /// `printf`-style string formatting has a number of quirks, and leads to less @@ -28,27 +29,28 @@ use crate::rules::pyupgrade::helpers::curly_escape; /// formatting. /// /// ## Known problems -/// This rule is unable to detect cases in which the format string contains -/// a single, generic format specifier (e.g. `%s`), and the right-hand side +/// This rule is unable to offer a fix in cases where the format string +/// contains a single, generic format specifier (e.g. `%s`), and the right-hand side /// is an ambiguous expression. /// /// For example, given: /// ```python -/// "%s" % value +/// "%s" % val /// ``` /// -/// `value` could be a single-element tuple, _or_ it could be a single value. -/// Both of these would resolve to the same formatted string when using -/// `printf`-style formatting, but _not_ when using f-strings: +/// `val` could be a single-element tuple, _or_ a single value (not +/// contained in a tuple). Both of these would resolve to the same +/// formatted string when using `printf`-style formatting, but +/// resolve differently when using f-strings: /// /// ```python -/// value = 1 -/// print("%s" % value) # "1" -/// print("{}".format(value)) # "1" +/// val = 1 +/// print("%s" % val) # "1" +/// print("{}".format(val)) # "1" /// -/// value = (1,) -/// print("%s" % value) # "1" -/// print("{}".format(value)) # "(1,)" +/// val = (1,) +/// print("%s" % val) # "1" +/// print("{}".format(val)) # "(1,)" /// ``` /// /// ## Example From 3a74c88f1c85978ee3c1faf6d2f44da9091c3152 Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Thu, 18 Apr 2024 08:51:25 -0700 Subject: [PATCH 02/12] [ruff UP031, issue #10187] change docs to put the example ahead of the exception --- .../rules/printf_string_formatting.rs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs index 52c8a503c7d42..f80f64d551346 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs @@ -28,6 +28,16 @@ use crate::rules::pyupgrade::helpers::curly_escape; /// the newer `str.format` and f-strings constructs over `printf`-style string /// formatting. /// +/// ## Example +/// ```python +/// "%s, %s" % ("Hello", "World") # "Hello, World" +/// ``` +/// +/// Use instead: +/// ```python +/// "{}, {}".format("Hello", "World") # "Hello, World" +/// ``` +/// /// ## Known problems /// This rule is unable to offer a fix in cases where the format string /// contains a single, generic format specifier (e.g. `%s`), and the right-hand side @@ -53,16 +63,6 @@ use crate::rules::pyupgrade::helpers::curly_escape; /// print("{}".format(val)) # "(1,)" /// ``` /// -/// ## Example -/// ```python -/// "%s, %s" % ("Hello", "World") # "Hello, World" -/// ``` -/// -/// Use instead: -/// ```python -/// "{}, {}".format("Hello", "World") # "Hello, World" -/// ``` -/// /// ## References /// - [Python documentation: `printf`-style String Formatting](https://docs.python.org/3/library/stdtypes.html#old-string-formatting) /// - [Python documentation: `str.format`](https://docs.python.org/3/library/stdtypes.html#str.format) From 0826eac78ea0fff65e978d9d60503bcfb65e3b3f Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Thu, 18 Apr 2024 15:39:07 -0700 Subject: [PATCH 03/12] [ruff UP031, issue #10187] deref and handle tuples specially; tests not updated --- .../test/fixtures/pyupgrade/UP031_0.py | 12 ++++ .../test/fixtures/pyupgrade/UP031_1.py | 3 + .../rules/printf_string_formatting.rs | 66 ++++++++++++++++--- 3 files changed, 72 insertions(+), 9 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py index 00fef079b6e30..c8ca43503a0d6 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py +++ b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py @@ -117,3 +117,15 @@ cert.not_valid_after.date().isoformat().replace("-", ""), # expiration date hexlify(cert.fingerprint(hashes.SHA256())).decode("ascii")[0:8], # fingerprint prefix ) + +s = set([x, y]) +"%s" % s +# UP031: deref s to non-tuple, offer fix + +t1 = (x,) +"%s" % t1 +# UP031: deref t1 to 1-tuple, offer fix + +t2 = (x,y) +"%s" % t2 +# UP031: deref t2 to n-tuple, this is a bug diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py index 57f25df712c0b..ba31da583d0c8 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py +++ b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py @@ -39,3 +39,6 @@ 'Hello %s' % bar.baz 'Hello %s' % bar['bop'] + +"%s" % zzz +# OK: unable to deref zzz diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs index f80f64d551346..d7e96d3615b98 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs @@ -10,6 +10,7 @@ use ruff_python_literal::cformat::{ CConversionFlags, CFormatPart, CFormatPrecision, CFormatQuantity, CFormatString, }; use ruff_python_parser::{lexer, AsMode, Tok}; +use ruff_python_semantic::analyze; use ruff_python_stdlib::identifiers::is_identifier; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; @@ -288,6 +289,56 @@ fn clean_params_dictionary(right: &Expr, locator: &Locator, stylist: &Stylist) - Some(contents) } +/// (In the case where the format string has only a single %-placeholder) and the righthand operand +/// to % is a variable, try to offer a fix. +/// +/// If the variable is not a tuple, then offer `({})`. +/// ```python +/// x = set(["hi", "hello"]) +/// print("%s" % x) +/// print("{}".format(x)) +/// ``` +/// +/// If the variable is a 1-tuple, then offer `({}[0])`. +/// ```python +/// x = (1,) +/// print("%s" % x) +/// print("{}".format(x[0])) +/// ``` +/// +/// If the variable is an n-tuple, the user has a bug in their code. Offer no fix. +fn clean_params_variable<'a>(checker: &mut Checker, right: &Expr) -> Option> { + let Expr::Name(ast::ExprName { id, .. }) = right else { + return None; + }; + let semantic = checker.semantic(); + let binding_id = semantic.lookup_symbol(id.as_str())?; + let binding = semantic.binding(binding_id); + let rt = checker.locator().slice(right); + if let Some(expr) = analyze::typing::find_binding_value(binding, semantic) { + match expr { + // n-tuple + Expr::Tuple(ast::ExprTuple { elts, .. }) if 1 < elts.len() => { + return None; + } + // 1-tuple + Expr::Tuple(ast::ExprTuple { elts, .. }) if 1 == elts.len() => { + return Some(Cow::Owned(format!("({}[0])", rt))); + } + // non-tuple + _ => { + return Some(Cow::Owned(format!("({})", rt))); + } + } + } + // when find_binding_value doesn't work, fall back to offering a fix only for non-tuples + if analyze::typing::is_tuple(binding, semantic) { + return None; + } else { + return Some(Cow::Owned(format!("({})", rt))); + } +} + /// Returns `true` if the sequence of [`PercentFormatPart`] indicate that an /// [`Expr`] can be converted. fn convertible(format_string: &CFormatString, params: &Expr) -> bool { @@ -426,16 +477,13 @@ pub(crate) fn printf_string_formatting(checker: &mut Checker, expr: &Expr, right // If we have multiple fields, but no named fields, assume the right-hand side is a // tuple. Cow::Owned(format!("(*{})", checker.locator().slice(right))) + } else if let Some(cow) = clean_params_variable(checker, right) { + // With only one field, if we have a variable we can try to deref it to handle a + // few cases. + cow } else { - // Otherwise, if we have a single field, we can't make any assumptions about the - // right-hand side. It _could_ be a tuple, but it could also be a single value, - // and we can't differentiate between them. - // For example: - // ```python - // x = (1,) - // print("%s" % x) - // print("{}".format(x)) - // ``` + // Otherwise we might have an attribute/subscript/call or a variable that we were + // unable to deref. return; } } From b0791cf70323c396133d885f49dac265a34ae0ca Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Thu, 18 Apr 2024 16:42:09 -0700 Subject: [PATCH 04/12] [ruff UP031, issue #10187] emit the diagnostic even when we cannot emit a fix --- .../src/rules/pyupgrade/rules/printf_string_formatting.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs index d7e96d3615b98..ab85664a89db1 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs @@ -483,7 +483,10 @@ pub(crate) fn printf_string_formatting(checker: &mut Checker, expr: &Expr, right cow } else { // Otherwise we might have an attribute/subscript/call or a variable that we were - // unable to deref. + // unable to deref, so emit a diagnostic with no fix. + checker + .diagnostics + .push(Diagnostic::new(PrintfStringFormatting, expr.range())); return; } } From d8f3f8695cce65c4c1d2bf48a7bdc61e44c3549e Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Thu, 18 Apr 2024 16:48:38 -0700 Subject: [PATCH 05/12] [ruff UP031, issue #10187] move "false negatives" from UP031_1.py to UP031_0.py (they are now true positives) --- .../resources/test/fixtures/pyupgrade/UP031_0.py | 7 +++++++ .../resources/test/fixtures/pyupgrade/UP031_1.py | 10 ---------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py index c8ca43503a0d6..fa184be0078d9 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py +++ b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py @@ -129,3 +129,10 @@ t2 = (x,y) "%s" % t2 # UP031: deref t2 to n-tuple, this is a bug + +# UP031 (no longer false negatives) +'Hello %s' % bar + +'Hello %s' % bar.baz + +'Hello %s' % bar['bop'] diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py index ba31da583d0c8..a03cbd1d1a943 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py +++ b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py @@ -32,13 +32,3 @@ "%(1)s" % {1: 2, "1": 2} "%(and)s" % {"and": 2} - -# OK (arguably false negatives) -'Hello %s' % bar - -'Hello %s' % bar.baz - -'Hello %s' % bar['bop'] - -"%s" % zzz -# OK: unable to deref zzz From 3f73ca80c08eb74cff55da94cee41469183de864 Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Thu, 18 Apr 2024 17:13:14 -0700 Subject: [PATCH 06/12] [ruff UP031, issue #10187] cargo insta review --- ...__rules__pyupgrade__tests__UP031_0.py.snap | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP031_0.py.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP031_0.py.snap index fd5827fcfa413..806e9391f72a0 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP031_0.py.snap +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP031_0.py.snap @@ -926,6 +926,8 @@ UP031_0.py:115:8: UP031 [*] Use format specifiers instead of percent format 118 | | hexlify(cert.fingerprint(hashes.SHA256())).decode("ascii")[0:8], # fingerprint prefix 119 | | ) | |_^ UP031 +120 | +121 | s = set([x, y]) | = help: Replace with format specifiers @@ -939,4 +941,89 @@ UP031_0.py:115:8: UP031 [*] Use format specifiers instead of percent format 117 117 | cert.not_valid_after.date().isoformat().replace("-", ""), # expiration date 118 118 | hexlify(cert.fingerprint(hashes.SHA256())).decode("ascii")[0:8], # fingerprint prefix +UP031_0.py:122:1: UP031 [*] Use format specifiers instead of percent format + | +121 | s = set([x, y]) +122 | "%s" % s + | ^^^^^^^^ UP031 +123 | # UP031: deref s to non-tuple, offer fix + | + = help: Replace with format specifiers + +ℹ Unsafe fix +119 119 | ) +120 120 | +121 121 | s = set([x, y]) +122 |-"%s" % s + 122 |+"{}".format(s) +123 123 | # UP031: deref s to non-tuple, offer fix +124 124 | +125 125 | t1 = (x,) + +UP031_0.py:126:1: UP031 [*] Use format specifiers instead of percent format + | +125 | t1 = (x,) +126 | "%s" % t1 + | ^^^^^^^^^ UP031 +127 | # UP031: deref t1 to 1-tuple, offer fix + | + = help: Replace with format specifiers + +ℹ Unsafe fix +123 123 | # UP031: deref s to non-tuple, offer fix +124 124 | +125 125 | t1 = (x,) +126 |-"%s" % t1 + 126 |+"{}".format(t1[0]) +127 127 | # UP031: deref t1 to 1-tuple, offer fix +128 128 | +129 129 | t2 = (x,y) + +UP031_0.py:130:1: UP031 Use format specifiers instead of percent format + | +129 | t2 = (x,y) +130 | "%s" % t2 + | ^^^^^^^^^ UP031 +131 | # UP031: deref t2 to n-tuple, this is a bug + | + = help: Replace with format specifiers +UP031_0.py:134:1: UP031 [*] Use format specifiers instead of percent format + | +133 | # UP031 (no longer false negatives) +134 | 'Hello %s' % bar + | ^^^^^^^^^^^^^^^^ UP031 +135 | +136 | 'Hello %s' % bar.baz + | + = help: Replace with format specifiers + +ℹ Unsafe fix +131 131 | # UP031: deref t2 to n-tuple, this is a bug +132 132 | +133 133 | # UP031 (no longer false negatives) +134 |-'Hello %s' % bar + 134 |+'Hello {}'.format(bar) +135 135 | +136 136 | 'Hello %s' % bar.baz +137 137 | + +UP031_0.py:136:1: UP031 Use format specifiers instead of percent format + | +134 | 'Hello %s' % bar +135 | +136 | 'Hello %s' % bar.baz + | ^^^^^^^^^^^^^^^^^^^^ UP031 +137 | +138 | 'Hello %s' % bar['bop'] + | + = help: Replace with format specifiers + +UP031_0.py:138:1: UP031 Use format specifiers instead of percent format + | +136 | 'Hello %s' % bar.baz +137 | +138 | 'Hello %s' % bar['bop'] + | ^^^^^^^^^^^^^^^^^^^^^^^ UP031 + | + = help: Replace with format specifiers From 27dbae994bf9938494bc6a1adbb3af535641afc3 Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Thu, 18 Apr 2024 17:27:55 -0700 Subject: [PATCH 07/12] [ruff UP031, issue #10187] explain when a fix will(not) be offered in docs --- .../src/rules/pyupgrade/rules/printf_string_formatting.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs index ab85664a89db1..7dd9c1a13d566 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs @@ -63,6 +63,8 @@ use crate::rules::pyupgrade::helpers::curly_escape; /// print("%s" % val) # "1" /// print("{}".format(val)) # "(1,)" /// ``` +/// In most cases, where `val` is not a literal that our analysis can locate, +/// no fix will be offered (but in these specific examples, a fix exists). /// /// ## References /// - [Python documentation: `printf`-style String Formatting](https://docs.python.org/3/library/stdtypes.html#old-string-formatting) From e86dd7d0cb26e272611ed74164b5fb92d6a7548d Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Thu, 18 Apr 2024 17:42:23 -0700 Subject: [PATCH 08/12] [ruff UP031, issue #10187] cargo clippy --- .../src/rules/pyupgrade/rules/printf_string_formatting.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs index 7dd9c1a13d566..4f3161b21d269 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs @@ -325,20 +325,19 @@ fn clean_params_variable<'a>(checker: &mut Checker, right: &Expr) -> Option { - return Some(Cow::Owned(format!("({}[0])", rt))); + return Some(Cow::Owned(format!("({rt}[0])"))); } // non-tuple _ => { - return Some(Cow::Owned(format!("({})", rt))); + return Some(Cow::Owned(format!("({rt})"))); } } } // when find_binding_value doesn't work, fall back to offering a fix only for non-tuples if analyze::typing::is_tuple(binding, semantic) { return None; - } else { - return Some(Cow::Owned(format!("({})", rt))); } + return Some(Cow::Owned(format!("({rt})"))); } /// Returns `true` if the sequence of [`PercentFormatPart`] indicate that an From 210613de31213d7695e44d4212543016b719f72c Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Fri, 19 Apr 2024 10:12:06 -0700 Subject: [PATCH 09/12] [ruff UP031, issue #10187] change docs "known problems" to "fix safety" and change the prose to follow --- .../rules/pyupgrade/rules/printf_string_formatting.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs index 4f3161b21d269..a609ab60f9a4a 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs @@ -39,10 +39,10 @@ use crate::rules::pyupgrade::helpers::curly_escape; /// "{}, {}".format("Hello", "World") # "Hello, World" /// ``` /// -/// ## Known problems -/// This rule is unable to offer a fix in cases where the format string -/// contains a single, generic format specifier (e.g. `%s`), and the right-hand side -/// is an ambiguous expression. +/// ## Fix safety +/// In cases where the format string contains a single generic format specifier +/// (e.g. `%s`), and the right-hand side is an ambiguous expression, +/// we cannot offer a safe fix. /// /// For example, given: /// ```python @@ -63,8 +63,6 @@ use crate::rules::pyupgrade::helpers::curly_escape; /// print("%s" % val) # "1" /// print("{}".format(val)) # "(1,)" /// ``` -/// In most cases, where `val` is not a literal that our analysis can locate, -/// no fix will be offered (but in these specific examples, a fix exists). /// /// ## References /// - [Python documentation: `printf`-style String Formatting](https://docs.python.org/3/library/stdtypes.html#old-string-formatting) From 8d74c0f8ea6d926a4c1e78552088e00191f2f5d3 Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Fri, 19 Apr 2024 10:12:37 -0700 Subject: [PATCH 10/12] [ruff UP031, issue #10187] remove test examples meant to exercise the variable inference/deref code --- .../resources/test/fixtures/pyupgrade/UP031_0.py | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py index fa184be0078d9..1bdcbc88c18c1 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py +++ b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py @@ -118,19 +118,7 @@ hexlify(cert.fingerprint(hashes.SHA256())).decode("ascii")[0:8], # fingerprint prefix ) -s = set([x, y]) -"%s" % s -# UP031: deref s to non-tuple, offer fix - -t1 = (x,) -"%s" % t1 -# UP031: deref t1 to 1-tuple, offer fix - -t2 = (x,y) -"%s" % t2 -# UP031: deref t2 to n-tuple, this is a bug - -# UP031 (no longer false negatives) +# UP031 (no longer false negatives; now offer potentially unsafe fixes) 'Hello %s' % bar 'Hello %s' % bar.baz From 08ec1d35dd77371288ecacc96f5e9c7a918f028e Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Fri, 19 Apr 2024 10:26:15 -0700 Subject: [PATCH 11/12] [ruff UP031, issue #10187] offer the fix regardless of whether the ambiguous variable is a tuple or not --- .../rules/printf_string_formatting.rs | 71 +++---------------- 1 file changed, 11 insertions(+), 60 deletions(-) diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs index a609ab60f9a4a..3c53924d6df4f 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs @@ -10,7 +10,6 @@ use ruff_python_literal::cformat::{ CConversionFlags, CFormatPart, CFormatPrecision, CFormatQuantity, CFormatString, }; use ruff_python_parser::{lexer, AsMode, Tok}; -use ruff_python_semantic::analyze; use ruff_python_stdlib::identifiers::is_identifier; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; @@ -289,55 +288,6 @@ fn clean_params_dictionary(right: &Expr, locator: &Locator, stylist: &Stylist) - Some(contents) } -/// (In the case where the format string has only a single %-placeholder) and the righthand operand -/// to % is a variable, try to offer a fix. -/// -/// If the variable is not a tuple, then offer `({})`. -/// ```python -/// x = set(["hi", "hello"]) -/// print("%s" % x) -/// print("{}".format(x)) -/// ``` -/// -/// If the variable is a 1-tuple, then offer `({}[0])`. -/// ```python -/// x = (1,) -/// print("%s" % x) -/// print("{}".format(x[0])) -/// ``` -/// -/// If the variable is an n-tuple, the user has a bug in their code. Offer no fix. -fn clean_params_variable<'a>(checker: &mut Checker, right: &Expr) -> Option> { - let Expr::Name(ast::ExprName { id, .. }) = right else { - return None; - }; - let semantic = checker.semantic(); - let binding_id = semantic.lookup_symbol(id.as_str())?; - let binding = semantic.binding(binding_id); - let rt = checker.locator().slice(right); - if let Some(expr) = analyze::typing::find_binding_value(binding, semantic) { - match expr { - // n-tuple - Expr::Tuple(ast::ExprTuple { elts, .. }) if 1 < elts.len() => { - return None; - } - // 1-tuple - Expr::Tuple(ast::ExprTuple { elts, .. }) if 1 == elts.len() => { - return Some(Cow::Owned(format!("({rt}[0])"))); - } - // non-tuple - _ => { - return Some(Cow::Owned(format!("({rt})"))); - } - } - } - // when find_binding_value doesn't work, fall back to offering a fix only for non-tuples - if analyze::typing::is_tuple(binding, semantic) { - return None; - } - return Some(Cow::Owned(format!("({rt})"))); -} - /// Returns `true` if the sequence of [`PercentFormatPart`] indicate that an /// [`Expr`] can be converted. fn convertible(format_string: &CFormatString, params: &Expr) -> bool { @@ -476,17 +426,18 @@ pub(crate) fn printf_string_formatting(checker: &mut Checker, expr: &Expr, right // If we have multiple fields, but no named fields, assume the right-hand side is a // tuple. Cow::Owned(format!("(*{})", checker.locator().slice(right))) - } else if let Some(cow) = clean_params_variable(checker, right) { - // With only one field, if we have a variable we can try to deref it to handle a - // few cases. - cow } else { - // Otherwise we might have an attribute/subscript/call or a variable that we were - // unable to deref, so emit a diagnostic with no fix. - checker - .diagnostics - .push(Diagnostic::new(PrintfStringFormatting, expr.range())); - return; + // Otherwise, if we have a single field, we can't make any assumptions about the + // right-hand side. It _could_ be a tuple, but it could also be a single value, + // and we can't differentiate between them. + // For example: + // ```python + // x = (1,) + // print("%s" % x) + // print("{}".format(x)) + // ``` + // So we offer an unsafe fix: + Cow::Owned(format!("({})", checker.locator().slice(right))) } } Expr::Tuple(_) => clean_params_tuple(right, checker.locator()), From 8c52f88db4c50e04b9576497e6b2224ae955328c Mon Sep 17 00:00:00 2001 From: PLR <51248199+plredmond@users.noreply.github.com> Date: Fri, 19 Apr 2024 10:40:36 -0700 Subject: [PATCH 12/12] [ruff UP031, issue #10187] cargo insta review - small change from the merge-base, despite apparent churn in this commit --- ...__rules__pyupgrade__tests__UP031_0.py.snap | 105 ++++++------------ 1 file changed, 37 insertions(+), 68 deletions(-) diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP031_0.py.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP031_0.py.snap index 806e9391f72a0..1a200861b65f8 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP031_0.py.snap +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP031_0.py.snap @@ -927,7 +927,7 @@ UP031_0.py:115:8: UP031 [*] Use format specifiers instead of percent format 119 | | ) | |_^ UP031 120 | -121 | s = set([x, y]) +121 | # UP031 (no longer false negatives; now offer potentially unsafe fixes) | = help: Replace with format specifiers @@ -943,87 +943,56 @@ UP031_0.py:115:8: UP031 [*] Use format specifiers instead of percent format UP031_0.py:122:1: UP031 [*] Use format specifiers instead of percent format | -121 | s = set([x, y]) -122 | "%s" % s - | ^^^^^^^^ UP031 -123 | # UP031: deref s to non-tuple, offer fix +121 | # UP031 (no longer false negatives; now offer potentially unsafe fixes) +122 | 'Hello %s' % bar + | ^^^^^^^^^^^^^^^^ UP031 +123 | +124 | 'Hello %s' % bar.baz | = help: Replace with format specifiers ℹ Unsafe fix 119 119 | ) 120 120 | -121 121 | s = set([x, y]) -122 |-"%s" % s - 122 |+"{}".format(s) -123 123 | # UP031: deref s to non-tuple, offer fix -124 124 | -125 125 | t1 = (x,) - -UP031_0.py:126:1: UP031 [*] Use format specifiers instead of percent format +121 121 | # UP031 (no longer false negatives; now offer potentially unsafe fixes) +122 |-'Hello %s' % bar + 122 |+'Hello {}'.format(bar) +123 123 | +124 124 | 'Hello %s' % bar.baz +125 125 | + +UP031_0.py:124:1: UP031 [*] Use format specifiers instead of percent format | -125 | t1 = (x,) -126 | "%s" % t1 - | ^^^^^^^^^ UP031 -127 | # UP031: deref t1 to 1-tuple, offer fix +122 | 'Hello %s' % bar +123 | +124 | 'Hello %s' % bar.baz + | ^^^^^^^^^^^^^^^^^^^^ UP031 +125 | +126 | 'Hello %s' % bar['bop'] | = help: Replace with format specifiers ℹ Unsafe fix -123 123 | # UP031: deref s to non-tuple, offer fix -124 124 | -125 125 | t1 = (x,) -126 |-"%s" % t1 - 126 |+"{}".format(t1[0]) -127 127 | # UP031: deref t1 to 1-tuple, offer fix -128 128 | -129 129 | t2 = (x,y) - -UP031_0.py:130:1: UP031 Use format specifiers instead of percent format - | -129 | t2 = (x,y) -130 | "%s" % t2 - | ^^^^^^^^^ UP031 -131 | # UP031: deref t2 to n-tuple, this is a bug - | - = help: Replace with format specifiers +121 121 | # UP031 (no longer false negatives; now offer potentially unsafe fixes) +122 122 | 'Hello %s' % bar +123 123 | +124 |-'Hello %s' % bar.baz + 124 |+'Hello {}'.format(bar.baz) +125 125 | +126 126 | 'Hello %s' % bar['bop'] -UP031_0.py:134:1: UP031 [*] Use format specifiers instead of percent format +UP031_0.py:126:1: UP031 [*] Use format specifiers instead of percent format | -133 | # UP031 (no longer false negatives) -134 | 'Hello %s' % bar - | ^^^^^^^^^^^^^^^^ UP031 -135 | -136 | 'Hello %s' % bar.baz +124 | 'Hello %s' % bar.baz +125 | +126 | 'Hello %s' % bar['bop'] + | ^^^^^^^^^^^^^^^^^^^^^^^ UP031 | = help: Replace with format specifiers ℹ Unsafe fix -131 131 | # UP031: deref t2 to n-tuple, this is a bug -132 132 | -133 133 | # UP031 (no longer false negatives) -134 |-'Hello %s' % bar - 134 |+'Hello {}'.format(bar) -135 135 | -136 136 | 'Hello %s' % bar.baz -137 137 | - -UP031_0.py:136:1: UP031 Use format specifiers instead of percent format - | -134 | 'Hello %s' % bar -135 | -136 | 'Hello %s' % bar.baz - | ^^^^^^^^^^^^^^^^^^^^ UP031 -137 | -138 | 'Hello %s' % bar['bop'] - | - = help: Replace with format specifiers - -UP031_0.py:138:1: UP031 Use format specifiers instead of percent format - | -136 | 'Hello %s' % bar.baz -137 | -138 | 'Hello %s' % bar['bop'] - | ^^^^^^^^^^^^^^^^^^^^^^^ UP031 - | - = help: Replace with format specifiers +123 123 | +124 124 | 'Hello %s' % bar.baz +125 125 | +126 |-'Hello %s' % bar['bop'] + 126 |+'Hello {}'.format(bar['bop'])