[ruff] Do not simplify round() calls (RUF046) (#14832)

## Summary Part 1 of the big change introduced in #14828. This temporarily causes all fixes for `round(...)` to be considered unsafe, but they will eventually be enhanced. ## Test Plan `cargo nextest run` and `cargo insta test`.
astral-sh · Dec 9, 2024 · c62ba48 · c62ba48
1 parent 68eb0a2
commit c62ba48
Show file tree

Hide file tree

Showing 4 changed files with 548 additions and 353 deletions.
diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF046.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF046.py
@@ -1,9 +1,12 @@
 import math
 
 
+inferred_int = 1
+inferred_float = 1.
+
+
 ### Safely fixable
 
-# Arguments are not checked
 int(id())
 int(len([]))
 int(ord(foo))
@@ -17,34 +20,51 @@
 int(math.isqrt())
 int(math.perm())
 
+int(round(1, 0))
+int(round(1, 10))
+
+int(round(1))
+int(round(1, None))
+
+int(round(1.))
+int(round(1., None))
+
 
 ### Unsafe
 
 int(math.ceil())
 int(math.floor())
 int(math.trunc())
 
+int(round(inferred_int, 0))
+int(round(inferred_int, 10))
 
-### `round()`
+int(round(inferred_int))
+int(round(inferred_int, None))
 
-## Errors
-int(round(0))
-int(round(0, 0))
-int(round(0, None))
+int(round(inferred_float))
+int(round(inferred_float, None))
 
-int(round(0.1))
-int(round(0.1, None))
+int(round(unknown))
+int(round(unknown, None))
 
-# Argument type is not checked
-foo = type("Foo", (), {"__round__": lambda self: 4.2})()
 
-int(round(foo))
-int(round(foo, 0))
-int(round(foo, None))
+### No errors
+
+int(round(1, unknown))
+int(round(1., unknown))
+
+int(round(1., 0))
+int(round(inferred_float, 0))
+
+int(round(inferred_int, unknown))
+int(round(inferred_float, unknown))
+
+int(round(unknown, 0))
+int(round(unknown, unknown))
 
-## No errors
 int(round(0, 3.14))
-int(round(0, non_literal))
+int(round(inferred_int, 3.14))
+
 int(round(0, 0), base)
 int(round(0, 0, extra=keyword))
-int(round(0.1, 0))
diff --git a/crates/ruff_linter/src/rules/ruff/rules/unnecessary_cast_to_int.rs b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_cast_to_int.rs
@@ -1,7 +1,7 @@
 use crate::checkers::ast::Checker;
 use ruff_diagnostics::{AlwaysFixableViolation, Applicability, Diagnostic, Edit, Fix};
 use ruff_macros::{derive_message_formats, ViolationMetadata};
-use ruff_python_ast::{Arguments, Expr, ExprCall, ExprName, ExprNumberLiteral, Number};
+use ruff_python_ast::{Arguments, Expr, ExprCall, ExprNumberLiteral, Number};
 use ruff_python_semantic::analyze::typing;
 use ruff_python_semantic::SemanticModel;
 use ruff_text_size::TextRange;
@@ -74,7 +74,7 @@ pub(crate) fn unnecessary_cast_to_int(checker: &mut Checker, call: &ExprCall) {
 
         // Depends on `ndigits` and `number.__round__`
         ["" | "builtins", "round"] => {
-            if let Some(fix) = replace_with_shortened_round_call(checker, outer_range, arguments) {
+            if let Some(fix) = replace_with_round(checker, outer_range, inner_range, arguments) {
                 fix
             } else {
                 return;
@@ -89,9 +89,9 @@ pub(crate) fn unnecessary_cast_to_int(checker: &mut Checker, call: &ExprCall) {
         _ => return,
     };
 
-    checker
-        .diagnostics
-        .push(Diagnostic::new(UnnecessaryCastToInt, call.range).with_fix(fix));
+    let diagnostic = Diagnostic::new(UnnecessaryCastToInt, call.range);
+
+    checker.diagnostics.push(diagnostic.with_fix(fix));
 }
 
 fn single_argument_to_int_call<'a>(
@@ -117,12 +117,29 @@ fn single_argument_to_int_call<'a>(
     Some(argument)
 }
 
-/// Returns an [`Edit`] when the call is of any of the forms:
-/// * `round(integer)`, `round(integer, 0)`, `round(integer, None)`
-/// * `round(whatever)`, `round(whatever, None)`
-fn replace_with_shortened_round_call(
+/// The type of the first argument to `round()`
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+enum Rounded {
+    InferredInt,
+    InferredFloat,
+    LiteralInt,
+    LiteralFloat,
+    Other,
+}
+
+/// The type of the second argument to `round()`
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+enum Ndigits {
+    NotGiven,
+    LiteralInt,
+    LiteralNone,
+    Other,
+}
+
+fn replace_with_round(
     checker: &Checker,
     outer_range: TextRange,
+    inner_range: TextRange,
     arguments: &Arguments,
 ) -> Option<Fix> {
     if arguments.len() > 2 {
@@ -132,48 +149,56 @@ fn replace_with_shortened_round_call(
     let number = arguments.find_argument("number", 0)?;
     let ndigits = arguments.find_argument("ndigits", 1);
 
-    let number_is_int = match number {
-        Expr::Name(name) => is_int(checker.semantic(), name),
-        Expr::NumberLiteral(ExprNumberLiteral { value, .. }) => matches!(value, Number::Int(..)),
-        _ => false,
-    };
+    let number_kind = match number {
+        Expr::Name(name) => {
+            let semantic = checker.semantic();
 
-    match ndigits {
-        Some(Expr::NumberLiteral(ExprNumberLiteral { value, .. }))
-            if is_literal_zero(value) && number_is_int => {}
-        Some(Expr::NoneLiteral(_)) | None => {}
-        _ => return None,
-    };
+            match semantic.only_binding(name).map(|id| semantic.binding(id)) {
+                Some(binding) if typing::is_int(binding, semantic) => Rounded::InferredInt,
+                Some(binding) if typing::is_float(binding, semantic) => Rounded::InferredFloat,
+                _ => Rounded::Other,
+            }
+        }
 
-    let number_expr = checker.locator().slice(number);
-    let new_content = format!("round({number_expr})");
+        Expr::NumberLiteral(ExprNumberLiteral { value, .. }) => match value {
+            Number::Int(..) => Rounded::LiteralInt,
+            Number::Float(..) => Rounded::LiteralFloat,
+            Number::Complex { .. } => Rounded::Other,
+        },
 
-    let applicability = if number_is_int {
-        Applicability::Safe
-    } else {
-        Applicability::Unsafe
+        _ => Rounded::Other,
     };
 
-    Some(Fix::applicable_edit(
-        Edit::range_replacement(new_content, outer_range),
-        applicability,
-    ))
-}
+    let ndigits_kind = match ndigits {
+        None => Ndigits::NotGiven,
+        Some(Expr::NoneLiteral(_)) => Ndigits::LiteralNone,
 
-fn is_int(semantic: &SemanticModel, name: &ExprName) -> bool {
-    let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else {
-        return false;
+        Some(Expr::NumberLiteral(ExprNumberLiteral {
+            value: Number::Int(..),
+            ..
+        })) => Ndigits::LiteralInt,
+
+        _ => Ndigits::Other,
     };
 
-    typing::is_int(binding, semantic)
-}
+    let applicability = match (number_kind, ndigits_kind) {
+        (Rounded::LiteralInt, Ndigits::LiteralInt)
+        | (Rounded::LiteralInt | Rounded::LiteralFloat, Ndigits::NotGiven | Ndigits::LiteralNone) => {
+            Applicability::Safe
+        }
+
+        (Rounded::InferredInt, Ndigits::LiteralInt)
+        | (
+            Rounded::InferredInt | Rounded::InferredFloat | Rounded::Other,
+            Ndigits::NotGiven | Ndigits::LiteralNone,
+        ) => Applicability::Unsafe,
 
-fn is_literal_zero(value: &Number) -> bool {
-    let Number::Int(int) = value else {
-        return false;
+        _ => return None,
     };
 
-    matches!(int.as_u8(), Some(0))
+    let edit = replace_with_inner(checker, outer_range, inner_range);
+
+    Some(Fix::applicable_edit(edit, applicability))
 }
 
 fn replace_with_inner(checker: &Checker, outer_range: TextRange, inner_range: TextRange) -> Edit {