From 63ffadf0b801db9a5877f5e467afe069ae7f6425 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 7 Aug 2023 13:18:58 -0400 Subject: [PATCH] Avoid omitting parentheses for trailing attributes on call expressions (#6322) ## Summary This PR modifies our `can_omit_optional_parentheses` rules to ensure that if we see a call followed by an attribute, we treat that as an attribute access rather than a splittable call expression. This in turn ensures that we wrap like: ```python ct_match = aaaaaaaaaaact_id == self.get_content_type( obj=rel_obj, using=instance._state.db ) ``` For calls, but: ```python ct_match = ( aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db).id ) ``` For calls with trailing attribute accesses. Closes https://github.com/astral-sh/ruff/issues/6065. ## Test Plan Similarity index before: - `zulip`: 0.99436 - `django`: 0.99779 - `warehouse`: 0.99504 - `transformers`: 0.99403 - `cpython`: 0.75912 - `typeshed`: 0.72293 And after: - `zulip`: 0.99436 - `django`: 0.99780 - `warehouse`: 0.99504 - `transformers`: 0.99404 - `cpython`: 0.75913 - `typeshed`: 0.72293 --- .../test/fixtures/ruff/expression/compare.py | 52 +++++++++ .../src/expression/mod.rs | 6 +- .../format@expression__compare.py.snap | 106 ++++++++++++++++++ 3 files changed, 162 insertions(+), 2 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/compare.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/compare.py index 906d5710aa..88b6da20bd 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/compare.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/compare.py @@ -59,3 +59,55 @@ return 1 == 2 and ( >= c ) ] + +def f(): + return ( + unicodedata.normalize("NFKC", s1).casefold() + == unicodedata.normalize("NFKC", s2).casefold() + ) + +# Call expressions with trailing attributes. + +ct_match = ( + aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db).id +) + +ct_match = ( + {aaaaaaaaaaaaaaaa} == self.get_content_type(obj=rel_obj, using=instance._state.db).id +) + +ct_match = ( + (aaaaaaaaaaaaaaaa) == self.get_content_type(obj=rel_obj, using=instance._state.db).id +) + +ct_match = aaaaaaaaaaact_id == self.get_content_type( + obj=rel_obj, using=instance._state.db +) + +# Call expressions with trailing subscripts. + +ct_match = ( + aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db)[id] +) + +ct_match = ( + {aaaaaaaaaaaaaaaa} == self.get_content_type(obj=rel_obj, using=instance._state.db)[id] +) + +ct_match = ( + (aaaaaaaaaaaaaaaa) == self.get_content_type(obj=rel_obj, using=instance._state.db)[id] +) + +# Subscripts expressions with trailing attributes. + +ct_match = ( + aaaaaaaaaaact_id == self.get_content_type[obj, rel_obj, using, instance._state.db].id +) + +ct_match = ( + {aaaaaaaaaaaaaaaa} == self.get_content_type[obj, rel_obj, using, instance._state.db].id +) + +ct_match = ( + (aaaaaaaaaaaaaaaa) == self.get_content_type[obj, rel_obj, using, instance._state.db].id +) diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs index f11b2a1f0d..971dc0898e 100644 --- a/crates/ruff_python_formatter/src/expression/mod.rs +++ b/crates/ruff_python_formatter/src/expression/mod.rs @@ -178,10 +178,9 @@ impl Format> for MaybeParenthesizeExpression<'_> { Parenthesize::Optional | Parenthesize::IfBreaks => needs_parentheses, }; - let can_omit_optional_parentheses = can_omit_optional_parentheses(expression, f.context()); match needs_parentheses { OptionalParentheses::Multiline if *parenthesize != Parenthesize::IfRequired => { - if can_omit_optional_parentheses { + if can_omit_optional_parentheses(expression, f.context()) { optional_parentheses(&expression.format().with_options(Parentheses::Never)) .fmt(f) } else { @@ -407,9 +406,12 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> { attr: _, ctx: _, }) => { + self.visit_expr(value); if has_parentheses(value, self.source) { self.update_max_priority(OperatorPriority::Attribute); } + self.last = Some(expr); + return; } Expr::NamedExpr(_) diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap index f62138f5d9..91690dd6de 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__compare.py.snap @@ -65,6 +65,58 @@ return 1 == 2 and ( >= c ) ] + +def f(): + return ( + unicodedata.normalize("NFKC", s1).casefold() + == unicodedata.normalize("NFKC", s2).casefold() + ) + +# Call expressions with trailing attributes. + +ct_match = ( + aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db).id +) + +ct_match = ( + {aaaaaaaaaaaaaaaa} == self.get_content_type(obj=rel_obj, using=instance._state.db).id +) + +ct_match = ( + (aaaaaaaaaaaaaaaa) == self.get_content_type(obj=rel_obj, using=instance._state.db).id +) + +ct_match = aaaaaaaaaaact_id == self.get_content_type( + obj=rel_obj, using=instance._state.db +) + +# Call expressions with trailing subscripts. + +ct_match = ( + aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db)[id] +) + +ct_match = ( + {aaaaaaaaaaaaaaaa} == self.get_content_type(obj=rel_obj, using=instance._state.db)[id] +) + +ct_match = ( + (aaaaaaaaaaaaaaaa) == self.get_content_type(obj=rel_obj, using=instance._state.db)[id] +) + +# Subscripts expressions with trailing attributes. + +ct_match = ( + aaaaaaaaaaact_id == self.get_content_type[obj, rel_obj, using, instance._state.db].id +) + +ct_match = ( + {aaaaaaaaaaaaaaaa} == self.get_content_type[obj, rel_obj, using, instance._state.db].id +) + +ct_match = ( + (aaaaaaaaaaaaaaaa) == self.get_content_type[obj, rel_obj, using, instance._state.db].id +) ``` ## Output @@ -171,6 +223,60 @@ return 1 == 2 and ( >= c ) ] + + +def f(): + return unicodedata.normalize("NFKC", s1).casefold() == unicodedata.normalize( + "NFKC", s2 + ).casefold() + + +# Call expressions with trailing attributes. + +ct_match = ( + aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db).id +) + +ct_match = {aaaaaaaaaaaaaaaa} == self.get_content_type( + obj=rel_obj, using=instance._state.db +).id + +ct_match = (aaaaaaaaaaaaaaaa) == self.get_content_type( + obj=rel_obj, using=instance._state.db +).id + +ct_match = aaaaaaaaaaact_id == self.get_content_type( + obj=rel_obj, using=instance._state.db +) + +# Call expressions with trailing subscripts. + +ct_match = ( + aaaaaaaaaaact_id == self.get_content_type(obj=rel_obj, using=instance._state.db)[id] +) + +ct_match = { + aaaaaaaaaaaaaaaa +} == self.get_content_type(obj=rel_obj, using=instance._state.db)[id] + +ct_match = ( + aaaaaaaaaaaaaaaa +) == self.get_content_type(obj=rel_obj, using=instance._state.db)[id] + +# Subscripts expressions with trailing attributes. + +ct_match = ( + aaaaaaaaaaact_id + == self.get_content_type[obj, rel_obj, using, instance._state.db].id +) + +ct_match = { + aaaaaaaaaaaaaaaa +} == self.get_content_type[obj, rel_obj, using, instance._state.db].id + +ct_match = ( + aaaaaaaaaaaaaaaa +) == self.get_content_type[obj, rel_obj, using, instance._state.db].id ```