Skip to content

Commit

Permalink
[ruff] Unnecessary cast to int (RUF046) (#14697)
Browse files Browse the repository at this point in the history
## Summary

Resolves #11412.

## Test Plan

`cargo nextest run` and `cargo insta test`.

---------

Co-authored-by: Micha Reiser <[email protected]>
  • Loading branch information
InSyncWithFoo and MichaReiser authored Dec 5, 2024
1 parent 2d3f557 commit fda8b1f
Show file tree
Hide file tree
Showing 9 changed files with 672 additions and 1 deletion.
50 changes: 50 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF046.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import math


### Safely fixable

# Arguments are not checked
int(id())
int(len([]))
int(ord(foo))
int(hash(foo, bar))
int(int(''))

int(math.comb())
int(math.factorial())
int(math.gcd())
int(math.lcm())
int(math.isqrt())
int(math.perm())


### Unsafe

int(math.ceil())
int(math.floor())
int(math.trunc())


### `round()`

## Errors
int(round(0))
int(round(0, 0))
int(round(0, None))

int(round(0.1))
int(round(0.1, 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(0, 3.14))
int(round(0, non_literal))
int(round(0, 0), base)
int(round(0, 0, extra=keyword))
int(round(0.1, 0))
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1093,6 +1093,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::Airflow3Removal) {
airflow::rules::removed_in_3(checker, expr);
}
if checker.enabled(Rule::UnnecessaryCastToInt) {
ruff::rules::unnecessary_cast_to_int(checker, call);
}
}
Expr::Dict(dict) => {
if checker.any_enabled(&[
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "039") => (RuleGroup::Preview, rules::ruff::rules::UnrawRePattern),
(Ruff, "040") => (RuleGroup::Preview, rules::ruff::rules::InvalidAssertMessageLiteralArgument),
(Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral),
(Ruff, "046") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryCastToInt),
(Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing),
(Ruff, "052") => (RuleGroup::Preview, rules::ruff::rules::UsedDummyVariable),
(Ruff, "055") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryRegularExpression),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,7 @@ mod tests {
#[test_case(Rule::UnrawRePattern, Path::new("RUF039_concat.py"))]
#[test_case(Rule::UnnecessaryRegularExpression, Path::new("RUF055_0.py"))]
#[test_case(Rule::UnnecessaryRegularExpression, Path::new("RUF055_1.py"))]
#[test_case(Rule::UnnecessaryCastToInt, Path::new("RUF046.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub(crate) use sort_dunder_slots::*;
pub(crate) use static_key_dict_comprehension::*;
#[cfg(any(feature = "test-rules", test))]
pub(crate) use test_rules::*;
pub(crate) use unnecessary_cast_to_int::*;
pub(crate) use unnecessary_iterable_allocation_for_first_element::*;
pub(crate) use unnecessary_key_check::*;
pub(crate) use unnecessary_nested_literal::*;
Expand Down Expand Up @@ -78,6 +79,7 @@ mod static_key_dict_comprehension;
mod suppression_comment_visitor;
#[cfg(any(feature = "test-rules", test))]
pub(crate) mod test_rules;
mod unnecessary_cast_to_int;
mod unnecessary_iterable_allocation_for_first_element;
mod unnecessary_key_check;
mod unnecessary_nested_literal;
Expand Down
183 changes: 183 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/unnecessary_cast_to_int.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
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_semantic::analyze::typing;
use ruff_python_semantic::SemanticModel;
use ruff_text_size::TextRange;

/// ## What it does
/// Checks for `int` conversions of values that are already integers.
///
/// ## Why is this bad?
/// Such a conversion is unnecessary.
///
/// ## Known problems
/// This rule may produce false positives for `round`, `math.ceil`, `math.floor`,
/// and `math.trunc` calls when values override the `__round__`, `__ceil__`, `__floor__`,
/// or `__trunc__` operators such that they don't return an integer.
///
/// ## Example
///
/// ```python
/// int(len([]))
/// int(round(foo, None))
/// ```
///
/// Use instead:
///
/// ```python
/// len([])
/// round(foo)
/// ```
///
/// ## Fix safety
/// The fix for `round`, `math.ceil`, `math.floor`, and `math.truncate` is unsafe
/// because removing the `int` conversion can change the semantics for values
/// overriding the `__round__`, `__ceil__`, `__floor__`, or `__trunc__` dunder methods
/// such that they don't return an integer.
#[derive(ViolationMetadata)]
pub(crate) struct UnnecessaryCastToInt;

impl AlwaysFixableViolation for UnnecessaryCastToInt {
#[derive_message_formats]
fn message(&self) -> String {
"Value being casted is already an integer".to_string()
}

fn fix_title(&self) -> String {
"Remove unnecessary conversion to `int`".to_string()
}
}

/// RUF046
pub(crate) fn unnecessary_cast_to_int(checker: &mut Checker, call: &ExprCall) {
let semantic = checker.semantic();

let Some(Expr::Call(inner_call)) = single_argument_to_int_call(semantic, call) else {
return;
};

let (func, arguments) = (&inner_call.func, &inner_call.arguments);
let (outer_range, inner_range) = (call.range, inner_call.range);

let Some(qualified_name) = checker.semantic().resolve_qualified_name(func) else {
return;
};

let fix = match qualified_name.segments() {
// Always returns a strict instance of `int`
["" | "builtins", "len" | "id" | "hash" | "ord" | "int"]
| ["math", "comb" | "factorial" | "gcd" | "lcm" | "isqrt" | "perm"] => {
Fix::safe_edit(replace_with_inner(checker, outer_range, inner_range))
}

// Depends on `ndigits` and `number.__round__`
["" | "builtins", "round"] => {
if let Some(fix) = replace_with_shortened_round_call(checker, outer_range, arguments) {
fix
} else {
return;
}
}

// Depends on `__ceil__`/`__floor__`/`__trunc__`
["math", "ceil" | "floor" | "trunc"] => {
Fix::unsafe_edit(replace_with_inner(checker, outer_range, inner_range))
}

_ => return,
};

checker
.diagnostics
.push(Diagnostic::new(UnnecessaryCastToInt, call.range).with_fix(fix));
}

fn single_argument_to_int_call<'a>(
semantic: &SemanticModel,
call: &'a ExprCall,
) -> Option<&'a Expr> {
let ExprCall {
func, arguments, ..
} = call;

if !semantic.match_builtin_expr(func, "int") {
return None;
}

if !arguments.keywords.is_empty() {
return None;
}

let [argument] = &*arguments.args else {
return None;
};

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(
checker: &Checker,
outer_range: TextRange,
arguments: &Arguments,
) -> Option<Fix> {
if arguments.len() > 2 {
return None;
}

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,
};

match ndigits {
Some(Expr::NumberLiteral(ExprNumberLiteral { value, .. }))
if is_literal_zero(value) && number_is_int => {}
Some(Expr::NoneLiteral(_)) | None => {}
_ => return None,
};

let number_expr = checker.locator().slice(number);
let new_content = format!("round({number_expr})");

let applicability = if number_is_int {
Applicability::Safe
} else {
Applicability::Unsafe
};

Some(Fix::applicable_edit(
Edit::range_replacement(new_content, outer_range),
applicability,
))
}

fn is_int(semantic: &SemanticModel, name: &ExprName) -> bool {
let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else {
return false;
};

typing::is_int(binding, semantic)
}

fn is_literal_zero(value: &Number) -> bool {
let Number::Int(int) = value else {
return false;
};

matches!(int.as_u8(), Some(0))
}

fn replace_with_inner(checker: &Checker, outer_range: TextRange, inner_range: TextRange) -> Edit {
let inner_expr = checker.locator().slice(inner_range);

Edit::range_replacement(inner_expr.to_string(), outer_range)
}
Loading

0 comments on commit fda8b1f

Please sign in to comment.