[ruff] Treat f-string interpolation as potential side effect in RUF019#24426
Conversation
|
e27a024 to
c2b5a83
Compare
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
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 |
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