From a9919707d4ea3bc3cd290a251cec948de637758b Mon Sep 17 00:00:00 2001 From: plredmond <51248199+plredmond@users.noreply.github.com> Date: Mon, 22 Apr 2024 08:40:51 -0700 Subject: [PATCH] [UP031] When encountering `"%s" % var` offer unsafe fix (#11019) Resolves #10187
Old PR description; accurate through commit e86dd7d; probably best to leave this fold closed ## Description of change In the case of a printf-style format string with only one %-placeholder and a variable at right (e.g. `"%s" % var`): * The new behavior attempts to dereference the variable and then match on the bound expression to distinguish between a 1-tuple (fix), n-tuple (bug :bug:), or a non-tuple (fix). Dereferencing is via `analyze::typing::find_binding_value`. * If the variable cannot be dereferenced, then the type-analysis routine is called to distinguish only tuple (no-fix) or non-tuple (fix). Type analysis is via `analyze::typing::is_tuple`. * If any of the above fails, the rule still fires, but no fix is offered. ## Alternatives * If the reviewers think that singling out the 1-tuple case is too complicated, I will remove that. * The ecosystem results show that no new fixes are detected. So I could probably delete all the variable dereferencing code and code that tries to generate fixes, tbh. ## Changes to existing behavior **All the previous rule-firings and fixes are unchanged except for** the "false negatives" in `crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py`. Those previous "false negatives" are now true positives and so I moved them to `crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py`.
Existing false negatives that are now true positives ``` crates/ruff_linter/resources/test/fixtures/pyupgrade/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 crates/ruff_linter/resources/test/fixtures/pyupgrade/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 crates/ruff_linter/resources/test/fixtures/pyupgrade/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 ``` One of them newly offers a fix. ``` # UP031 (no longer false negatives) -'Hello %s' % bar +'Hello {}'.format(bar) ``` This fix occurs because the new code dereferences `bar` to where it was defined earlier in the file as a non-tuple: ```python bar = {"bar": y} ``` ---
## Behavior requiring new tests Additionally, we now handle a few cases that we didn't previously test. These cases are when a string has a single %-placeholder and the righthand operand to the modulo operator is a variable **which can be dereferenced.** One of those was shown in the previous section (the "dereference non-tuple" case).
New cases handled ``` crates/ruff_linter/resources/test/fixtures/pyupgrade/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 crates/ruff_linter/resources/test/fixtures/pyupgrade/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 ``` One of these offers a fix. ``` t1 = (x,) -"%s" % t1 +"{}".format(t1[0]) # UP031: deref t1 to 1-tuple, offer fix ``` The other doesn't offer a fix because it's a bug. ---
---
## Changes to existing behavior In the case of a string with a single %-placeholder and a single ambiguous righthand argument to the modulo operator, (e.g. `"%s" % var`) the rule now fires and offers a fix. We explain about this in the "fix safety" section of the updated documentation. ## Documentation changes I swapped the order of the "known problems" and the "examples" sections so that the examples which describe the rule are first, before the exceptions to the rule are described. I also tweaked the language to be more explicit, as I had trouble understanding the documentation at first. The "known problems" section is now "fix safety" but the content is largely similar. The diff of the documentation changes looks a little difficult unless you look at the individual commits. --- .../test/fixtures/pyupgrade/UP031_0.py | 7 +++ .../test/fixtures/pyupgrade/UP031_1.py | 7 --- .../rules/printf_string_formatting.rs | 51 +++++++++-------- ...__rules__pyupgrade__tests__UP031_0.py.snap | 56 +++++++++++++++++++ 4 files changed, 90 insertions(+), 31 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..1bdcbc88c18c1 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,10 @@ cert.not_valid_after.date().isoformat().replace("-", ""), # expiration date hexlify(cert.fingerprint(hashes.SHA256())).decode("ascii")[0:8], # fingerprint prefix ) + +# UP031 (no longer false negatives; now offer potentially unsafe fixes) +'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 57f25df712c0b..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,10 +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'] 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..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 @@ -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 @@ -27,38 +28,39 @@ use crate::rules::pyupgrade::helpers::curly_escape; /// the newer `str.format` and f-strings constructs over `printf`-style string /// 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 -/// is an ambiguous expression. -/// -/// For example, given: +/// ## Example /// ```python -/// "%s" % value +/// "%s, %s" % ("Hello", "World") # "Hello, World" /// ``` /// -/// `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: -/// +/// Use instead: /// ```python -/// value = 1 -/// print("%s" % value) # "1" -/// print("{}".format(value)) # "1" -/// -/// value = (1,) -/// print("%s" % value) # "1" -/// print("{}".format(value)) # "(1,)" +/// "{}, {}".format("Hello", "World") # "Hello, World" /// ``` /// -/// ## Example +/// ## 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 -/// "%s, %s" % ("Hello", "World") # "Hello, World" +/// "%s" % val /// ``` /// -/// Use instead: +/// `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 -/// "{}, {}".format("Hello", "World") # "Hello, World" +/// val = 1 +/// print("%s" % val) # "1" +/// print("{}".format(val)) # "1" +/// +/// val = (1,) +/// print("%s" % val) # "1" +/// print("{}".format(val)) # "(1,)" /// ``` /// /// ## References @@ -434,7 +436,8 @@ pub(crate) fn printf_string_formatting(checker: &mut Checker, expr: &Expr, right // print("%s" % x) // print("{}".format(x)) // ``` - return; + // So we offer an unsafe fix: + Cow::Owned(format!("({})", checker.locator().slice(right))) } } Expr::Tuple(_) => clean_params_tuple(right, checker.locator()), 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..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 @@ -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 | # UP031 (no longer false negatives; now offer potentially unsafe fixes) | = help: Replace with format specifiers @@ -939,4 +941,58 @@ 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 | # 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 | # 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 + | +122 | 'Hello %s' % bar +123 | +124 | 'Hello %s' % bar.baz + | ^^^^^^^^^^^^^^^^^^^^ UP031 +125 | +126 | 'Hello %s' % bar['bop'] + | + = help: Replace with format specifiers + +ℹ Unsafe fix +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:126:1: UP031 [*] Use format specifiers instead of percent format + | +124 | 'Hello %s' % bar.baz +125 | +126 | 'Hello %s' % bar['bop'] + | ^^^^^^^^^^^^^^^^^^^^^^^ UP031 + | + = help: Replace with format specifiers +ℹ Unsafe fix +123 123 | +124 124 | 'Hello %s' % bar.baz +125 125 | +126 |-'Hello %s' % bar['bop'] + 126 |+'Hello {}'.format(bar['bop'])