Cyclic0007/ruff
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[UP031] When encountering "%s" % var offer unsafe fix (#11019)

Browse Source 
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.
This commit is contained in:
plredmond
2024-04-22 08:40:51 -07:00
committed by GitHub
parent 5dcb1d9e8c
commit a9919707d4
4 changed files with 92 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py vendored
View File

@@ -117,3 +117,10 @@ path = "%s-%s-%s.pem" % (
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']

7
crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py vendored
View File

@@ -32,10 +32,3 @@ pytest.param('"%8s" % (None,)', id="unsafe width-string conversion"),
"%(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']

55
crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs
View File

@@ -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,30 +28,6 @@ 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:
/// ```python
/// "%s" % value
/// ```
///
/// `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:
///
/// ```python
/// value = 1
/// print("%s" % value) # "1"
/// print("{}".format(value)) # "1"
///
/// value = (1,)
/// print("%s" % value) # "1"
/// print("{}".format(value)) # "(1,)"
/// ```
///
/// ## Example
/// ```python
/// "%s, %s" % ("Hello", "World") # "Hello, World"
@@ -61,6 +38,31 @@ use crate::rules::pyupgrade::helpers::curly_escape;
/// "{}, {}".format("Hello", "World") # "Hello, World"
/// ```
///
/// ## 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" % val
/// ```
///
/// `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
/// val = 1
/// print("%s" % val) # "1"
/// print("{}".format(val)) # "1"
///
/// val = (1,)
/// print("%s" % val) # "1"
/// print("{}".format(val)) # "(1,)"
/// ```
///
/// ## 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)
@@ -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()),

56
crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP031_0.py.snap
View File

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