Skip to content

Commit

Permalink
Add is_parenthesized flag to ExprTuple
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaReiser committed Jan 22, 2024
1 parent f5061db commit 1e449cd
Show file tree
Hide file tree
Showing 57 changed files with 339 additions and 66 deletions.
1 change: 1 addition & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
elts,
ctx,
range: _,
is_parenthesized: _,
})
| Expr::List(ast::ExprList {
elts,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1232,6 +1232,7 @@ where
elts,
ctx,
range: _,
is_parenthesized: _,
}) = slice.as_ref()
{
let mut iter = elts.iter();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ fn type_pattern(elts: Vec<&Expr>) -> Expr {
elts: elts.into_iter().cloned().collect(),
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
}
.into()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) {
.collect(),
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
});
let node1 = Expr::Name(ast::ExprName {
id: arg_name.into(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ pub(crate) fn unnecessary_literal_union<'a>(checker: &mut Checker, expr: &'a Exp
elts,
range: _,
ctx: _,
is_parenthesized: _,
}) = slice.as_ref()
{
for expr in elts {
Expand Down Expand Up @@ -123,6 +124,7 @@ pub(crate) fn unnecessary_literal_union<'a>(checker: &mut Checker, expr: &'a Exp
elts: literal_exprs.into_iter().cloned().collect(),
range: TextRange::default(),
ctx: ExprContext::Load,
is_parenthesized: true,
})),
range: TextRange::default(),
ctx: ExprContext::Load,
Expand All @@ -148,6 +150,7 @@ pub(crate) fn unnecessary_literal_union<'a>(checker: &mut Checker, expr: &'a Exp
elts,
range: TextRange::default(),
ctx: ExprContext::Load,
is_parenthesized: true,
})),
range: TextRange::default(),
ctx: ExprContext::Load,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ pub(crate) fn unnecessary_type_union<'a>(checker: &mut Checker, union: &'a Expr)
.collect(),
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
})),
ctx: ExprContext::Load,
range: TextRange::default(),
Expand All @@ -151,6 +152,7 @@ pub(crate) fn unnecessary_type_union<'a>(checker: &mut Checker, union: &'a Expr)
elts: exprs.into_iter().cloned().collect(),
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
})),
ctx: ExprContext::Load,
range: TextRange::default(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ fn check_names(checker: &mut Checker, decorator: &Decorator, expr: &Expr) {
.collect(),
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
});
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
format!("({})", checker.generator().expr(&node)),
Expand Down Expand Up @@ -437,6 +438,7 @@ fn check_names(checker: &mut Checker, decorator: &Decorator, expr: &Expr) {
elts: elts.clone(),
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
});
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
format!("({})", checker.generator().expr(&node)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,7 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
.collect(),
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
};
let node1 = ast::ExprName {
id: "isinstance".into(),
Expand Down Expand Up @@ -543,6 +544,7 @@ pub(crate) fn compare_with_tuple(checker: &mut Checker, expr: &Expr) {
elts: comparators.into_iter().map(Clone::clone).collect(),
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
};
let node1 = ast::ExprName {
id: id.into(),
Expand Down Expand Up @@ -718,7 +720,7 @@ fn get_short_circuit_edit(
generator.expr(expr)
};
Edit::range_replacement(
if matches!(expr, Expr::Tuple(ast::ExprTuple { elts, ctx: _, range: _}) if !elts.is_empty())
if matches!(expr, Expr::Tuple(ast::ExprTuple { elts, ctx: _, range: _, is_parenthesized: _}) if !elts.is_empty())
{
format!("({content})")
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,7 @@ fn assignment_targets_from_expr<'a>(
ctx: ExprContext::Store,
elts,
range: _,
is_parenthesized: _,
}) => Box::new(
elts.iter()
.flat_map(|elt| assignment_targets_from_expr(elt, dummy_variable_rgx)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ pub(crate) fn repeated_equality_comparison(checker: &mut Checker, bool_op: &ast:
elts: comparators.iter().copied().cloned().collect(),
range: TextRange::default(),
ctx: ExprContext::Load,
is_parenthesized: true,
})],
range: bool_op.range(),
})),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ fn tuple_diagnostic(checker: &mut Checker, tuple: &ast::ExprTuple, aliases: &[&E
elts: remaining,
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
};
format!("({})", checker.generator().expr(&node.into()))
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ fn tuple_diagnostic(checker: &mut Checker, tuple: &ast::ExprTuple, aliases: &[&E
elts: remaining,
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
};
format!("({})", checker.generator().expr(&node.into()))
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ pub(crate) fn non_pep695_type_alias(checker: &mut Checker, stmt: &StmtAnnAssign)
range: TextRange::default(),
elts: constraints.into_iter().cloned().collect(),
ctx: ast::ExprContext::Load,
is_parenthesized: true,
})))
}
None => None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ fn make_suggestion(group: &AppendGroup, generator: Generator) -> String {
elts,
ctx: ast::ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
};
// Make `var.extend`.
// NOTE: receiver is the same for all appends and that's why we can take the first.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ fn concatenate_expressions(expr: &Expr) -> Option<(Expr, Type)> {
elts: new_elts,
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
}
.into(),
};
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/never_union.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ pub(crate) fn never_union(checker: &mut Checker, expr: &Expr) {
elts,
ctx: _,
range: _,
is_parenthesized: _,
}) = slice.as_ref()
else {
return;
Expand Down Expand Up @@ -157,6 +158,7 @@ pub(crate) fn never_union(checker: &mut Checker, expr: &Expr) {
elts: rest,
ctx: ast::ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
})),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
Expand Down
10 changes: 6 additions & 4 deletions crates/ruff_linter/src/rules/ruff/rules/sequence_sorting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,14 @@ impl SequenceKind<'_> {
// N.B. We only need the source code for the Tuple variant here,
// but if you already have a `Locator` instance handy,
// getting the source code is very cheap.
fn surrounding_brackets(&self, source: &str) -> (&'static str, &'static str) {
fn surrounding_brackets(&self) -> (&'static str, &'static str) {
match self {
Self::List => ("[", "]"),
Self::Set => ("{", "}"),
Self::Tuple(ast_node) => {
if ast_node.is_parenthesized(source) {
Self::Tuple(ast::ExprTuple {
is_parenthesized, ..
}) => {
if *is_parenthesized {
("(", ")")
} else {
("", "")
Expand Down Expand Up @@ -225,7 +227,7 @@ pub(super) fn sort_single_line_elements_sequence(
) -> String {
let element_pairs = SequenceElements::new(elements, elts);
let last_item_index = element_pairs.last_item_index();
let (opening_paren, closing_paren) = kind.surrounding_brackets(locator.contents());
let (opening_paren, closing_paren) = kind.surrounding_brackets();
let mut result = String::from(opening_paren);
// We grab the original source-code ranges using `locator.slice()`
// rather than using the expression generator, as this approach allows
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_python_ast/src/comparable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1072,6 +1072,7 @@ impl<'a> From<&'a ast::Expr> for ComparableExpr<'a> {
elts,
ctx: _,
range: _,
is_parenthesized: _,
}) => Self::Tuple(ExprTuple {
elts: elts.iter().map(Into::into).collect(),
}),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_python_ast/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1419,6 +1419,7 @@ pub fn pep_604_union(elts: &[Expr]) -> Expr {
elts: vec![],
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
}),
[Expr::Tuple(ast::ExprTuple { elts, .. })] => pep_604_union(elts),
[elt] => elt.clone(),
Expand Down Expand Up @@ -1453,6 +1454,7 @@ pub fn typing_union(elts: &[Expr], binding: String) -> Expr {
elts: vec![],
ctx: ExprContext::Load,
range: TextRange::default(),
is_parenthesized: true,
}),
[Expr::Tuple(ast::ExprTuple { elts, .. })] => typing_union(elts, binding),
[elt] => elt.clone(),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_python_ast/src/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3212,6 +3212,7 @@ impl AstNode for ast::ExprTuple {
elts,
ctx: _,
range: _,
is_parenthesized: _,
} = self;

for expr in elts {
Expand Down
35 changes: 3 additions & 32 deletions crates/ruff_python_ast/src/nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use std::slice::{Iter, IterMut};

use itertools::Itertools;

use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::{Ranged, TextRange, TextSize};

use crate::{int, LiteralExpressionRef};
Expand Down Expand Up @@ -1794,6 +1793,9 @@ pub struct ExprTuple {
pub range: TextRange,
pub elts: Vec<Expr>,
pub ctx: ExprContext,

/// Whether the tuple is parenthesized in the source code.
pub is_parenthesized: bool,
}

impl From<ExprTuple> for Expr {
Expand All @@ -1802,37 +1804,6 @@ impl From<ExprTuple> for Expr {
}
}

impl ExprTuple {
/// Return `true` if a tuple is parenthesized in the source code.
pub fn is_parenthesized(&self, source: &str) -> bool {
let Some(elt) = self.elts.first() else {
return true;
};

// 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(self.start(), elt.start()))
.skip_trivia()
.filter(|token| token.kind() == SimpleTokenKind::LParen)
.count();
if open_parentheses_count == 0 {
return false;
}

// 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(), self.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
}
}

/// See also [Slice](https://docs.python.org/3/library/ast.html#ast.Slice)
#[derive(Clone, Debug, PartialEq)]
pub struct ExprSlice {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_python_ast/src/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,7 @@ pub fn walk_expr<'a, V: Visitor<'a> + ?Sized>(visitor: &mut V, expr: &'a Expr) {
elts,
ctx,
range: _,
is_parenthesized: _,
}) => {
for expr in elts {
visitor.visit_expr(expr);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_python_ast/src/visitor/transformer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,7 @@ pub fn walk_expr<V: Transformer + ?Sized>(visitor: &V, expr: &mut Expr) {
elts,
ctx,
range: _,
is_parenthesized: _,
}) => {
for expr in elts {
visitor.visit_expr(expr);
Expand Down
7 changes: 4 additions & 3 deletions crates/ruff_python_formatter/src/comments/placement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,9 +293,10 @@ fn handle_enclosed_comment<'a>(
| AnyNodeRef::ExprSet(_)
| AnyNodeRef::ExprListComp(_)
| AnyNodeRef::ExprSetComp(_) => handle_bracketed_end_of_line_comment(comment, locator),
AnyNodeRef::ExprTuple(tuple) if tuple.is_parenthesized(locator.contents()) => {
handle_bracketed_end_of_line_comment(comment, locator)
}
AnyNodeRef::ExprTuple(ast::ExprTuple {
is_parenthesized: true,
..
}) => handle_bracketed_end_of_line_comment(comment, locator),
AnyNodeRef::ExprGeneratorExp(generator)
if is_generator_parenthesized(generator, locator.contents()) =>
{
Expand Down
5 changes: 3 additions & 2 deletions crates/ruff_python_formatter/src/expression/expr_tuple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ impl FormatNodeRule<ExprTuple> for FormatExprTuple {
elts,
ctx: _,
range: _,
is_parenthesized,
} = item;

let comments = f.context().comments().clone();
Expand All @@ -136,7 +137,7 @@ impl FormatNodeRule<ExprTuple> for FormatExprTuple {
return empty_parenthesized("(", dangling, ")").fmt(f);
}
[single] => match self.parentheses {
TupleParentheses::Preserve if !item.is_parenthesized(f.context().source()) => {
TupleParentheses::Preserve if !is_parenthesized => {
write!(f, [single.format(), token(",")])
}
_ =>
Expand All @@ -152,7 +153,7 @@ impl FormatNodeRule<ExprTuple> for FormatExprTuple {
//
// Unlike other expression parentheses, tuple parentheses are part of the range of the
// tuple itself.
_ if item.is_parenthesized(f.context().source())
_ if *is_parenthesized
&& !(self.parentheses == TupleParentheses::NeverPreserve
&& dangling.is_empty()) =>
{
Expand Down
12 changes: 10 additions & 2 deletions crates/ruff_python_formatter/src/expression/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,10 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
return;
}

Expr::Tuple(tuple) if tuple.is_parenthesized(self.context.source()) => {
Expr::Tuple(ast::ExprTuple {
is_parenthesized: true,
..
}) => {
self.any_parenthesized_expressions = true;
// The values are always parenthesized, don't visit.
return;
Expand Down Expand Up @@ -1058,7 +1061,12 @@ pub(crate) fn has_own_parentheses(
}
}

Expr::Tuple(tuple) if tuple.is_parenthesized(context.source()) => {
Expr::Tuple(
tuple @ ast::ExprTuple {
is_parenthesized: true,
..
},
) => {
if !tuple.elts.is_empty() || context.comments().has_dangling(AnyNodeRef::from(expr)) {
Some(OwnParentheses::NonEmpty)
} else {
Expand Down
8 changes: 7 additions & 1 deletion crates/ruff_python_parser/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,16 @@ use ruff_python_ast::{self as ast, Expr, ExprContext};
pub(crate) fn set_context(expr: Expr, ctx: ExprContext) -> Expr {
match expr {
Expr::Name(ast::ExprName { id, range, .. }) => ast::ExprName { range, id, ctx }.into(),
Expr::Tuple(ast::ExprTuple { elts, range, .. }) => ast::ExprTuple {
Expr::Tuple(ast::ExprTuple {
elts,
range,
is_parenthesized,
ctx: _,
}) => ast::ExprTuple {
elts: elts.into_iter().map(|elt| set_context(elt, ctx)).collect(),
range,
ctx,
is_parenthesized,
}
.into(),

Expand Down
Loading

0 comments on commit 1e449cd

Please sign in to comment.