[ruff] New rule float-comparison (RUF069)#20585
Conversation
|
We need more tests and cases where the rule can give FP and so on |
|
Also, I randomly picked CODE for this rule |
|
I haven't reviewed, but if there's not a corresponding E723 rule in pycodestyle, I would default to making this a ruff rule. We have a PR in progress for RUF066, so RUF067 would be the next free code. |
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF069 | 1130 | 1130 | 0 | 0 | 0 |
pycodestyle] New rule float-comparison (E723)ruff] New rule float-comparison (RUF067)
This breaks the behavior of code and results in: |
|
What should we do in such a case? Should we somehow mark this fact in the documentation or what should we do, because we will not be able to check in general that such calls will not lead to some code breakages |
|
The rule detects potentially problematic float comparisons correctly, but we should not suggest replacing only |
|
Would it help with the false positives, and maybe with the numpy cases, to apply the rule only in boolean contexts? I think we have other rules that apply only to expressions used as ruff/crates/ruff_python_semantic/src/model.rs Line 1974 in 5f136c2 This rule is going to require type inference to get exactly right anyway, so this might be a good way to be more conservative in the meantime. I also only gave the implementation a quick skim and could have overlooked this, but does this rule apply to comparisons like |
Yes, this only applies to |
ntBre
left a comment
There was a problem hiding this comment.
Thank you! And sorry for the delay on the review.
I think this will be a really helpful rule, but I'm a bit wary of recommending np.isclose. What do you think about trying to restrict the rule to plain floats for now? We could always expand the scope later.
My other comments were just small suggestions.
crates/ruff_linter/src/rules/ruff/rules/float_equality_comparison.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/float_equality_comparison.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/float_equality_comparison.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/float_equality_comparison.rs
Outdated
Show resolved
Hide resolved
Then we will severely restrict the rule, since comparing two floats is less common |
|
|
ruff] New rule float-comparison (RUF067)ruff] New rule float-comparison (RUF070)
|
I also slightly updated the code to find the following cases: def foo(a, b):
return a == b - 0.1
def foo(a, b):
return a == b - float(“2”)Ecosystem report should become larger (upd. 1008 -> 1148) |
crates/ruff_linter/src/rules/ruff/rules/float_equality_comparison.rs
Outdated
Show resolved
Hide resolved
ntBre
left a comment
There was a problem hiding this comment.
Thanks! I found a couple more very small nits in the code, and I also found some interesting cases in the ecosystem report:
- tests/integration/tools/test_box_zoom_tool.py:106:16: RUF069 Unreliable floating point equality comparison
(results['xrstart'] + results['xrend'])/2.0 == pytest.approx(0.25) - tests/integration/tools/test_box_zoom_tool.py:107:16: RUF069 Unreliable floating point equality comparison
(results['yrstart'] + results['yrend'])/2.0 == pytest.approx(0.75)
I think these are technically false positives because pytest.approx is another way to solve the approximate equality issue.
- airflow-core/tests/unit/models/test_pool.py:205:16: RUF069 Unreliable floating point equality comparison
float("inf") == pool.open_slots()
If I understand correctly, this is actually a safe comparison because two infinities can be equal, unlike NaNs.
I think we should try to suppress these two cases before landing.
crates/ruff_linter/src/rules/ruff/rules/float_equality_comparison.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/float_equality_comparison.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/float_equality_comparison.rs
Outdated
Show resolved
Hide resolved
…son.rs Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
…son.rs Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
It seems that everything should be fine now. |
ntBre
left a comment
There was a problem hiding this comment.
Thanks for your patience and all of your work here. I think I found one more issue with math.inf, but this otherwise looks great to me.
crates/ruff_linter/src/rules/ruff/rules/float_equality_comparison.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/float_equality_comparison.rs
Outdated
Show resolved
Hide resolved
ntBre
left a comment
There was a problem hiding this comment.
Thank you! I pushed two commits slightly simplifying the math.inf checks (resolve_qualified_name already handles both names and attributes) and adding a test case for complex("inf"). I think this is ready to land!

Summary
Part of #14220
Test Plan
cargo nextest run ruf069