astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
33.38k stars 1.11k forks source link

`contains_effect` should consider f-strings to have effects #12953

Open dscorbett opened 3 months ago

dscorbett commented 3 months ago

contains_effect should consider f-strings to have effects because of the implicit __str__ or __repr__ call. This affects any rule that relies on contains_effect; here is an example with RUF019.

$ ruff --version
ruff 0.6.1
$ cat ruf019.py
class C:
    def __init__(self):
        self.i = 0

    def __str__(self):
        self.i += 1
        return str(self.i)

if __name__ == "__main__":
    c = C()
    dct = {"1": "a", "2": "b"}
    if f"{c}" in dct and dct[f"{c}"]:
        pass
    print(c.i)
$ python ruf019.py
2
$ ruff check --isolated --select RUF019 ruf019.py --fix
Found 1 error (1 fixed, 0 remaining).
$ python ruf019.py
1

Some f-strings can be statically proven to not have effects, if they contain no formatted expressions (f"x") or they only format values of types formatting which is known not to have effects (f"{1}").

charliermarsh commented 3 months ago

I would vote against changing this... The same could be said for attribute accesses, and we don't include those. I would say that rules that rely on detecting effects should have unsafe fixes, in general, instead of taking away user access to the fixes entirely.

dscorbett commented 3 months ago

Some expressions, like literals, definitely don’t have side effects, so their fixes needn’t be marked unsafe. contains_effect could have three return values, indicating the availability of safe fixes (definitely no side effect), unsafe fixes (maybe a side effect, but probably not), and no fixes (probably a side effect).