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

FURB105 fix removes sep arguments with side effects #13126

Closed
dscorbett opened this issue Aug 27, 2024 · 1 comment · Fixed by #13165
Closed

FURB105 fix removes sep arguments with side effects #13126

dscorbett opened this issue Aug 27, 2024 · 1 comment · Fixed by #13165
Assignees
Labels
accepted Ready for implementation bug Something isn't working fixes Related to suggested fixes for violations

Comments

@dscorbett
Copy link

The fix for FURB105 removes unneeded sep arguments even when they might have side effects. In that case, the fix should be omitted or marked unsafe.

$ ruff --version
ruff 0.6.2
$ cat furb105.py
print(sep=print("sep"))
$ python furb105.py
sep

$ ruff check --isolated --select FURB105 furb105.py --fix
Found 1 error (1 fixed, 0 remaining).
$ cat furb105.py
print()
$ python furb105.py

@AlexWaygood AlexWaygood added bug Something isn't working fixes Related to suggested fixes for violations labels Aug 27, 2024
@AlexWaygood
Copy link
Member

I'd be happy to accept a PR that marked the fix as unsafe if we detected the argument to sep= as something that could have a side effect. We could use the contains_effect function we have here for that purpose:

pub fn contains_effect<F>(expr: &Expr, is_builtin: F) -> bool
.

However, I think we should still offer the fix (though it'll be marked as unsafe), even in these situations. I think it'll honestly be relatively rare for somebody to pass in a function that has a side effect to sep= here, and contains_effect() errs on the side of assuming that arbitrary functions might have side effects, although in reality they often won't.

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

Successfully merging a pull request may close this issue.

3 participants