[ruff] Reduce false positives for missing-fstring-syntax (RUF027) by analyzing references to the binding when the string is assigned to a variable#13076
Draft
AlexWaygood wants to merge 4 commits intomainfrom
Draft
Conversation
…`) by analyzing references to the binding when the string is assigned to a variable
AlexWaygood
force-pushed
the
alex/ruf027-3
branch
from
25cd016 to
fdf8fc6
Compare
Contributor
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF027 | 20 | 0 | 20 | 0 | 0 |
AlexWaygood
force-pushed
the
alex/ruf027-3
branch
from
f09ec0b to
d4ada3f
Compare
CodSpeed Performance ReportMerging #13076 will not alter performanceComparing Summary
|
Member
Author
That's 5 false positives going away, so the PR is "working". I was hoping for the impact to be a little bigger, though. I suppose cases like this aren't counted as "simple assignments" according to the logic I added because the r.h.s. is a conditional expression rather than just a string literal. This is slightly underwhelming, but I think it's still an improvement. I think it's probably worth the extra complexity, but not sure. |
AlexWaygood
force-pushed
the
alex/ruf027-3
branch
from
45fa17a to
09edde8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When we tried to stabilise RUF027 for the 0.6 release earlier this month, the ecosystem report revealed lots of false positives along the lines of this:
A human can see quite clearly that this isn't meant to be an f-string, since the variable it's assigned to immediately has
.format()called on it. Because of the indirection in the code, however, the logic for RUF027 can't see that right now, since it only checks for methods being directly called on strings with braces; it doesn't check for methods being called on symbols that have strings with braces as their assigned value.This PR tackles these false positives by splitting the check into two functions. If we detect that a string literal is part of a "simple assignment" (
x: str = "foo"orx = "foo"), we defer checking whether it's an "f-string with thef" until the entire AST has been visited. This enables us to iterate through all the references to the binding and check whether any of them are used in ways that make it clear the string is intended to be a template string rather than an f-string. (For string literals that are not inside simple assignments, we analyze them as we did before, while we're doing our initial traversal of the AST.)Test Plan
I added some new fixtures, but I won't consider this PR a success unless it results in some false positives going away from the ecosystem report. Otherwise, I don't think it's worth the added complexity.