Expand expressions to include parentheses in E712 #6575

charliermarsh · 2023-08-14T22:30:27Z

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.

github-actions · 2023-08-14T22:42:37Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.7±0.36ms    10.9 MB/sec    1.00      3.7±0.22ms    10.9 MB/sec
formatter/numpy/ctypeslib.py               1.01   759.7±51.26µs    21.9 MB/sec    1.00   749.3±54.89µs    22.2 MB/sec
formatter/numpy/globals.py                 1.00     74.5±6.51µs    39.6 MB/sec    1.09     81.2±6.26µs    36.3 MB/sec
formatter/pydantic/types.py                1.00  1466.0±91.30µs    17.4 MB/sec    1.05  1535.1±92.96µs    16.6 MB/sec
linter/all-rules/large/dataset.py          1.00     12.4±0.44ms     3.3 MB/sec    1.00     12.4±0.65ms     3.3 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.05      3.4±0.16ms     5.0 MB/sec    1.00      3.2±0.13ms     5.2 MB/sec
linter/all-rules/numpy/globals.py          1.03   486.3±32.16µs     6.1 MB/sec    1.00   472.0±22.25µs     6.3 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.5±0.31ms     3.9 MB/sec    1.01      6.6±0.23ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.00      6.4±0.26ms     6.3 MB/sec    1.03      6.7±0.38ms     6.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1407.0±77.20µs    11.8 MB/sec    1.02  1429.3±54.88µs    11.6 MB/sec
linter/default-rules/numpy/globals.py      1.00   173.9±10.42µs    17.0 MB/sec    1.02    178.1±8.45µs    16.6 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.9±0.14ms     8.7 MB/sec    1.01      3.0±0.14ms     8.6 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.7±0.05ms    10.9 MB/sec    1.01      3.7±0.09ms    10.9 MB/sec
formatter/numpy/ctypeslib.py               1.00   734.6±10.42µs    22.7 MB/sec    1.01    741.6±9.03µs    22.5 MB/sec
formatter/numpy/globals.py                 1.00     76.0±0.87µs    38.8 MB/sec    1.02     77.9±2.21µs    37.9 MB/sec
formatter/pydantic/types.py                1.00  1511.2±19.27µs    16.9 MB/sec    1.01  1524.3±19.95µs    16.7 MB/sec
linter/all-rules/large/dataset.py          1.00     13.0±0.12ms     3.1 MB/sec    1.01     13.1±0.18ms     3.1 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.6±0.02ms     4.6 MB/sec    1.02      3.7±0.05ms     4.5 MB/sec
linter/all-rules/numpy/globals.py          1.00    376.4±7.02µs     7.8 MB/sec    1.02    382.4±8.43µs     7.7 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.8±0.13ms     3.7 MB/sec    1.01      6.9±0.12ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      7.1±0.05ms     5.7 MB/sec    1.02      7.2±0.07ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1487.5±13.47µs    11.2 MB/sec    1.01  1505.4±20.22µs    11.1 MB/sec
linter/default-rules/numpy/globals.py      1.00    150.4±1.75µs    19.6 MB/sec    1.03    154.2±1.61µs    19.1 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.03ms     8.0 MB/sec    1.03      3.3±0.06ms     7.8 MB/sec

crates/ruff_python_ast/src/parenthesize.rs

MichaReiser · 2023-08-16T18:37:46Z

crates/ruff_python_ast/src/parenthesize.rs

+#[derive(Debug)]
+pub struct ParenthesizedExpression<'a> {
+    /// The underlying AST node.
+    expr: AnyNodeRef<'a>,


Is the expression read anywhere? Would be nice to have the ExpressionRef that I started introducing once.

No, only the range. Should I add ExpressionRef? (AnyExpressionRef?)

I guess AnyExpressionRef is redundant and ExpressionRef is more appropriate.

Dug up the old PR (#5644), I can reintroduce it separately.

charliermarsh · 2023-08-17T00:58:48Z

crates/ruff_python_ast/src/parenthesize.rs

+
+/// 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(


I got rid of the struct in favor of a simple function that returns Option. I've gone back and forth on it a little bit.

## Summary No behavior changes, but these need some refactoring to support #6575 (namely, they need to take the `ast::ExprCompare` or similar node instead of the attribute fields), and I don't want to muddy that PR. ## Test Plan `cargo test`

MichaReiser · 2023-08-17T05:40:58Z

crates/ruff_python_ast/src/parenthesize.rs

+    // from our search range. Otherwise, we risk matching on calls, like `func(x)`.
+    // We can exclude tuples here, since 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.is_parameters() {


parameters shouldn't be a problem (or we have one in the formatter) because the parameter isn't an expression, it's a Parameter node and the default expression is guaranteed to be preceded by a name, making it unambiguous.

MichaReiser · 2023-08-17T05:42:51Z

crates/ruff_python_ast/src/parenthesize.rs

+    // We can exclude tuples here, since 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.is_parameters() {
+        parent.end().sub(TextSize::new(1))


Suggested change

parent.end().sub(TextSize::new(1))

parent.end() - TextSize::new(1)

## Summary This PR revives the `ExpressionRef` concept introduced in #5644, motivated by the change we want to make in #6575 to narrow the type of the expression that can be passed to `parenthesized_range`. ## Test Plan `cargo test`

charliermarsh added the bug Something isn't working label Aug 14, 2023

charliermarsh force-pushed the charlie/invalid-parens branch from 9d0a620 to 291ae62 Compare August 14, 2023 23:21

charliermarsh requested a review from MichaReiser August 14, 2023 23:24

charliermarsh force-pushed the charlie/invalid-parens branch 2 times, most recently from 3d3bebe to 6ec59a5 Compare August 16, 2023 04:03

MichaReiser reviewed Aug 16, 2023

View reviewed changes

crates/ruff_python_ast/src/parenthesize.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Aug 16, 2023

View reviewed changes

charliermarsh force-pushed the charlie/invalid-parens branch from 6ec59a5 to bc164d6 Compare August 17, 2023 00:51

charliermarsh mentioned this pull request Aug 17, 2023

Refactor literal-comparison and not-test rules #6636

Merged

charliermarsh force-pushed the charlie/invalid-parens branch from bc164d6 to b1d0c5f Compare August 17, 2023 00:58

charliermarsh changed the base branch from main to charlie/rules August 17, 2023 00:58

charliermarsh requested a review from MichaReiser August 17, 2023 00:58

charliermarsh commented Aug 17, 2023

View reviewed changes

Base automatically changed from charlie/rules to main August 17, 2023 01:02

charliermarsh force-pushed the charlie/invalid-parens branch from b1d0c5f to 62a370c Compare August 17, 2023 01:09

charliermarsh mentioned this pull request Aug 17, 2023

Introduce ExpressionRef #6637

Merged

charliermarsh force-pushed the charlie/invalid-parens branch from 62a370c to 59b2baa Compare August 17, 2023 01:12

charliermarsh changed the base branch from main to charlie/any-expression-ref August 17, 2023 01:12

charliermarsh force-pushed the charlie/any-expression-ref branch from cf85e79 to a714572 Compare August 17, 2023 01:18

charliermarsh force-pushed the charlie/invalid-parens branch from 59b2baa to 04a6341 Compare August 17, 2023 01:18

MichaReiser approved these changes Aug 17, 2023

View reviewed changes

Base automatically changed from charlie/any-expression-ref to main August 17, 2023 14:07

Expand expressions to include parentheses in E712

658b550

charliermarsh force-pushed the charlie/invalid-parens branch from 04a6341 to feae628 Compare August 17, 2023 15:42

Remove struct

7ca7d7b

charliermarsh force-pushed the charlie/invalid-parens branch from feae628 to 7ca7d7b Compare August 17, 2023 15:43

charliermarsh enabled auto-merge (squash) August 17, 2023 15:43

charliermarsh merged commit 1050142 into main Aug 17, 2023
16 checks passed

charliermarsh deleted the charlie/invalid-parens branch August 17, 2023 15:51

chenrui333 mentioned this pull request Aug 17, 2023

ruff 0.0.285 Homebrew/homebrew-core#139823

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand expressions to include parentheses in E712 #6575

Expand expressions to include parentheses in E712 #6575

charliermarsh commented Aug 14, 2023 •

edited

Loading

github-actions bot commented Aug 14, 2023 •

edited

Loading

MichaReiser Aug 16, 2023

charliermarsh Aug 16, 2023

charliermarsh Aug 16, 2023

charliermarsh Aug 16, 2023 •

edited

Loading

charliermarsh Aug 17, 2023

MichaReiser Aug 17, 2023

MichaReiser Aug 17, 2023

	parent.end().sub(TextSize::new(1))
	parent.end() - TextSize::new(1)

Expand expressions to include parentheses in E712 #6575

Expand expressions to include parentheses in E712 #6575

Conversation

charliermarsh commented Aug 14, 2023 • edited Loading

Summary

Test Plan

github-actions bot commented Aug 14, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

MichaReiser Aug 16, 2023

Choose a reason for hiding this comment

charliermarsh Aug 16, 2023

Choose a reason for hiding this comment

charliermarsh Aug 16, 2023

Choose a reason for hiding this comment

charliermarsh Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh Aug 17, 2023

Choose a reason for hiding this comment

MichaReiser Aug 17, 2023

Choose a reason for hiding this comment

MichaReiser Aug 17, 2023

Choose a reason for hiding this comment

charliermarsh commented Aug 14, 2023 •

edited

Loading

github-actions bot commented Aug 14, 2023 •

edited

Loading

charliermarsh Aug 16, 2023 •

edited

Loading