-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Expand expressions to include parentheses in E712 (#6575)
## Summary This PR exposes our `is_expression_parenthesized` logic such that we can use it to expand expressions when autofixing to include their parenthesized ranges. This solution has a few drawbacks: (1) we need to compute parenthesized ranges in more places, which also relies on backwards lexing; and (2) we need to make use of this in any relevant fixes. However, I still think it's worth pursuing. On (1), the implementation is very contained, so IMO we can easily swap this out for a more performant solution in the future if needed. On (2), this improves correctness and fixes some bad syntax errors detected by fuzzing, which means it has value even if it's not as robust as an _actual_ `ParenthesizedExpression` node in the AST itself. Closes #4925. ## Test Plan `cargo test` with new cases that previously failed the fuzzer.
- Loading branch information
1 parent
db1c556
commit 1050142
Showing
9 changed files
with
208 additions
and
14 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; | ||
| use ruff_text_size::{TextRange, TextSize}; | ||
|
|
||
| use crate::node::AnyNodeRef; | ||
| use crate::{ExpressionRef, Ranged}; | ||
|
|
||
| /// Returns the [`TextRange`] of a given expression including parentheses, if the expression is | ||
| /// parenthesized; or `None`, if the expression is not parenthesized. | ||
| pub fn parenthesized_range( | ||
| expr: ExpressionRef, | ||
| parent: AnyNodeRef, | ||
| contents: &str, | ||
| ) -> Option<TextRange> { | ||
| // If the parent is a node that brings its own parentheses, exclude the closing parenthesis | ||
| // from our search range. Otherwise, we risk matching on calls, like `func(x)`, for which | ||
| // the open and close parentheses are part of the `Arguments` node. | ||
| // | ||
| // There are a few other nodes that may have their own parentheses, but are fine to exclude: | ||
| // - `Parameters`: The parameters to a function definition. Any expressions would represent | ||
| // default arguments, and so must be preceded by _at least_ the parameter name. As such, | ||
| // we won't mistake any parentheses for the opening and closing parentheses on the | ||
| // `Parameters` node itself. | ||
| // - `Tuple`: The elements of a tuple. The only risk is a single-element tuple (e.g., `(x,)`), | ||
| // which must have a trailing comma anyway. | ||
| let exclusive_parent_end = if parent.is_arguments() { | ||
| parent.end() - TextSize::new(1) | ||
| } else { | ||
| parent.end() | ||
| }; | ||
|
|
||
| // First, test if there's a closing parenthesis because it tends to be cheaper. | ||
| let tokenizer = | ||
| SimpleTokenizer::new(contents, TextRange::new(expr.end(), exclusive_parent_end)); | ||
| let right = tokenizer.skip_trivia().next()?; | ||
|
|
||
| if right.kind == SimpleTokenKind::RParen { | ||
| // Next, test for the opening parenthesis. | ||
| let mut tokenizer = | ||
| SimpleTokenizer::up_to_without_back_comment(expr.start(), contents).skip_trivia(); | ||
| let left = tokenizer.next_back()?; | ||
| if left.kind == SimpleTokenKind::LParen { | ||
| return Some(TextRange::new(left.start(), right.end())); | ||
| } | ||
| } | ||
|
|
||
| None | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| use ruff_python_ast::parenthesize::parenthesized_range; | ||
| use ruff_python_parser::parse_expression; | ||
|
|
||
| #[test] | ||
| fn test_parenthesized_name() { | ||
| let source_code = r#"(x) + 1"#; | ||
| let expr = parse_expression(source_code, "<filename>").unwrap(); | ||
|
|
||
| let bin_op = expr.as_bin_op_expr().unwrap(); | ||
| let name = bin_op.left.as_ref(); | ||
|
|
||
| let parenthesized = parenthesized_range(name.into(), bin_op.into(), source_code); | ||
| assert!(parenthesized.is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_non_parenthesized_name() { | ||
| let source_code = r#"x + 1"#; | ||
| let expr = parse_expression(source_code, "<filename>").unwrap(); | ||
|
|
||
| let bin_op = expr.as_bin_op_expr().unwrap(); | ||
| let name = bin_op.left.as_ref(); | ||
|
|
||
| let parenthesized = parenthesized_range(name.into(), bin_op.into(), source_code); | ||
| assert!(parenthesized.is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parenthesized_argument() { | ||
| let source_code = r#"f((a))"#; | ||
| let expr = parse_expression(source_code, "<filename>").unwrap(); | ||
|
|
||
| let call = expr.as_call_expr().unwrap(); | ||
| let arguments = &call.arguments; | ||
| let argument = arguments.args.first().unwrap(); | ||
|
|
||
| let parenthesized = parenthesized_range(argument.into(), arguments.into(), source_code); | ||
| assert!(parenthesized.is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_non_parenthesized_argument() { | ||
| let source_code = r#"f(a)"#; | ||
| let expr = parse_expression(source_code, "<filename>").unwrap(); | ||
|
|
||
| let call = expr.as_call_expr().unwrap(); | ||
| let arguments = &call.arguments; | ||
| let argument = arguments.args.first().unwrap(); | ||
|
|
||
| let parenthesized = parenthesized_range(argument.into(), arguments.into(), source_code); | ||
| assert!(parenthesized.is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parenthesized_tuple_member() { | ||
| let source_code = r#"(a, (b))"#; | ||
| let expr = parse_expression(source_code, "<filename>").unwrap(); | ||
|
|
||
| let tuple = expr.as_tuple_expr().unwrap(); | ||
| let member = tuple.elts.last().unwrap(); | ||
|
|
||
| let parenthesized = parenthesized_range(member.into(), tuple.into(), source_code); | ||
| assert!(parenthesized.is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_non_parenthesized_tuple_member() { | ||
| let source_code = r#"(a, b)"#; | ||
| let expr = parse_expression(source_code, "<filename>").unwrap(); | ||
|
|
||
| let tuple = expr.as_tuple_expr().unwrap(); | ||
| let member = tuple.elts.last().unwrap(); | ||
|
|
||
| let parenthesized = parenthesized_range(member.into(), tuple.into(), source_code); | ||
| assert!(parenthesized.is_none()); | ||
| } |