Merge #9890

9890: internal: refactor binary operator handling r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <[email protected]>

Author: bors[bot]

crates/hir_def/src/body/lower.rs

@@ -27,9 +27,8 @@ use crate::{
     builtin_type::{BuiltinFloat, BuiltinInt, BuiltinUint},
     db::DefDatabase,
     expr::{
-        dummy_expr_id, ArithOp, Array, BinaryOp, BindingAnnotation, CmpOp, Expr, ExprId, Label,
-        LabelId, Literal, LogicOp, MatchArm, MatchGuard, Ordering, Pat, PatId, RecordFieldPat,
-        RecordLitField, Statement,
+        dummy_expr_id, Array, BindingAnnotation, Expr, ExprId, Label, LabelId, Literal, MatchArm,
+        MatchGuard, Pat, PatId, RecordFieldPat, RecordLitField, Statement,
     },
     intern::Interned,
     item_scope::BuiltinShadowMode,
@@ -509,7 +508,7 @@ impl ExprCollector<'_> {
             ast::Expr::BinExpr(e) => {
                 let lhs = self.collect_expr_opt(e.lhs());
                 let rhs = self.collect_expr_opt(e.rhs());
-                let op = e.op_kind().map(BinaryOp::from);
+                let op = e.op_kind();
                 self.alloc_expr(Expr::BinaryOp { lhs, rhs, op }, syntax_ptr)
             }
             ast::Expr::TupleExpr(e) => {
@@ -954,50 +953,6 @@ impl ExprCollector<'_> {
     }
 }
 
-impl From<ast::BinOp> for BinaryOp {
-    fn from(ast_op: ast::BinOp) -> Self {
-        match ast_op {
-            ast::BinOp::BooleanOr => BinaryOp::LogicOp(LogicOp::Or),
-            ast::BinOp::BooleanAnd => BinaryOp::LogicOp(LogicOp::And),
-            ast::BinOp::EqualityTest => BinaryOp::CmpOp(CmpOp::Eq { negated: false }),
-            ast::BinOp::NegatedEqualityTest => BinaryOp::CmpOp(CmpOp::Eq { negated: true }),
-            ast::BinOp::LesserEqualTest => {
-                BinaryOp::CmpOp(CmpOp::Ord { ordering: Ordering::Less, strict: false })
-            }
-            ast::BinOp::GreaterEqualTest => {
-                BinaryOp::CmpOp(CmpOp::Ord { ordering: Ordering::Greater, strict: false })
-            }
-            ast::BinOp::LesserTest => {
-                BinaryOp::CmpOp(CmpOp::Ord { ordering: Ordering::Less, strict: true })
-            }
-            ast::BinOp::GreaterTest => {
-                BinaryOp::CmpOp(CmpOp::Ord { ordering: Ordering::Greater, strict: true })
-            }
-            ast::BinOp::Addition => BinaryOp::ArithOp(ArithOp::Add),
-            ast::BinOp::Multiplication => BinaryOp::ArithOp(ArithOp::Mul),
-            ast::BinOp::Subtraction => BinaryOp::ArithOp(ArithOp::Sub),
-            ast::BinOp::Division => BinaryOp::ArithOp(ArithOp::Div),
-            ast::BinOp::Remainder => BinaryOp::ArithOp(ArithOp::Rem),
-            ast::BinOp::LeftShift => BinaryOp::ArithOp(ArithOp::Shl),
-            ast::BinOp::RightShift => BinaryOp::ArithOp(ArithOp::Shr),
-            ast::BinOp::BitwiseXor => BinaryOp::ArithOp(ArithOp::BitXor),
-            ast::BinOp::BitwiseOr => BinaryOp::ArithOp(ArithOp::BitOr),
-            ast::BinOp::BitwiseAnd => BinaryOp::ArithOp(ArithOp::BitAnd),
-            ast::BinOp::Assignment => BinaryOp::Assignment { op: None },
-            ast::BinOp::AddAssign => BinaryOp::Assignment { op: Some(ArithOp::Add) },
-            ast::BinOp::DivAssign => BinaryOp::Assignment { op: Some(ArithOp::Div) },
-            ast::BinOp::MulAssign => BinaryOp::Assignment { op: Some(ArithOp::Mul) },
-            ast::BinOp::RemAssign => BinaryOp::Assignment { op: Some(ArithOp::Rem) },
-            ast::BinOp::ShlAssign => BinaryOp::Assignment { op: Some(ArithOp::Shl) },
-            ast::BinOp::ShrAssign => BinaryOp::Assignment { op: Some(ArithOp::Shr) },
-            ast::BinOp::SubAssign => BinaryOp::Assignment { op: Some(ArithOp::Sub) },
-            ast::BinOp::BitOrAssign => BinaryOp::Assignment { op: Some(ArithOp::BitOr) },
-            ast::BinOp::BitAndAssign => BinaryOp::Assignment { op: Some(ArithOp::BitAnd) },
-            ast::BinOp::BitXorAssign => BinaryOp::Assignment { op: Some(ArithOp::BitXor) },
-        }
-    }
-}
-
 impl From<ast::LiteralKind> for Literal {
     fn from(ast_lit_kind: ast::LiteralKind) -> Self {
         match ast_lit_kind {

crates/hir_def/src/expr.rs

@@ -14,7 +14,6 @@
 
 use hir_expand::name::Name;
 use la_arena::{Idx, RawIdx};
-use syntax::ast::RangeOp;
 
 use crate::{
     builtin_type::{BuiltinFloat, BuiltinInt, BuiltinUint},
@@ -24,6 +23,8 @@ use crate::{
     BlockId,
 };
 
+pub use syntax::ast::{ArithOp, BinaryOp, CmpOp, LogicOp, Ordering, RangeOp, UnaryOp};
+
 pub type ExprId = Idx<Expr>;
 pub(crate) fn dummy_expr_id() -> ExprId {
     ExprId::from_raw(RawIdx::from(!0))
@@ -179,47 +180,6 @@ pub enum Expr {
     Literal(Literal),
 }
 
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
-pub enum BinaryOp {
-    LogicOp(LogicOp),
-    ArithOp(ArithOp),
-    CmpOp(CmpOp),
-    Assignment { op: Option<ArithOp> },
-}
-
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
-pub enum LogicOp {
-    And,
-    Or,
-}
-
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
-pub enum CmpOp {
-    Eq { negated: bool },
-    Ord { ordering: Ordering, strict: bool },
-}
-
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
-pub enum Ordering {
-    Less,
-    Greater,
-}
-
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
-pub enum ArithOp {
-    Add,
-    Mul,
-    Sub,
-    Div,
-    Rem,
-    Shl,
-    Shr,
-    BitXor,
-    BitOr,
-    BitAnd,
-}
-
-pub use syntax::ast::PrefixOp as UnaryOp;
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub enum Array {
     ElementList(Vec<ExprId>),

crates/ide/src/syntax_highlighting/highlight.rs

@@ -126,7 +126,7 @@ pub(super) fn element(
                 let ty = sema.type_of_expr(&expr)?.original;
                 if ty.is_raw_ptr() {
                     HlTag::Operator(HlOperator::Other) | HlMod::Unsafe
-                } else if let Some(ast::PrefixOp::Deref) = prefix_expr.op_kind() {
+                } else if let Some(ast::UnaryOp::Deref) = prefix_expr.op_kind() {
                     HlOperator::Other.into()
                 } else {
                     HlPunct::Other.into()

crates/ide_assists/src/handlers/apply_demorgan.rs

@@ -19,14 +19,20 @@ use crate::{utils::invert_boolean_expression, AssistContext, AssistId, AssistKin
 // ->
 // ```
 // fn main() {
-//     if !(x == 4 && !(y < 3.14)) {}
+//     if !(x == 4 && y >= 3.14) {}
 // }
 // ```
 pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
     let expr = ctx.find_node_at_offset::<ast::BinExpr>()?;
     let op = expr.op_kind()?;
     let op_range = expr.op_token()?.text_range();
-    let opposite_op = opposite_logic_op(op)?;
+
+    let opposite_op = match op {
+        ast::BinaryOp::LogicOp(ast::LogicOp::And) => "||",
+        ast::BinaryOp::LogicOp(ast::LogicOp::Or) => "&&",
+        _ => return None,
+    };
+
     let cursor_in_range = op_range.contains_range(ctx.frange.range);
     if !cursor_in_range {
         return None;
@@ -85,7 +91,7 @@ pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<(
                 .and_then(|paren_expr| paren_expr.syntax().parent())
                 .and_then(ast::PrefixExpr::cast)
                 .and_then(|prefix_expr| {
-                    if prefix_expr.op_kind().unwrap() == ast::PrefixOp::Not {
+                    if prefix_expr.op_kind().unwrap() == ast::UnaryOp::Not {
                         Some(prefix_expr)
                     } else {
                         None
@@ -99,7 +105,7 @@ pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<(
             if let Some(paren_expr) = paren_expr {
                 for term in terms {
                     let range = term.syntax().text_range();
-                    let not_term = invert_boolean_expression(&ctx.sema, term);
+                    let not_term = invert_boolean_expression(term);
 
                     edit.replace(range, not_term.syntax().text());
                 }
@@ -114,37 +120,28 @@ pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<(
             } else {
                 if let Some(lhs) = terms.pop_front() {
                     let lhs_range = lhs.syntax().text_range();
-                    let not_lhs = invert_boolean_expression(&ctx.sema, lhs);
+                    let not_lhs = invert_boolean_expression(lhs);
 
                     edit.replace(lhs_range, format!("!({}", not_lhs.syntax().text()));
                 }
 
                 if let Some(rhs) = terms.pop_back() {
                     let rhs_range = rhs.syntax().text_range();
-                    let not_rhs = invert_boolean_expression(&ctx.sema, rhs);
+                    let not_rhs = invert_boolean_expression(rhs);
 
                     edit.replace(rhs_range, format!("{})", not_rhs.syntax().text()));
                 }
 
                 for term in terms {
                     let term_range = term.syntax().text_range();
-                    let not_term = invert_boolean_expression(&ctx.sema, term);
+                    let not_term = invert_boolean_expression(term);
                     edit.replace(term_range, not_term.syntax().text());
                 }
             }
         },
     )
 }
 
-// Return the opposite text for a given logical operator, if it makes sense
-fn opposite_logic_op(kind: ast::BinOp) -> Option<&'static str> {
-    match kind {
-        ast::BinOp::BooleanOr => Some("&&"),
-        ast::BinOp::BooleanAnd => Some("||"),
-        _ => None,
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use crate::tests::{check_assist, check_assist_not_applicable};
@@ -156,40 +153,12 @@ mod tests {
         check_assist(
             apply_demorgan,
             r#"
-//- minicore: ord, derive
-#[derive(PartialEq, Eq, PartialOrd, Ord)]
-struct S;
-
-fn f() {
-    S < S &&$0 S <= S
-}
-"#,
-            r#"
-#[derive(PartialEq, Eq, PartialOrd, Ord)]
-struct S;
-
-fn f() {
-    !(S >= S || S > S)
-}
-"#,
-        );
-
-        check_assist(
-            apply_demorgan,
-            r#"
-//- minicore: ord, derive
 struct S;
-
-fn f() {
-    S < S &&$0 S <= S
-}
+fn f() { S < S &&$0 S <= S }
 "#,
             r#"
 struct S;
-
-fn f() {
-    !(!(S < S) || !(S <= S))
-}
+fn f() { !(S >= S || S > S) }
 "#,
         );
     }
@@ -199,39 +168,12 @@ fn f() {
         check_assist(
             apply_demorgan,
             r#"
-//- minicore: ord, derive
-#[derive(PartialEq, Eq, PartialOrd, Ord)]
-struct S;
-
-fn f() {
-    S > S &&$0 S >= S
-}
-"#,
-            r#"
-#[derive(PartialEq, Eq, PartialOrd, Ord)]
-struct S;
-
-fn f() {
-    !(S <= S || S < S)
-}
-"#,
-        );
-        check_assist(
-            apply_demorgan,
-            r#"
-//- minicore: ord, derive
 struct S;
-
-fn f() {
-    S > S &&$0 S >= S
-}
+fn f() { S > S &&$0 S >= S }
 "#,
             r#"
 struct S;
-
-fn f() {
-    !(!(S > S) || !(S >= S))
-}
+fn f() { !(S <= S || S < S) }
 "#,
         );
     }

crates/ide_assists/src/handlers/convert_bool_then.rs

@@ -97,7 +97,7 @@ pub(crate) fn convert_if_to_bool_then(acc: &mut Assists, ctx: &AssistContext) ->
                 e => e,
             };
 
-            let cond = if invert_cond { invert_boolean_expression(&ctx.sema, cond) } else { cond };
+            let cond = if invert_cond { invert_boolean_expression(cond) } else { cond };
             let arg_list = make::arg_list(Some(make::expr_closure(None, closure_body)));
             let mcall = make::expr_method_call(cond, make::name_ref("then"), arg_list);
             builder.replace(target, mcall.to_string());

crates/ide_assists/src/handlers/early_return.rs

@@ -115,7 +115,7 @@ pub(crate) fn convert_to_guarded_return(acc: &mut Assists, ctx: &AssistContext)
                     let new_expr = {
                         let then_branch =
                             make::block_expr(once(make::expr_stmt(early_expression).into()), None);
-                        let cond = invert_boolean_expression(&ctx.sema, cond_expr);
+                        let cond = invert_boolean_expression(cond_expr);
                         make::expr_if(make::condition(cond, None), then_branch, None)
                             .indent(if_indent_level)
                     };

crates/ide_assists/src/handlers/extract_function.rs

@@ -892,7 +892,7 @@ fn expr_require_exclusive_access(ctx: &AssistContext, expr: &ast::Expr) -> Optio
     let parent = expr.syntax().parent()?;
 
     if let Some(bin_expr) = ast::BinExpr::cast(parent.clone()) {
-        if bin_expr.op_kind()?.is_assignment() {
+        if matches!(bin_expr.op_kind()?, ast::BinaryOp::Assignment { .. }) {
             return Some(bin_expr.lhs()?.syntax() == expr.syntax());
         }
         return Some(false);

crates/ide_assists/src/handlers/flip_binexpr.rs

@@ -1,4 +1,4 @@
-use syntax::ast::{AstNode, BinExpr, BinOp};
+use syntax::ast::{self, AstNode, BinExpr};
 
 use crate::{AssistContext, AssistId, AssistKind, Assists};
 
@@ -56,14 +56,19 @@ enum FlipAction {
     DontFlip,
 }
 
-impl From<BinOp> for FlipAction {
-    fn from(op_kind: BinOp) -> Self {
+impl From<ast::BinaryOp> for FlipAction {
+    fn from(op_kind: ast::BinaryOp) -> Self {
         match op_kind {
-            kind if kind.is_assignment() => FlipAction::DontFlip,
-            BinOp::GreaterTest => FlipAction::FlipAndReplaceOp("<"),
-            BinOp::GreaterEqualTest => FlipAction::FlipAndReplaceOp("<="),
-            BinOp::LesserTest => FlipAction::FlipAndReplaceOp(">"),
-            BinOp::LesserEqualTest => FlipAction::FlipAndReplaceOp(">="),
+            ast::BinaryOp::Assignment { .. } => FlipAction::DontFlip,
+            ast::BinaryOp::CmpOp(ast::CmpOp::Ord { ordering, strict }) => {
+                let rev_op = match (ordering, strict) {
+                    (ast::Ordering::Less, true) => ">",
+                    (ast::Ordering::Less, false) => ">=",
+                    (ast::Ordering::Greater, true) => "<",
+                    (ast::Ordering::Greater, false) => "<=",
+                };
+                FlipAction::FlipAndReplaceOp(rev_op)
+            }
             _ => FlipAction::Flip,
         }
     }

crates/ide_assists/src/handlers/invert_if.rs

@@ -47,7 +47,7 @@ pub(crate) fn invert_if(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
     };
 
     acc.add(AssistId("invert_if", AssistKind::RefactorRewrite), "Invert if", if_range, |edit| {
-        let flip_cond = invert_boolean_expression(&ctx.sema, cond.clone());
+        let flip_cond = invert_boolean_expression(cond.clone());
         edit.replace_ast(cond, flip_cond);
 
         let else_node = else_block.syntax();