Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ruff] Parenthesize arguments to int when removing int would change semantics in unnecessary-cast-to-int (RUF046) #15277

Merged
merged 10 commits into from
Jan 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF046.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,49 @@ async def f():

int(round(1,
0))

# function calls may need to retain parentheses
# if the parentheses for the call itself
# lie on the next line.
# See https://github.com/astral-sh/ruff/issues/15263
int(round
(1))

int(round # a comment
# and another comment
(10)
)

int(round (17)) # this is safe without parens

int( round (
17
)) # this is also safe without parens
dylwil3 marked this conversation as resolved.
Show resolved Hide resolved

int((round) # Comment
(42)
)

int((round # Comment
)(42)
)

int( # Unsafe fix because of this comment
( # Comment
(round
) # Comment
)(42)
)

int(
round(
42
) # unsafe fix because of this comment
)

int(
round(
42
)
# unsafe fix because of this comment
)
71 changes: 64 additions & 7 deletions crates/ruff_linter/src/rules/ruff/rules/unnecessary_cast_to_int.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use ruff_python_ast::parenthesize::parenthesized_range;
use ruff_python_ast::{Arguments, Expr, ExprCall};
use ruff_python_semantic::analyze::type_inference::{NumberLike, PythonType, ResolvedPythonType};
use ruff_python_semantic::SemanticModel;
use ruff_python_trivia::CommentRanges;
use ruff_python_trivia::{lines_after_ignoring_trivia, CommentRanges};
use ruff_source_file::LineRanges;
use ruff_text_size::Ranged;
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
use crate::rules::ruff::rules::unnecessary_round::{
Expand Down Expand Up @@ -114,12 +114,36 @@ fn unwrap_int_expression(
let parenthesize = semantic.current_expression_parent().is_some()
|| argument.is_named_expr()
|| locator.count_lines(argument.range()) > 0;
if parenthesize && !has_own_parentheses(argument) {
if parenthesize && !has_own_parentheses(argument, comment_ranges, source) {
format!("({})", locator.slice(argument.range()))
} else {
locator.slice(argument.range()).to_string()
}
};

// Since we're deleting the complement of the argument range within
// the call range, we have to check both ends for comments.
//
// For example:
// ```python
// int( # comment
// round(
// 42.1
// ) # comment
// )
// ```
let applicability = {
let call_to_arg_start = TextRange::new(call.start(), argument.start());
let arg_to_call_end = TextRange::new(argument.end(), call.end());
if comment_ranges.intersects(call_to_arg_start)
|| comment_ranges.intersects(arg_to_call_end)
{
Applicability::Unsafe
} else {
applicability
}
};

let edit = Edit::range_replacement(content, call.range());
Fix::applicable_edit(edit, applicability)
}
Expand Down Expand Up @@ -229,16 +253,49 @@ fn round_applicability(arguments: &Arguments, semantic: &SemanticModel) -> Optio
}

/// Returns `true` if the given [`Expr`] has its own parentheses (e.g., `()`, `[]`, `{}`).
fn has_own_parentheses(expr: &Expr) -> bool {
fn has_own_parentheses(expr: &Expr, comment_ranges: &CommentRanges, source: &str) -> bool {
match expr {
Expr::ListComp(_)
| Expr::SetComp(_)
| Expr::DictComp(_)
| Expr::Subscript(_)
| Expr::List(_)
| Expr::Set(_)
| Expr::Dict(_)
| Expr::Call(_) => true,
| Expr::Dict(_) => true,
Expr::Call(call_expr) => {
// A call where the function and parenthesized
// argument(s) appear on separate lines
// requires outer parentheses. That is:
// ```
// (f
// (10))
// ```
// is different than
// ```
// f
// (10)
// ```
let func_end = parenthesized_range(
call_expr.func.as_ref().into(),
call_expr.into(),
comment_ranges,
source,
)
.unwrap_or(call_expr.func.range())
.end();
lines_after_ignoring_trivia(func_end, source) == 0
}
Expr::Subscript(subscript_expr) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add couple of test cases for subscript expressions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, type inference is not robust enough to ever apply this rule to a subscript. So I can't write a test that would trigger this branch. On the other hand, I think I should leave this match arm in case type inference is later improved to cover this case.

// Same as above
let subscript_end = parenthesized_range(
subscript_expr.value.as_ref().into(),
subscript_expr.into(),
comment_ranges,
source,
)
.unwrap_or(subscript_expr.value.range())
.end();
lines_after_ignoring_trivia(subscript_end, source) == 0
}
Expr::Generator(generator) => generator.parenthesized,
Expr::Tuple(tuple) => tuple.parenthesized,
_ => false,
Expand Down
Loading
Loading