Fix parenthesized detection for tuples (#6599)

## Summary

This PR fixes our code for detecting whether a tuple has its own
parentheses, which is necessary when attempting to preserve parentheses.
As-is, we were getting some cases wrong, like `(a := 1), (b := 3))` --
the detection code inferred that this _was_ parenthesized, and so
wrapped the entire thing in an unnecessary set of parentheses.

## Test Plan

`cargo test`

Before:

| project      | similarity index |
|--------------|------------------|
| cpython      | 0.75472          |
| django       | 0.99804          |
| transformers | 0.99618          |
| twine        | 0.99876          |
| typeshed     | 0.74288          |
| warehouse    | 0.99601          |
| zulip        | 0.99727          |

After:
| project      | similarity index |
|--------------|------------------|
| cpython      | 0.75473          |
| django       | 0.99804 |
| transformers | 0.99618          |
| twine        | 0.99876          |
| typeshed     | 0.74288          |
| warehouse    | 0.99601          |
| zulip        | 0.99727          |
This commit is contained in:
Charlie Marsh 2023-08-16 09:20:48 -04:00 committed by GitHub
parent daac31d2b9
commit 95f78821ad
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 67 additions and 71 deletions

View file

@ -1,7 +1,8 @@
use ruff_formatter::{format_args, write, FormatRuleWithOptions};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::ExprTuple;
use ruff_python_ast::{Expr, Ranged};
use ruff_python_ast::Ranged;
use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::TextRange;
use crate::builders::parenthesize_if_expands;
@ -11,7 +12,7 @@ use crate::expression::parentheses::{
};
use crate::prelude::*;
#[derive(Eq, PartialEq, Debug, Default)]
#[derive(Debug, Clone, Copy, Eq, PartialEq, Default)]
pub enum TupleParentheses {
/// By default tuples with a single element will include parentheses. Tuples with multiple elements
/// will parenthesize if the expression expands. This means that tuples will often *preserve*
@ -100,9 +101,9 @@ impl FormatRuleWithOptions<ExprTuple, PyFormatContext<'_>> for FormatExprTuple {
impl FormatNodeRule<ExprTuple> for FormatExprTuple {
fn fmt_fields(&self, item: &ExprTuple, f: &mut PyFormatter) -> FormatResult<()> {
let ExprTuple {
range,
elts,
ctx: _,
range: _,
} = item;
let comments = f.context().comments().clone();
@ -124,7 +125,7 @@ impl FormatNodeRule<ExprTuple> for FormatExprTuple {
}
[single] => match self.parentheses {
TupleParentheses::Preserve
if !is_parenthesized(*range, elts, f.context().source()) =>
if !is_tuple_parenthesized(item, f.context().source()) =>
{
write!(f, [single.format(), text(",")])
}
@ -141,7 +142,7 @@ impl FormatNodeRule<ExprTuple> for FormatExprTuple {
//
// Unlike other expression parentheses, tuple parentheses are part of the range of the
// tuple itself.
_ if is_parenthesized(*range, elts, f.context().source())
_ if is_tuple_parenthesized(item, f.context().source())
&& !(self.parentheses == TupleParentheses::NeverPreserve
&& dangling.is_empty()) =>
{
@ -203,21 +204,30 @@ impl NeedsParentheses for ExprTuple {
}
/// Check if a tuple has already had parentheses in the input
fn is_parenthesized(tuple_range: TextRange, elts: &[Expr], source: &str) -> bool {
let parentheses = '(';
let first_char = &source[usize::from(tuple_range.start())..].chars().next();
let Some(first_char) = first_char else {
fn is_tuple_parenthesized(tuple: &ExprTuple, source: &str) -> bool {
let Some(elt) = tuple.elts.first() else {
return false;
};
if *first_char != parentheses {
// Count the number of open parentheses between the start of the tuple and the first element.
let open_parentheses_count =
SimpleTokenizer::new(source, TextRange::new(tuple.start(), elt.start()))
.skip_trivia()
.filter(|token| token.kind() == SimpleTokenKind::LParen)
.count();
if open_parentheses_count == 0 {
return false;
}
// Consider `a = (1, 2), 3`: The first char of the current expr starts is a parentheses, but
// it's not its own but that of its first tuple child. We know that it belongs to the child
// because if it wouldn't, the child would start (at least) a char later
let Some(first_child) = elts.first() else {
return false;
};
first_child.range().start() != tuple_range.start()
// Count the number of parentheses between the end of the first element and its trailing comma.
let close_parentheses_count =
SimpleTokenizer::new(source, TextRange::new(elt.end(), tuple.end()))
.skip_trivia()
.take_while(|token| token.kind() != SimpleTokenKind::Comma)
.filter(|token| token.kind() == SimpleTokenKind::RParen)
.count();
// If the number of open parentheses is greater than the number of close parentheses, the tuple
// is parenthesized.
open_parentheses_count > close_parentheses_count
}