Repository navigation
[refurb] Mark fix unsafe and move to suspicious (FURB152) - #28405
Merged
Merged
Conversation
…B152) `math-constant` matches any float literal that rounds to `pi`, `e` or `tau` at 2 to 15 decimal places, so `3.14` and `2.718` are reported alongside full precision spellings. The fix was always marked safe, which meant `ruff check --fix` silently rewrote those shorter literals to `math.pi` and `math.e` and changed the values the program computes with. Measurements, thresholds and test fixtures that happen to start with the same digits were altered without review. Mark the fix safe only when the literal already denotes the same float as the constant, so substituting it cannot change a result, and unsafe otherwise. Move the rule from the `correctness` category to `suspicious`, since matching a constant's leading digits does not establish that the constant was meant.
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| math-constant | 250 | 125 | 125 | 0 | 0 |
ntBre
reviewed
Oct 2, 2026
ntBre
left a comment
Contributor
There was a problem hiding this comment.
Thanks! Just a couple of simplification suggestions.
Comment on lines
+98
to
+107
| // The rule matches any literal that rounds to the constant, so `3.14` is reported | ||
| // just as `3.141592653589793` is. Substituting the constant only leaves the | ||
| // program computing the same values in the latter case; in the former it silently | ||
| // replaces the author's number with a different one. | ||
| #[expect( | ||
| clippy::float_cmp, | ||
| reason = "an exact comparison is the point: only a literal that is already the \ | ||
| same float can be replaced without changing what the program computes" | ||
| )] | ||
| let is_exact = value == constant.value(); |
Contributor
There was a problem hiding this comment.
I think I would probably just always mark the fix unsafe. It's not a ton of complexity, but it would still be a little simpler and avoid so much explanation in the docs, and it seems relatively rare to have pasted the exact value of a constant without using the constant, so we'll rarely offer the safe fix anyway.
Comment on lines
+29
to
+41
| /// ## Known problems | ||
| /// A literal is matched on its digits alone, so a value that merely happens to | ||
| /// begin with the same digits as a mathematical constant is flagged even when it | ||
| /// means something else entirely. A measurement that rounds to `3.14`, or a price | ||
| /// of `2.718`, is indistinguishable here from an approximation of π or e. | ||
| /// | ||
| /// ## Fix safety | ||
| /// This rule's fix is marked as safe only when the literal already denotes exactly | ||
| /// the same float as the constant, so that replacing it cannot change a result. | ||
| /// Shorter approximations are rewritten under an unsafe fix, because `math.pi` is | ||
| /// not equal to `3.14`: a literal that was deliberately chosen — a rounded | ||
| /// measurement, a threshold, a test fixture — computes a different value once it | ||
| /// becomes a constant. |
Contributor
There was a problem hiding this comment.
Assuming we implement my other comment, let's fold this into a much shorter fix safety section. All we really need to say is that the fix is unsafe because it changes the result of calculations.
Per review: drop the exact-literal special case and shorten the fix safety section.
refurb] Only mark math-constant fix safe for exact literals (FURB152)refurb] Mark math-constant fix as unsafe (FURB152)
refurb] Mark math-constant fix as unsafe (FURB152)refurb] Mark fix unsafe and move to suspicious (FURB152)
ntBre
approved these changes
Oct 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
matches_constanttreats a literal as a match if it equals π, e, or τ rounded or truncated anywhere from 2 to 15 decimal places. Truncating π at two places gives3.14, so3.14matches and gets rewritten tomath.pi.On its own that's just an aggressive heuristic. What made it bite is that the fix was always
Fix::safe_edits, and ruff applies safe fixes under a plain--fixwith nothing to review. The reporter had a crop-harvest constant of3.14that turned intomath.pi(3.141592653589793), and a2.718that turned intomath.e. Their numbers changed and nothing told them.The fix here marks it unsafe, so every rewrite now needs
--unsafe-fixes. I also moved the rule fromcorrectnesstosuspicious, per @ntBre's comment.References
Closes: #28127
Test Plan
Every fix in the FURB152 fixture is now unsafe.