diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB164.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB164.py index e83f8578b6672..745c1bf90480c 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(" 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 897a8bbc72103..fc2e525a01e3c 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 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; + } + + // 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; + } - 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; + 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) + } } - let [float] = arguments.args.as_ref() else { - break 'short_circuit; - }; - if as_non_finite_float_string_literal(float).is_none() { - break 'short_circuit; + MethodName::FromDecimal => { + // from_decimal(dec) - should have exactly one positional argument or 'dec' keyword + call.arguments.find_argument_value("dec", 0) } + } +} - // Must be a call to the `float` builtin. - if !semantic.match_builtin_expr(func, "float") { - break 'short_circuit; - } +/// 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 replacement = checker.locator().slice(float).to_string(); - diagnostic.set_fix(Fix::safe_edits( - edit, - [Edit::range_replacement(replacement, call.range())], - )); + let Expr::Call(ast::ExprCall { + func, arguments, .. + }) = arg_value + else { + return None; + }; - return; + // Must be a call to the `float` builtin. + if !checker.semantic().match_builtin_expr(func, "float") { + return None; } - diagnostic.set_fix(Fix::safe_edit(edit)); + // 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 5f72efdfcd186..13f94c45f9fa9 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)