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

FURB188 false positive for affix expression with side effect #15334

Open
dscorbett opened this issue Jan 7, 2025 · 2 comments
Open

FURB188 false positive for affix expression with side effect #15334

dscorbett opened this issue Jan 7, 2025 · 2 comments
Labels
bug Something isn't working fixes Related to suggested fixes for violations

Comments

@dscorbett
Copy link

slice-to-remove-prefix-or-suffix (FURB188) produces a false positive in Ruff 0.8.6 when the affix expression has a side effect.

$ cat furb188.py
gen = (x for x in ["1", "23"])
string = "0x1"
print(string[:-len(next(gen))] if string.endswith(next(gen)) else string)

$ python furb188.py
0

$ ruff check --isolated --preview --select FURB188 furb188.py --fix
Found 1 error (1 fixed, 0 remaining).

$ cat furb188.py
gen = (x for x in ["1", "23"])
string = "0x1"
print(string.removesuffix(next(gen)))

$ python furb188.py
0x
@AlexWaygood AlexWaygood added bug Something isn't working fixes Related to suggested fixes for violations labels Jan 7, 2025
@AlexWaygood
Copy link
Member

While you're absolutely correct on this, and we should fix it, it feels like enough of an edge case that I think we can still stabilise this rule as part of Ruff 0.9. It's just very unlikely that anybody would write code like this, realistically, I think :-)

Cc. @MichaReiser and @dylwil3 in case either of you disagree, though.

@dylwil3
Copy link
Collaborator

dylwil3 commented Jan 7, 2025

Was just about to write the same thing - I've never seen this before... and it would seem very strange to rely on that behavior in any case.

It's not even clear to me that we should try to fix this behavior? In a dynamically typed language like Python, even with an excellent type checker, I think it's impractical (bordering on impossible?) to tell if function calls have side effects, statically. At the moment the only prior art in Ruff itself is very heavy-handed: we just assume that all functions have side effects (except for builtins that we think are not being overloaded):

/// Return `true` if the `Expr` contains an expression that appears to include a
/// side-effect (like a function call).
///
/// Accepts a closure that determines whether a given name (e.g., `"list"`) is a Python builtin.
pub fn contains_effect<F>(expr: &Expr, is_builtin: F) -> bool

That doesn't really seem appropriate here. We probably could (and should) make this method smarter by teaching it about various builtins and things from the standard library, but for any given rule I think we have to accept some level of approximation to the truth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixes Related to suggested fixes for violations
Projects
None yet
Development

No branches or pull requests

3 participants