[ruff] Treat f-string interpolation as potential side effect in RUF019#24426
Conversation
|
MichaReiser
force-pushed
the
fix-ruf019-fstring-side-effects
branch
from
e27a024 to
c2b5a83
Compare
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
I think this is somewhat pedantic but it's probably fine for RUF019. I left a few smaller nit comments on how we can improve the code.
|
@MichaReiser Thank you so much for the review! Addressed all feedback, refactored to SideEffect::from_expr with a match, renamed variants to Absent/Possible/Present, added Named as a side effect, and updated the fixture with a class that has a side-effectful str. I really appreciate the suggestions. Ready for re-review whenever you have a moment. |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This is great. Thank you. A few more smaller nits.
|
@MichaReiser thank you so much for the review. Addressed all three feedback, removed Option from from_expr, made both matches exhaustive, added const fn. Ready for another look whenever you get a chance. thank you |
#24426) <!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? - Does this PR follow our AI policy (https://github.com/astral-sh/.github/blob/main/AI_POLICY.md)? --> ## Summary Fixes #12953 F-string interpolation can call `__format__`/`__str__`/`__repr__`, which may have side effects. RUF019 was applying a safe auto-fix that collapsed two `__str__` calls into one, changing behavior. Added a tri-state `SideEffect` enum (`No`/`Maybe`/`Yes`) and a `side_effect()` function that reuses the existing `any_over_expr` traversal via a new `FnMut` variant (`any_over_expr_mut`), following the approach suggested by @ntBre in the other closed pr tagged in the issue In RUF019, `SideEffect::Maybe` (non-literal f-string interpolation) now produces an unsafe fix instead of a safe one. Literal interpolations like `f"{1}"` remain safe. ## Test Plan - Added f-string fixture cases to `RUF019.py` (non-literal → unsafe, literal → safe, no interpolation → safe). - `cargo nextest run -p ruff_linter` - Ecosystem check (stable + preview)
3 participants
Summary
Fixes #12953
F-string interpolation can call
__format__/__str__/__repr__, which may have side effects. RUF019 was applying a safe auto-fix that collapsed two__str__calls into one, changing behavior.Added a tri-state
SideEffectenum (No/Maybe/Yes) and aside_effect()function that reuses the existingany_over_exprtraversal via a newFnMutvariant (any_over_expr_mut), following the approach suggested by @ntBre in the other closed pr tagged in the issueIn RUF019,
SideEffect::Maybe(non-literal f-string interpolation) now produces an unsafe fix instead of a safe one. Literal interpolations likef"{1}"remain safe.Test Plan
RUF019.py(non-literal → unsafe, literal → safe, no interpolation → safe).cargo nextest run -p ruff_linter