Unnamed repository; edit this file 'description' to name the repository.
Auto merge of #13764 - WaffleLapkin:badassexprs, r=Veykril
fix: Correctly check for parentheses redundancy in `remove_parentheses` assist
This is quite a bunch of code and some hacks, but I _think_ this time it's correct.
I've added a lot of tests, most of which fail with the assist impl from #13733 :')
| -rw-r--r-- | crates/ide-assists/src/handlers/remove_parentheses.rs | 136 | ||||
| -rw-r--r-- | crates/syntax/src/ast/prec.rs | 401 |
2 files changed, 440 insertions, 97 deletions
diff --git a/crates/ide-assists/src/handlers/remove_parentheses.rs b/crates/ide-assists/src/handlers/remove_parentheses.rs index 185beda9d0..e9c7c6bae9 100644 --- a/crates/ide-assists/src/handlers/remove_parentheses.rs +++ b/crates/ide-assists/src/handlers/remove_parentheses.rs @@ -29,9 +29,8 @@ pub(crate) fn remove_parentheses(acc: &mut Assists, ctx: &AssistContext<'_>) -> let expr = parens.expr()?; - let parent = ast::Expr::cast(parens.syntax().parent()?); - let is_ok_to_remove = expr.precedence() >= parent.as_ref().and_then(ast::Expr::precedence); - if !is_ok_to_remove { + let parent = parens.syntax().parent()?; + if expr.needs_parens_in(parent) { return None; } @@ -59,6 +58,31 @@ mod tests { } #[test] + fn remove_parens_closure() { + check_assist(remove_parentheses, r#"fn f() { &$0(|| 42) }"#, r#"fn f() { &|| 42 }"#); + + check_assist_not_applicable(remove_parentheses, r#"fn f() { $0(|| 42).f() }"#); + } + + #[test] + fn remove_parens_if_let_chains() { + check_assist_not_applicable( + remove_parentheses, + r#"fn f() { if let true = $0(true && true) {} }"#, + ); + } + + #[test] + fn remove_parens_associativity() { + check_assist( + remove_parentheses, + r#"fn f() { $0(2 + 2) + 2; }"#, + r#"fn f() { 2 + 2 + 2; }"#, + ); + check_assist_not_applicable(remove_parentheses, r#"fn f() { 2 + $0(2 + 2); }"#); + } + + #[test] fn remove_parens_precedence() { check_assist( remove_parentheses, @@ -88,4 +112,110 @@ mod tests { check_assist_not_applicable(remove_parentheses, r#"fn f() { (2 +$0 2) }"#); check_assist_not_applicable(remove_parentheses, r#"fn f() {$0 (2 + 2) }"#); } + + #[test] + fn remove_parens_doesnt_apply_when_expr_would_be_turned_into_a_statement() { + check_assist_not_applicable(remove_parentheses, r#"fn x() -> u8 { $0({ 0 } + 1) }"#); + check_assist_not_applicable( + remove_parentheses, + r#"fn x() -> u8 { $0(if true { 0 } else { 1 } + 1) }"#, + ); + check_assist_not_applicable(remove_parentheses, r#"fn x() -> u8 { $0(loop {} + 1) }"#); + } + + #[test] + fn remove_parens_doesnt_apply_weird_syntax_and_adge_cases() { + // removing `()` would break code because {} would be counted as the loop/if body + check_assist_not_applicable(remove_parentheses, r#"fn f() { for _ in $0(0..{3}) {} }"#); + check_assist_not_applicable(remove_parentheses, r#"fn f() { for _ in $0(S {}) {} }"#); + check_assist_not_applicable(remove_parentheses, r#"fn f() { if $0(S {} == 2) {} }"#); + check_assist_not_applicable(remove_parentheses, r#"fn f() { if $0(return) {} }"#); + } + + #[test] + fn remove_parens_return_with_value_followed_by_block() { + check_assist( + remove_parentheses, + r#"fn f() { if $0(return ()) {} }"#, + r#"fn f() { if return () {} }"#, + ); + } + + #[test] + fn remove_exprs_let_else_restrictions() { + // `}` is not allowed before `else` here + check_assist_not_applicable( + remove_parentheses, + r#"fn f() { let _ = $0(S{}) else { return }; }"#, + ); + + // logic operators can't directly appear in the let-else + check_assist_not_applicable( + remove_parentheses, + r#"fn f() { let _ = $0(false || false) else { return }; }"#, + ); + check_assist_not_applicable( + remove_parentheses, + r#"fn f() { let _ = $0(true && true) else { return }; }"#, + ); + } + + #[test] + fn remove_parens_weird_places() { + check_assist( + remove_parentheses, + r#"fn f() { match () { _=>$0(()) } }"#, + r#"fn f() { match () { _=>() } }"#, + ); + + check_assist( + remove_parentheses, + r#"fn x() -> u8 { { [$0({ 0 } + 1)] } }"#, + r#"fn x() -> u8 { { [{ 0 } + 1] } }"#, + ); + } + + #[test] + fn remove_parens_return_dot_f() { + check_assist( + remove_parentheses, + r#"fn f() { $0(return).f() }"#, + r#"fn f() { return.f() }"#, + ); + } + + #[test] + fn remove_parens_prefix_then_return_something() { + check_assist( + remove_parentheses, + r#"fn f() { &$0(return ()) }"#, + r#"fn f() { &return () }"#, + ); + } + + #[test] + fn remove_parens_double_paren_stmt() { + check_assist( + remove_parentheses, + r#"fn x() -> u8 { $0(({ 0 } + 1)) }"#, + r#"fn x() -> u8 { ({ 0 } + 1) }"#, + ); + + check_assist( + remove_parentheses, + r#"fn x() -> u8 { (($0{ 0 } + 1)) }"#, + r#"fn x() -> u8 { ({ 0 } + 1) }"#, + ); + } + + #[test] + fn remove_parens_im_tired_of_naming_tests() { + check_assist( + remove_parentheses, + r#"fn f() { 2 + $0(return 2) }"#, + r#"fn f() { 2 + return 2 }"#, + ); + + check_assist_not_applicable(remove_parentheses, r#"fn f() { $0(return 2) + 2 }"#); + } } diff --git a/crates/syntax/src/ast/prec.rs b/crates/syntax/src/ast/prec.rs index 6253c4dc3e..ac7ef45c1d 100644 --- a/crates/syntax/src/ast/prec.rs +++ b/crates/syntax/src/ast/prec.rs @@ -1,115 +1,328 @@ //! Precedence representation. -use crate::ast::{self, BinExpr, Expr}; - -/// Precedence of an expression. -#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] -pub enum ExprPrecedence { - // N.B.: Order is important - Closure, - Jump, - Range, - Bin(BinOpPresedence), - Prefix, - Postfix, - Paren, -} - -/// Precedence of a binary operator. -#[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq)] -pub enum BinOpPresedence { - // N.B.: Order is important - /// `=`, `+=`, `-=`, `*=`, `/=`, `%=`, `|=`, `&=` - Assign, - /// `||` - LOr, - /// `&&` - LAnd, - /// `<`, `<=`, `>`, `>=`, `==` and `!=` - Cmp, - /// `|` - BitOr, - /// `^` - BitXor, - /// `&` - BitAnd, - /// `<<` and `>>` - Shift, - /// `+` and `-` - Add, - /// `*`, `/` and `%` - Mul, - /// `as` - As, -} +use crate::{ + ast::{self, BinaryOp, Expr, HasArgList}, + match_ast, AstNode, SyntaxNode, +}; impl Expr { - /// Returns precedence of this expression. - /// Usefull to preserve semantics in assists. - /// - /// Returns `None` if this is a [`BinExpr`] and its [`op_kind`] returns `None`. - /// - /// [`op_kind`]: BinExpr::op_kind - /// [`BinExpr`]: Expr::BinExpr - pub fn precedence(&self) -> Option<ExprPrecedence> { - // Copied from <https://github.com/rust-lang/rust/blob/b6852428a8ea9728369b64b9964cad8e258403d3/compiler/rustc_ast/src/util/parser.rs#L296> + // Implementation is based on + // - https://doc.rust-lang.org/reference/expressions.html#expression-precedence + // - https://matklad.github.io/2020/04/13/simple-but-powerful-pratt-parsing.html + // - rustc source, including, but not limited to + // - https://github.com/rust-lang/rust/blob/b6852428a8ea9728369b64b9964cad8e258403d3/compiler/rustc_ast/src/util/parser.rs#L296 + + /// Returns `true` if `self` would need to be wrapped in parentheses given that its parent is `parent`. + pub fn needs_parens_in(&self, parent: SyntaxNode) -> bool { + match_ast! { + match parent { + ast::Expr(e) => self.needs_parens_in_expr(&e), + ast::Stmt(e) => self.needs_parens_in_stmt(Some(&e)), + ast::StmtList(_) => self.needs_parens_in_stmt(None), + ast::ArgList(_) => false, + ast::MatchArm(_) => false, + _ => false, + } + } + } + + fn needs_parens_in_expr(&self, parent: &Expr) -> bool { + // Special-case block weirdness + if parent.child_is_followed_by_a_block() { + use Expr::*; + match self { + // Cases like `if return {}` (need parens or else `{}` is returned, instead of being `if`'s body) + ReturnExpr(e) if e.expr().is_none() => return true, + BreakExpr(e) if e.expr().is_none() => return true, + YieldExpr(e) if e.expr().is_none() => return true, + + // Same but with `..{}` + RangeExpr(e) if matches!(e.end(), Some(BlockExpr(..))) => return true, + + // Similarly with struct literals, e.g. `if S{} == 1 {}` + _ if self.contains_exterior_struct_lit() => return true, + _ => {} + } + } + + // Special-case `return.f()` + if self.is_ret_like_with_no_value() && parent.is_postfix() { + return false; + } + + if self.is_paren_like() + || parent.is_paren_like() + || self.is_prefix() && (parent.is_prefix() || !self.is_ordered_before(parent)) + || self.is_postfix() && (parent.is_postfix() || self.is_ordered_before(parent)) + { + return false; + } + + let (left, right, inv) = match self.is_ordered_before(parent) { + true => (self, parent, false), + false => (parent, self, true), + }; + + let (_, left_right_bp) = left.binding_power(); + let (right_left_bp, _) = right.binding_power(); + + (left_right_bp < right_left_bp) ^ inv + } + + fn needs_parens_in_stmt(&self, stmt: Option<&ast::Stmt>) -> bool { use Expr::*; - let prec = match self { - ClosureExpr(_) => ExprPrecedence::Closure, + // Prevent false-positives in cases like `fn x() -> u8 { ({ 0 } + 1) }`, + // `{ { 0 } + 1 }` won't parse -- `{ 0 }` would be parsed as a self-contained stmt, + // leaving `+ 1` as a parse error. + let mut innermost = self.clone(); + loop { + let next = match &innermost { + BinExpr(e) => e.lhs(), + CallExpr(e) => e.expr(), + CastExpr(e) => e.expr(), + IndexExpr(e) => e.base(), + _ => break, + }; - ContinueExpr(_) | ReturnExpr(_) | YieldExpr(_) | BreakExpr(_) => ExprPrecedence::Jump, + if let Some(next) = next { + innermost = next; + if !innermost.requires_semi_to_be_stmt() { + return true; + } + } else { + break; + } + } - RangeExpr(_) => ExprPrecedence::Range, + // Not every expression can be followed by `else` in the `let-else` + if let Some(ast::Stmt::LetStmt(e)) = stmt { + if e.let_else().is_some() { + match self { + BinExpr(e) + if e.op_kind() + .map(|op| matches!(op, BinaryOp::LogicOp(_))) + .unwrap_or(false) => + { + return true + } + _ if self.clone().trailing_brace().is_some() => return true, + _ => {} + } + } + } - BinExpr(bin_expr) => return bin_expr.precedence().map(ExprPrecedence::Bin), - CastExpr(_) => ExprPrecedence::Bin(BinOpPresedence::As), + false + } + + /// Returns left and right so-called "binding powers" of this expression. + fn binding_power(&self) -> (u8, u8) { + use ast::{ArithOp::*, BinaryOp::*, Expr::*, LogicOp::*}; + + let dps = match self { + // (0, 0) -- paren-like/nullary + // (0, N) -- prefix + // (N, 0) -- postfix + // (N, N) -- infix, requires parens + // (N, N+1) -- infix, left to right associative + // (N+1, N) -- infix, right to left associative + // N is odd + // + ContinueExpr(_) => (0, 0), + + ClosureExpr(_) | ReturnExpr(_) | YieldExpr(_) | BreakExpr(_) => (0, 1), + + RangeExpr(_) => (5, 5), + + BinExpr(e) => { + // Return a dummy value if we don't know the op + let Some(op) = e.op_kind() else { return (0, 0) }; + match op { + Assignment { .. } => (4, 3), + // + // Ranges are here in order :) + // + LogicOp(op) => match op { + Or => (7, 8), + And => (9, 10), + }, + CmpOp(_) => (11, 11), + ArithOp(op) => match op { + BitOr => (13, 14), + BitXor => (15, 16), + BitAnd => (17, 18), + Shl | Shr => (19, 20), + Add | Sub => (21, 22), + Mul | Div | Rem => (23, 24), + }, + } + } + + CastExpr(_) => (25, 26), + + BoxExpr(_) | RefExpr(_) | LetExpr(_) | PrefixExpr(_) => (0, 27), - BoxExpr(_) | RefExpr(_) | LetExpr(_) | PrefixExpr(_) => ExprPrecedence::Prefix, + AwaitExpr(_) | CallExpr(_) | MethodCallExpr(_) | IndexExpr(_) | TryExpr(_) + | MacroExpr(_) => (29, 0), - AwaitExpr(_) | CallExpr(_) | MethodCallExpr(_) | FieldExpr(_) | IndexExpr(_) - | TryExpr(_) | MacroExpr(_) => ExprPrecedence::Postfix, + FieldExpr(_) => (31, 32), ArrayExpr(_) | TupleExpr(_) | Literal(_) | PathExpr(_) | ParenExpr(_) | IfExpr(_) | WhileExpr(_) | ForExpr(_) | LoopExpr(_) | MatchExpr(_) | BlockExpr(_) - | RecordExpr(_) | UnderscoreExpr(_) => ExprPrecedence::Paren, + | RecordExpr(_) | UnderscoreExpr(_) => (0, 0), }; - Some(prec) + dps } -} -impl BinExpr { - /// Returns precedence of this binary expression. - /// Usefull to preserve semantics in assists. - /// - /// Returns `None` if [`op_kind`] returns `None`. - /// - /// [`op_kind`]: BinExpr::op_kind - pub fn precedence(&self) -> Option<BinOpPresedence> { - use ast::{ArithOp::*, BinaryOp::*, LogicOp::*}; - - let prec = match self.op_kind()? { - LogicOp(op) => match op { - And => BinOpPresedence::LAnd, - Or => BinOpPresedence::LOr, - }, - ArithOp(op) => match op { - Add => BinOpPresedence::Add, - Mul => BinOpPresedence::Mul, - Sub => BinOpPresedence::Add, - Div => BinOpPresedence::Mul, - Rem => BinOpPresedence::Mul, - Shl => BinOpPresedence::Shift, - Shr => BinOpPresedence::Shift, - BitXor => BinOpPresedence::BitXor, - BitOr => BinOpPresedence::BitOr, - BitAnd => BinOpPresedence::BitAnd, - }, - CmpOp(_) => BinOpPresedence::Cmp, - Assignment { .. } => BinOpPresedence::Assign, - }; + fn is_paren_like(&self) -> bool { + matches!(self.binding_power(), (0, 0)) + } + + fn is_prefix(&self) -> bool { + matches!(self.binding_power(), (0, 1..)) + } + + fn is_postfix(&self) -> bool { + matches!(self.binding_power(), (1.., 0)) + } + + /// Returns `true` if this expression can't be a standalone statement. + fn requires_semi_to_be_stmt(&self) -> bool { + use Expr::*; + !matches!( + self, + IfExpr(..) | MatchExpr(..) | BlockExpr(..) | WhileExpr(..) | LoopExpr(..) | ForExpr(..) + ) + } + + /// If an expression ends with `}`, returns the innermost expression ending in this `}`. + fn trailing_brace(mut self) -> Option<Expr> { + use Expr::*; + + loop { + let rhs = match self { + RefExpr(e) => e.expr(), + BinExpr(e) => e.rhs(), + BoxExpr(e) => e.expr(), + BreakExpr(e) => e.expr(), + LetExpr(e) => e.expr(), + RangeExpr(e) => e.end(), + ReturnExpr(e) => e.expr(), + PrefixExpr(e) => e.expr(), + YieldExpr(e) => e.expr(), + ClosureExpr(e) => e.body(), + + BlockExpr(..) | ForExpr(..) | IfExpr(..) | LoopExpr(..) | MatchExpr(..) + | RecordExpr(..) | WhileExpr(..) => break Some(self), + _ => break None, + }; + + self = rhs?; + } + } + + /// Expressions that syntactically contain an "exterior" struct literal i.e., not surrounded by any + /// parens or other delimiters, e.g., `X { y: 1 }`, `X { y: 1 }.method()`, `foo == X { y: 1 }` and + /// `X { y: 1 } == foo` all do, but `(X { y: 1 }) == foo` does not. + fn contains_exterior_struct_lit(&self) -> bool { + return contains_exterior_struct_lit_inner(self).is_some(); + + fn contains_exterior_struct_lit_inner(expr: &Expr) -> Option<()> { + use Expr::*; + + match expr { + RecordExpr(..) => Some(()), + + // X { y: 1 } + X { y: 2 } + BinExpr(e) => e + .lhs() + .as_ref() + .and_then(contains_exterior_struct_lit_inner) + .or_else(|| e.rhs().as_ref().and_then(contains_exterior_struct_lit_inner)), + + // `&X { y: 1 }`, `X { y: 1 }.y`, `X { y: 1 }.bar(...)`, etc + IndexExpr(e) => contains_exterior_struct_lit_inner(&e.base()?), + AwaitExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?), + PrefixExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?), + CastExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?), + FieldExpr(e) => contains_exterior_struct_lit_inner(&e.expr()?), + MethodCallExpr(e) => contains_exterior_struct_lit_inner(&e.receiver()?), + + _ => None, + } + } + } + + /// Returns true if self is one of `return`, `break`, `continue` or `yield` with **no associated value**. + fn is_ret_like_with_no_value(&self) -> bool { + use Expr::*; + + match self { + ReturnExpr(e) => e.expr().is_none(), + BreakExpr(e) => e.expr().is_none(), + ContinueExpr(_) => true, + YieldExpr(e) => e.expr().is_none(), + _ => false, + } + } + + fn is_ordered_before(&self, other: &Expr) -> bool { + use Expr::*; + + return order(self) < order(other); + + /// Returns text range that can be used to compare two expression for order (which goes first). + fn order(this: &Expr) -> rowan::TextSize { + // For non-paren-like operators: get the operator itself + let token = match this { + RangeExpr(e) => e.op_token(), + BinExpr(e) => e.op_token(), + CastExpr(e) => e.as_token(), + FieldExpr(e) => e.dot_token(), + AwaitExpr(e) => e.dot_token(), + BoxExpr(e) => e.box_token(), + BreakExpr(e) => e.break_token(), + CallExpr(e) => e.arg_list().and_then(|args| args.l_paren_token()), + ClosureExpr(e) => e.param_list().and_then(|params| params.l_paren_token()), + ContinueExpr(e) => e.continue_token(), + IndexExpr(e) => e.l_brack_token(), + MethodCallExpr(e) => e.dot_token(), + PrefixExpr(e) => e.op_token(), + RefExpr(e) => e.amp_token(), + ReturnExpr(e) => e.return_token(), + TryExpr(e) => e.question_mark_token(), + YieldExpr(e) => e.yield_token(), + LetExpr(e) => e.let_token(), + + ArrayExpr(_) | TupleExpr(_) | Literal(_) | PathExpr(_) | ParenExpr(_) + | IfExpr(_) | WhileExpr(_) | ForExpr(_) | LoopExpr(_) | MatchExpr(_) + | BlockExpr(_) | RecordExpr(_) | UnderscoreExpr(_) | MacroExpr(_) => None, + }; + + token.map(|t| t.text_range()).unwrap_or_else(|| this.syntax().text_range()).start() + } + } + + fn child_is_followed_by_a_block(&self) -> bool { + use Expr::*; + + match self { + ArrayExpr(_) | AwaitExpr(_) | BlockExpr(_) | CallExpr(_) | CastExpr(_) + | ClosureExpr(_) | FieldExpr(_) | IndexExpr(_) | Literal(_) | LoopExpr(_) + | MacroExpr(_) | MethodCallExpr(_) | ParenExpr(_) | PathExpr(_) | RecordExpr(_) + | TryExpr(_) | TupleExpr(_) | UnderscoreExpr(_) => false, + + // For BinExpr and RangeExpr this is technically wrong -- the child can be on the left... + BinExpr(_) | RangeExpr(_) | BoxExpr(_) | BreakExpr(_) | ContinueExpr(_) + | PrefixExpr(_) | RefExpr(_) | ReturnExpr(_) | YieldExpr(_) | LetExpr(_) => self + .syntax() + .parent() + .and_then(Expr::cast) + .map(|e| e.child_is_followed_by_a_block()) + .unwrap_or(false), - Some(prec) + ForExpr(_) | IfExpr(_) | MatchExpr(_) | WhileExpr(_) => true, + } } } |