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

[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
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AlexWaygood
Copy link
Member

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:

        msg = (
            "module or package not found: {arg} (missing __init__.py?)"
            if as_pypath
            else "file or directory not found: {arg}"
        )
        raise UsageError(msg.format(arg=arg))

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" or x = "foo"), we defer checking whether it's an "f-string with the f" 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.

@AlexWaygood AlexWaygood added the rule Implementing or modifying a lint rule label Aug 23, 2024
…`) by analyzing references to the binding when the string is assigned to a variable
@AlexWaygood AlexWaygood added the preview Related to preview mode features label Aug 23, 2024
Copy link
Contributor

github-actions bot commented Aug 23, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -20 violations, +0 -0 fixes in 6 projects; 48 projects unchanged)

apache/airflow (+0 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- tests/providers/docker/operators/test_docker.py:568:28: RUF027 Possible f-string without an `f` prefix

apache/superset (+0 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- superset/db_engine_specs/base.py:835:25: RUF027 Possible f-string without an `f` prefix

pandas-dev/pandas (+0 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- pandas/io/formats/format.py:1247:27: RUF027 Possible f-string without an `f` prefix

zulip/zulip (+0 -2 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- zerver/webhooks/librato/view.py:129:38: RUF027 Possible f-string without an `f` prefix
- zerver/webhooks/librato/view.py:148:13: RUF027 Possible f-string without an `f` prefix

pytest-dev/pytest (+0 -4 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- src/_pytest/main.py:1056:13: RUF027 Possible f-string without an `f` prefix
- src/_pytest/main.py:1058:18: RUF027 Possible f-string without an `f` prefix
- src/_pytest/main.py:1063:13: RUF027 Possible f-string without an `f` prefix
- src/_pytest/main.py:1065:18: RUF027 Possible f-string without an `f` prefix

pdm-project/pdm (+0 -11 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- src/pdm/pep582/sitecustomize.py:80:19: RUF027 Possible f-string without an `f` prefix
- src/pdm/pep582/sitecustomize.py:81:23: RUF027 Possible f-string without an `f` prefix
- src/pdm/pep582/sitecustomize.py:82:20: RUF027 Possible f-string without an `f` prefix
- src/pdm/pep582/sitecustomize.py:83:20: RUF027 Possible f-string without an `f` prefix
- src/pdm/pep582/sitecustomize.py:84:20: RUF027 Possible f-string without an `f` prefix
- src/pdm/pep582/sitecustomize.py:86:17: RUF027 Possible f-string without an `f` prefix
- src/pdm/pep582/sitecustomize.py:87:19: RUF027 Possible f-string without an `f` prefix
- src/pdm/pep582/sitecustomize.py:88:20: RUF027 Possible f-string without an `f` prefix
- tests/cli/test_run.py:172:22: RUF027 Possible f-string without an `f` prefix
- tests/cli/test_run.py:314:22: RUF027 Possible f-string without an `f` prefix
- tests/cli/test_run.py:776:41: RUF027 Possible f-string without an `f` prefix

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
RUF027 20 0 20 0 0

Copy link

codspeed-hq bot commented Aug 23, 2024

CodSpeed Performance Report

Merging #13076 will not alter performance

Comparing alex/ruf027-3 (09edde8) with main (a998320)

Summary

✅ 32 untouched benchmarks

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Aug 23, 2024

ℹ️ ecosystem check detected linter changes. (+0 -5 violations, +0 -0 fixes in 4 projects; 50 projects unchanged)

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 AlexWaygood marked this pull request as draft August 29, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant