[refurb] FURB164 fix should validate arguments and should usually be marked unsafe
#19136
+338
−56
Conversation
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
Contributor
|
ntBre
added
bug
ntBre
reviewed
Jul 14, 2025
Contributor
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks, this looks good to me. I just had some suggestions for simplifying the code a bit.
crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Outdated
Show resolved
Hide resolved
| } | ||
| MethodName::FromDecimal => { | ||
| // from_decimal(dec) - should have exactly one positional argument or 'dec' keyword | ||
| if call.arguments.args.len() == 1 && call.arguments.keywords.is_empty() { |
Contributor
There was a problem hiding this comment.
Same as above, if it works above.
crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Outdated
Show resolved
Hide resolved
Comment on lines
338
to
340
| // Must be a call to the `float` builtin. | ||
| if !checker.semantic().match_builtin_expr(func, "float") { | ||
| return None; |
Contributor
There was a problem hiding this comment.
nit: I'd probably put this right after the let-else checking that it's a Call. I'm not sure which of these checks is most expensive, but that makes sense logically at least.
ntBre
approved these changes
Jul 16, 2025
Contributor
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! Just a few more slight simplifications.
crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs
Outdated
Show resolved
Hide resolved
…at.rs Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
…at.rs Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
…at.rs Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
…at.rs Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
…at.rs Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
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
Fixes #19076
An attempt at fixing #19076 where the rule could change program behavior by incorrectly converting from_float/from_decimal method calls to constructor calls.
The fix implements argument validation using Ruff's existing type inference system (
ResolvedPythonType,typing::is_int,typing::is_float) to determine when conversions are actually safe, adds logic to detect invalid method calls (wrong argument counts, incorrect keyword names) and suppress fixes for them, and changes the default fix applicability fromSafetoUnsafewith safe fixes only offered when the argument type is known to be compatible and no problematic keywords are used.One uncertainty is whether the type inference catches all possible edge cases in complex codebases, but the new approach is significantly more conservative and safer than the previous implementation.
Test Plan
I updated the existing test fixtures with edge cases from the issue and manually verified behavior with temporary test files for valid/unsafe/invalid scenarios.