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

FURB152 has too many false positives #9049

Closed
alanhdu opened this issue Dec 7, 2023 · 2 comments · Fixed by #9054
Closed

FURB152 has too many false positives #9049

alanhdu opened this issue Dec 7, 2023 · 2 comments · Fixed by #9054
Assignees
Labels
bug Something isn't working

Comments

@alanhdu
Copy link
Contributor

alanhdu commented Dec 7, 2023

FURB152 tries to replace hard-coded constants with the built-in constants (e.g. 3.1415 to math.pi). Unfortunately -- I have found it to be too false-positive happy -- e.g. it will correct 3.15 to math.pi.

At the very least, I'd argue that this should be an unsafe fix if the constant does not exactly match the builtin (since we will observe some behavioral difference). But I think we should also increase the precision required (e.g. https://github.com/astral-sh/ruff/blob/6bbabceead840e0d232950a1ed1e8f02cdafb92c/crates/ruff_linter/src/rules/refurb/rules/math_constant.rs#L61C29-L61C29) -- I don't have a strong opinion about how precise it should be, but IMO 3.15 should not be corrected to math.pi (maybe do it based on the # of digits vs the rounded version?).

Would be happy to contribute a patch if you think this a good idea.

@zanieb
Copy link
Member

zanieb commented Dec 7, 2023

There was some discussion about safety at #8727 (comment) and we're a bit divided on it. I'd prefer to leave it as safe for now and instead reduce the false positives. Unsafe isn't really intended to account for false positives i.e. if the rule was always applied correctly would it be safe? It's true that it changes the precision slightly but I don't think it should effect the intent of the code. If we still get feedback that it's causing problems, we can consider changing it.

I agree 3.15 should definitely not change to math.pi. I'd welcome a pull request resolving that issue (and similar).

@zanieb zanieb added the bug Something isn't working label Dec 7, 2023
@charliermarsh charliermarsh self-assigned this Dec 8, 2023
@charliermarsh
Copy link
Member

Gonna fix this real quick, seems bad.

charliermarsh added a commit that referenced this issue Dec 8, 2023
## Summary

We now only flag `math.pi` if the value is in `[3.14, 3.15)`, and apply
similar rules to the other constants.

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

Successfully merging a pull request may close this issue.

3 participants