Skip to content

Commit

Permalink
[UP031] When encountering "%s" % var offer unsafe fix (#11019)
Browse files Browse the repository at this point in the history
Resolves #10187

<details>
<summary>Old PR description; accurate through commit e86dd7d; probably
best to leave this fold closed</summary>

## 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 🐛), 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`.

<details>
<summary>Existing false negatives that are now true positives</summary>

```
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}
```

---

</details>

## 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).

<details>
<summary>New cases handled</summary>

```
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.

---

</details>

---

</details>


## 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.
  • Loading branch information
plredmond committed Apr 22, 2024
1 parent 5dcb1d9 commit a991970
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Original file line number Diff line number Diff line change
Expand Up @@ -19,46 +19,48 @@ 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
/// readable code than using `str.format` calls or f-strings. In general, prefer
/// 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
Expand Down Expand Up @@ -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()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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'])

0 comments on commit a991970

Please sign in to comment.