From 291699b375f694c659fe82bdd68b149bde2494ee Mon Sep 17 00:00:00 2001 From: Dan Parizher <105245560+danparizher@users.noreply.github.com> Date: Wed, 16 Jul 2025 11:38:33 -0400 Subject: [PATCH] [`refurb`] `FURB164` fix should validate arguments and should usually be marked unsafe (#19136) ## 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 from `Safe` to `Unsafe` with 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. --------- Co-authored-by: Brent Westbrook --- .../resources/test/fixtures/refurb/FURB164.py | 19 +- .../refurb/rules/unnecessary_from_float.rs | 238 ++++++++++++++---- ...es__refurb__tests__FURB164_FURB164.py.snap | 145 ++++++++++- 3 files changed, 342 insertions(+), 60 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB164.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB164.py index e83f8578b6..745c1bf904 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB164.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB164.py @@ -27,7 +27,24 @@ _ = Decimal.from_float(float("  -inF\n \t")) _ = Decimal.from_float(float(" InfinIty \n\t ")) _ = Decimal.from_float(float("  -InfinIty\n \t")) -# OK +# Cases with keyword arguments - should produce unsafe fixes +_ = Fraction.from_decimal(dec=Decimal("4.2")) +_ = Decimal.from_float(f=4.2) + +# Cases with invalid argument counts - should not get fixes +_ = Fraction.from_decimal(Decimal("4.2"), 1) +_ = Decimal.from_float(4.2, None) + +# Cases with wrong keyword arguments - should not get fixes +_ = Fraction.from_decimal(numerator=Decimal("4.2")) +_ = Decimal.from_float(value=4.2) + +# Cases with type validation issues - should produce unsafe fixes +_ = Decimal.from_float("4.2") # Invalid type for from_float +_ = Fraction.from_decimal(4.2) # Invalid type for from_decimal +_ = Fraction.from_float("4.2") # Invalid type for from_float + +# OK - should not trigger the rule _ = Fraction(0.1) _ = Fraction(-0.5) _ = Fraction(5.0) diff --git a/crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs b/crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs index 897a8bbc72..fc2e525a01 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/unnecessary_from_float.rs @@ -1,10 +1,12 @@ use ruff_macros::{ViolationMetadata, derive_message_formats}; use ruff_python_ast::{self as ast, Expr, ExprCall}; +use ruff_python_semantic::analyze::type_inference::{NumberLike, PythonType, ResolvedPythonType}; +use ruff_python_semantic::analyze::typing; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::linter::float::as_non_finite_float_string_literal; -use crate::{Edit, Fix, FixAvailability, Violation}; +use crate::{Applicability, Edit, Fix, FixAvailability, Violation}; /// ## What it does /// Checks for unnecessary `from_float` and `from_decimal` usages to construct @@ -16,6 +18,12 @@ use crate::{Edit, Fix, FixAvailability, Violation}; /// the use of `from_float` and `from_decimal` methods is unnecessary, and /// should be avoided in favor of the more concise constructor syntax. /// +/// However, there are important behavioral differences between the `from_*` methods +/// and the constructors: +/// - The `from_*` methods validate their argument types and raise `TypeError` for invalid types +/// - The constructors accept broader argument types without validation +/// - The `from_*` methods have different parameter names than the constructors +/// /// ## Example /// ```python /// Decimal.from_float(4.2) @@ -32,6 +40,16 @@ use crate::{Edit, Fix, FixAvailability, Violation}; /// Fraction(Decimal(4.2)) /// ``` /// +/// ## Fix safety +/// This rule's fix is marked as unsafe by default because: +/// - The `from_*` methods provide type validation that the constructors don't +/// - Removing type validation can change program behavior +/// - The parameter names are different between methods and constructors +/// +/// The fix is marked as safe only when: +/// - The argument type is known to be valid for the target constructor +/// - No keyword arguments are used, or they match the constructor's parameters +/// /// ## References /// - [Python documentation: `decimal`](https://docs.python.org/3/library/decimal.html) /// - [Python documentation: `fractions`](https://docs.python.org/3/library/fractions.html) @@ -101,62 +119,178 @@ pub(crate) fn unnecessary_from_float(checker: &Checker, call: &ExprCall) { call.range(), ); - let edit = Edit::range_replacement( - checker.locator().slice(&**value).to_string(), - call.func.range(), - ); + // Validate that the method call has correct arguments and get the argument value + let Some(arg_value) = has_valid_method_arguments(call, method_name, constructor) else { + // Don't suggest a fix for invalid calls + return; + }; - // Short-circuit case for special values, such as: `Decimal.from_float(float("inf"))` to `Decimal("inf")`. - 'short_circuit: { - if !matches!(constructor, Constructor::Decimal) { - break 'short_circuit; - } - if !(method_name == MethodName::FromFloat) { - break 'short_circuit; - } - - let Some(value) = (match method_name { - MethodName::FromFloat => call.arguments.find_argument_value("f", 0), - MethodName::FromDecimal => call.arguments.find_argument_value("dec", 0), - }) else { - return; - }; - - let Expr::Call( - call @ ast::ExprCall { - func, arguments, .. - }, - ) = value - else { - break 'short_circuit; - }; - - // Must have exactly one argument, which is a string literal. - if !arguments.keywords.is_empty() { - break 'short_circuit; - } - let [float] = arguments.args.as_ref() else { - break 'short_circuit; - }; - if as_non_finite_float_string_literal(float).is_none() { - break 'short_circuit; - } - - // Must be a call to the `float` builtin. - if !semantic.match_builtin_expr(func, "float") { - break 'short_circuit; - } - - let replacement = checker.locator().slice(float).to_string(); - diagnostic.set_fix(Fix::safe_edits( - edit, - [Edit::range_replacement(replacement, call.range())], - )); + let constructor_name = checker.locator().slice(&**value).to_string(); + // Special case for non-finite float literals: Decimal.from_float(float("inf")) -> Decimal("inf") + if let Some(replacement) = handle_non_finite_float_special_case( + call, + method_name, + constructor, + arg_value, + &constructor_name, + checker, + ) { + diagnostic.set_fix(Fix::safe_edit(replacement)); return; } - diagnostic.set_fix(Fix::safe_edit(edit)); + // Check if we should suppress the fix due to type validation concerns + let is_type_safe = is_valid_argument_type(arg_value, method_name, constructor, checker); + let has_keywords = !call.arguments.keywords.is_empty(); + + // Determine fix safety + let applicability = if is_type_safe && !has_keywords { + Applicability::Safe + } else { + Applicability::Unsafe + }; + + // Build the replacement + let arg_text = checker.locator().slice(arg_value); + let replacement_text = format!("{constructor_name}({arg_text})"); + + let edit = Edit::range_replacement(replacement_text, call.range()); + + diagnostic.set_fix(Fix::applicable_edit(edit, applicability)); +} + +/// Check if the argument would be valid for the target constructor +fn is_valid_argument_type( + arg_expr: &Expr, + method_name: MethodName, + constructor: Constructor, + checker: &Checker, +) -> bool { + let semantic = checker.semantic(); + let resolved_type = ResolvedPythonType::from(arg_expr); + + let (is_int, is_float) = if let ResolvedPythonType::Unknown = resolved_type { + arg_expr + .as_name_expr() + .and_then(|name| semantic.only_binding(name).map(|id| semantic.binding(id))) + .map(|binding| { + ( + typing::is_int(binding, semantic), + typing::is_float(binding, semantic), + ) + }) + .unwrap_or_default() + } else { + (false, false) + }; + + match (method_name, constructor) { + // Decimal.from_float accepts int, bool, float + (MethodName::FromFloat, Constructor::Decimal) => match resolved_type { + ResolvedPythonType::Atom(PythonType::Number( + NumberLike::Integer | NumberLike::Bool | NumberLike::Float, + )) => true, + ResolvedPythonType::Unknown => is_int || is_float, + _ => false, + }, + // Fraction.from_float accepts int, bool, float + (MethodName::FromFloat, Constructor::Fraction) => match resolved_type { + ResolvedPythonType::Atom(PythonType::Number( + NumberLike::Integer | NumberLike::Bool | NumberLike::Float, + )) => true, + ResolvedPythonType::Unknown => is_int || is_float, + _ => false, + }, + // Fraction.from_decimal accepts int, bool, Decimal + (MethodName::FromDecimal, Constructor::Fraction) => match resolved_type { + ResolvedPythonType::Atom(PythonType::Number( + NumberLike::Integer | NumberLike::Bool, + )) => true, + ResolvedPythonType::Unknown => is_int, + _ => { + // Check if it's a Decimal instance + arg_expr + .as_call_expr() + .and_then(|call| semantic.resolve_qualified_name(&call.func)) + .is_some_and(|qualified_name| { + matches!(qualified_name.segments(), ["decimal", "Decimal"]) + }) + } + }, + _ => false, + } +} + +/// Check if the call has valid arguments for the from_* method +fn has_valid_method_arguments( + call: &ExprCall, + method_name: MethodName, + constructor: Constructor, +) -> Option<&Expr> { + if call.arguments.len() != 1 { + return None; + } + + match method_name { + MethodName::FromFloat => { + // Decimal.from_float is positional-only; Fraction.from_float allows keyword 'f'. + if constructor == Constructor::Decimal { + // Only allow positional argument for Decimal.from_float + call.arguments.find_positional(0) + } else { + // Fraction.from_float allows either positional or 'f' keyword + call.arguments.find_argument_value("f", 0) + } + } + MethodName::FromDecimal => { + // from_decimal(dec) - should have exactly one positional argument or 'dec' keyword + call.arguments.find_argument_value("dec", 0) + } + } +} + +/// Handle the special case for non-finite float literals +fn handle_non_finite_float_special_case( + call: &ExprCall, + method_name: MethodName, + constructor: Constructor, + arg_value: &Expr, + constructor_name: &str, + checker: &Checker, +) -> Option { + // Only applies to Decimal.from_float + if !matches!( + (method_name, constructor), + (MethodName::FromFloat, Constructor::Decimal) + ) { + return None; + } + + let Expr::Call(ast::ExprCall { + func, arguments, .. + }) = arg_value + else { + return None; + }; + + // Must be a call to the `float` builtin. + if !checker.semantic().match_builtin_expr(func, "float") { + return None; + } + + // Must have exactly one argument, which is a string literal. + if !arguments.keywords.is_empty() { + return None; + } + let [float_arg] = arguments.args.as_ref() else { + return None; + }; + as_non_finite_float_string_literal(float_arg)?; + + let replacement_arg = checker.locator().slice(float_arg).to_string(); + let replacement_text = format!("{constructor_name}({replacement_arg})"); + Some(Edit::range_replacement(replacement_text, call.range())) } #[derive(Debug, Copy, Clone, PartialEq, Eq)] diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB164_FURB164.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB164_FURB164.py.snap index 5f72efdfcd..13f94c45f9 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB164_FURB164.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB164_FURB164.py.snap @@ -95,7 +95,7 @@ FURB164.py:11:5: FURB164 [*] Verbose method `from_decimal` in `Fraction` constru | = help: Replace with `Fraction` constructor -ℹ Safe fix +ℹ Unsafe fix 8 8 | _ = Fraction.from_float(-0.5) 9 9 | _ = Fraction.from_float(5.0) 10 10 | _ = fractions.Fraction.from_float(4.2) @@ -116,7 +116,7 @@ FURB164.py:12:5: FURB164 [*] Verbose method `from_decimal` in `Fraction` constru | = help: Replace with `Fraction` constructor -ℹ Safe fix +ℹ Unsafe fix 9 9 | _ = Fraction.from_float(5.0) 10 10 | _ = fractions.Fraction.from_float(4.2) 11 11 | _ = Fraction.from_decimal(Decimal("4.2")) @@ -137,7 +137,7 @@ FURB164.py:13:5: FURB164 [*] Verbose method `from_decimal` in `Fraction` constru | = help: Replace with `Fraction` constructor -ℹ Safe fix +ℹ Unsafe fix 10 10 | _ = fractions.Fraction.from_float(4.2) 11 11 | _ = Fraction.from_decimal(Decimal("4.2")) 12 12 | _ = Fraction.from_decimal(Decimal("-4.2")) @@ -459,7 +459,7 @@ FURB164.py:27:5: FURB164 [*] Verbose method `from_float` in `Decimal` constructi 27 |+_ = Decimal(" InfinIty \n\t ") 28 28 | _ = Decimal.from_float(float("  -InfinIty\n \t")) 29 29 | -30 30 | # OK +30 30 | # Cases with keyword arguments - should produce unsafe fixes FURB164.py:28:5: FURB164 [*] Verbose method `from_float` in `Decimal` construction | @@ -468,7 +468,7 @@ FURB164.py:28:5: FURB164 [*] Verbose method `from_float` in `Decimal` constructi 28 | _ = Decimal.from_float(float("  -InfinIty\n \t")) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB164 29 | -30 | # OK +30 | # Cases with keyword arguments - should produce unsafe fixes | = help: Replace with `Decimal` constructor @@ -479,5 +479,136 @@ FURB164.py:28:5: FURB164 [*] Verbose method `from_float` in `Decimal` constructi 28 |-_ = Decimal.from_float(float("  -InfinIty\n \t")) 28 |+_ = Decimal("  -InfinIty\n \t") 29 29 | -30 30 | # OK -31 31 | _ = Fraction(0.1) +30 30 | # Cases with keyword arguments - should produce unsafe fixes +31 31 | _ = Fraction.from_decimal(dec=Decimal("4.2")) + +FURB164.py:31:5: FURB164 [*] Verbose method `from_decimal` in `Fraction` construction + | +30 | # Cases with keyword arguments - should produce unsafe fixes +31 | _ = Fraction.from_decimal(dec=Decimal("4.2")) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB164 +32 | _ = Decimal.from_float(f=4.2) + | + = help: Replace with `Fraction` constructor + +ℹ Unsafe fix +28 28 | _ = Decimal.from_float(float("  -InfinIty\n \t")) +29 29 | +30 30 | # Cases with keyword arguments - should produce unsafe fixes +31 |-_ = Fraction.from_decimal(dec=Decimal("4.2")) + 31 |+_ = Fraction(Decimal("4.2")) +32 32 | _ = Decimal.from_float(f=4.2) +33 33 | +34 34 | # Cases with invalid argument counts - should not get fixes + +FURB164.py:32:5: FURB164 Verbose method `from_float` in `Decimal` construction + | +30 | # Cases with keyword arguments - should produce unsafe fixes +31 | _ = Fraction.from_decimal(dec=Decimal("4.2")) +32 | _ = Decimal.from_float(f=4.2) + | ^^^^^^^^^^^^^^^^^^^^^^^^^ FURB164 +33 | +34 | # Cases with invalid argument counts - should not get fixes + | + = help: Replace with `Decimal` constructor + +FURB164.py:35:5: FURB164 Verbose method `from_decimal` in `Fraction` construction + | +34 | # Cases with invalid argument counts - should not get fixes +35 | _ = Fraction.from_decimal(Decimal("4.2"), 1) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB164 +36 | _ = Decimal.from_float(4.2, None) + | + = help: Replace with `Fraction` constructor + +FURB164.py:36:5: FURB164 Verbose method `from_float` in `Decimal` construction + | +34 | # Cases with invalid argument counts - should not get fixes +35 | _ = Fraction.from_decimal(Decimal("4.2"), 1) +36 | _ = Decimal.from_float(4.2, None) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB164 +37 | +38 | # Cases with wrong keyword arguments - should not get fixes + | + = help: Replace with `Decimal` constructor + +FURB164.py:39:5: FURB164 Verbose method `from_decimal` in `Fraction` construction + | +38 | # Cases with wrong keyword arguments - should not get fixes +39 | _ = Fraction.from_decimal(numerator=Decimal("4.2")) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB164 +40 | _ = Decimal.from_float(value=4.2) + | + = help: Replace with `Fraction` constructor + +FURB164.py:40:5: FURB164 Verbose method `from_float` in `Decimal` construction + | +38 | # Cases with wrong keyword arguments - should not get fixes +39 | _ = Fraction.from_decimal(numerator=Decimal("4.2")) +40 | _ = Decimal.from_float(value=4.2) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB164 +41 | +42 | # Cases with type validation issues - should produce unsafe fixes + | + = help: Replace with `Decimal` constructor + +FURB164.py:43:5: FURB164 [*] Verbose method `from_float` in `Decimal` construction + | +42 | # Cases with type validation issues - should produce unsafe fixes +43 | _ = Decimal.from_float("4.2") # Invalid type for from_float + | ^^^^^^^^^^^^^^^^^^^^^^^^^ FURB164 +44 | _ = Fraction.from_decimal(4.2) # Invalid type for from_decimal +45 | _ = Fraction.from_float("4.2") # Invalid type for from_float + | + = help: Replace with `Decimal` constructor + +ℹ Unsafe fix +40 40 | _ = Decimal.from_float(value=4.2) +41 41 | +42 42 | # Cases with type validation issues - should produce unsafe fixes +43 |-_ = Decimal.from_float("4.2") # Invalid type for from_float + 43 |+_ = Decimal("4.2") # Invalid type for from_float +44 44 | _ = Fraction.from_decimal(4.2) # Invalid type for from_decimal +45 45 | _ = Fraction.from_float("4.2") # Invalid type for from_float +46 46 | + +FURB164.py:44:5: FURB164 [*] Verbose method `from_decimal` in `Fraction` construction + | +42 | # Cases with type validation issues - should produce unsafe fixes +43 | _ = Decimal.from_float("4.2") # Invalid type for from_float +44 | _ = Fraction.from_decimal(4.2) # Invalid type for from_decimal + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB164 +45 | _ = Fraction.from_float("4.2") # Invalid type for from_float + | + = help: Replace with `Fraction` constructor + +ℹ Unsafe fix +41 41 | +42 42 | # Cases with type validation issues - should produce unsafe fixes +43 43 | _ = Decimal.from_float("4.2") # Invalid type for from_float +44 |-_ = Fraction.from_decimal(4.2) # Invalid type for from_decimal + 44 |+_ = Fraction(4.2) # Invalid type for from_decimal +45 45 | _ = Fraction.from_float("4.2") # Invalid type for from_float +46 46 | +47 47 | # OK - should not trigger the rule + +FURB164.py:45:5: FURB164 [*] Verbose method `from_float` in `Fraction` construction + | +43 | _ = Decimal.from_float("4.2") # Invalid type for from_float +44 | _ = Fraction.from_decimal(4.2) # Invalid type for from_decimal +45 | _ = Fraction.from_float("4.2") # Invalid type for from_float + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB164 +46 | +47 | # OK - should not trigger the rule + | + = help: Replace with `Fraction` constructor + +ℹ Unsafe fix +42 42 | # Cases with type validation issues - should produce unsafe fixes +43 43 | _ = Decimal.from_float("4.2") # Invalid type for from_float +44 44 | _ = Fraction.from_decimal(4.2) # Invalid type for from_decimal +45 |-_ = Fraction.from_float("4.2") # Invalid type for from_float + 45 |+_ = Fraction("4.2") # Invalid type for from_float +46 46 | +47 47 | # OK - should not trigger the rule +48 48 | _ = Fraction(0.1)