Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UP031 not violated for variables, only literal values #10187

Closed
lev-blit opened this issue Mar 1, 2024 · 5 comments · Fixed by #11019
Closed

UP031 not violated for variables, only literal values #10187

lev-blit opened this issue Mar 1, 2024 · 5 comments · Fixed by #11019
Assignees
Labels
good first issue Good for newcomers rule Implementing or modifying a lint rule

Comments

@lev-blit
Copy link

lev-blit commented Mar 1, 2024

UP031 (printf-string-formatting, suggests using str.format instead of percent formatting) doesn't trigger when using a variable and not a literal value.

Even using the example from the docs -

value = 1
print("%s" % value)  # seems to be OK

print("%s" % 1)  # violating UP031

I've tried this out in the rust playground as well (here's an example) with version 0.3.0, selecting all UP rules

@charliermarsh
Copy link
Member

I believe the docs are wrong the rule is correct. It's not safe to inject an arbitrary variable unless we can definitively infer its type. Here's an example to illustrate why:

>>> value = 1
>>> print("%s" % value)
1
>>> value = (1,)
>>> print("%s" % value)
1
>>> value = 1
>>> print(f"{value}")
1
>>> value = (1,)
>>> print(f"{value}")
(1,)

We could improve the logic a bit to allow replacements in cases in which we can definitively infer the type (like above), but in general we won't always be able to do this conversion.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Mar 2, 2024
@zanieb zanieb added the good first issue Good for newcomers label Mar 11, 2024
@zanieb
Copy link
Member

zanieb commented Mar 11, 2024

The documentation part seems like a good first issue. Inference of the type seems a bit harder to implement but definitely doable if someone is interested.

@omar-abdelgawad
Copy link

Hi, I am interested in taking the documentation part of this issue. I am also new here 😄.

However, I need some clarifications regarding what needs to be updated. From what I have understood, The rule implementation is correct and it is successful in detecting both safe (when we know its definitely not a tuple) and unsafe (when there is a chance it might be a tuple) fixes. The only part missing from the rule is further type detection to find all cases where the variable is NOT a tuple. The thing is, when I read the docs now I understand the exact same thing with no mistakes found.

Did some PR fix this already and forget to mention this issue or am I just confused?

@zanieb
Copy link
Member

zanieb commented Apr 18, 2024

Hey @omar-abdelgawad — you're right this example from the docs is already a part of the "Known problems" section. I don't think there's a documentation change to be made here until we improve inference.

@zanieb
Copy link
Member

zanieb commented Apr 18, 2024

Welcome to the project :) sorry about the confusion.

plredmond added a commit that referenced this issue Apr 19, 2024
… that the "problem" is only that the fix is not offered in some cases; general readability
plredmond added a commit that referenced this issue Apr 19, 2024
plredmond added a commit that referenced this issue Apr 19, 2024
plredmond added a commit that referenced this issue Apr 19, 2024
plredmond added a commit that referenced this issue Apr 19, 2024
plredmond added a commit that referenced this issue Apr 19, 2024
… merge-base, despite apparent churn in this commit
@plredmond plredmond self-assigned this Apr 22, 2024
plredmond added a commit that referenced this issue Apr 22, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants