Merge #7719

7719: De Morgan's Law assist now correctly parenthesizes binary expressions. r=Veykril a=lbrande

Closes #7694 by parenthesizing binary expressions that are negated.

Co-authored-by: lbrande <lovbra00@gmail.com>
Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

Author: 3 people

crates/ide_assists/src/handlers/apply_demorgan.rs

@@ -7,18 +7,17 @@ use crate::{utils::invert_boolean_expression, AssistContext, AssistId, AssistKin
 // Apply https://en.wikipedia.org/wiki/De_Morgan%27s_laws[De Morgan's law].
 // This transforms expressions of the form `!l || !r` into `!(l && r)`.
 // This also works with `&&`. This assist can only be applied with the cursor
-// on either `||` or `&&`, with both operands being a negation of some kind.
-// This means something of the form `!x` or `x != y`.
+// on either `||` or `&&`.
 //
 // ```
 // fn main() {
-//     if x != 4 ||$0 !y {}
+//     if x != 4 ||$0 y < 3.14 {}
 // }
 // ```
 // ->
 // ```
 // fn main() {
-//     if !(x == 4 && y) {}
+//     if !(x == 4 && !(y < 3.14)) {}
 // }
 // ```
 pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
@@ -33,11 +32,11 @@ pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<(
 
     let lhs = expr.lhs()?;
     let lhs_range = lhs.syntax().text_range();
-    let not_lhs = invert_boolean_expression(lhs);
+    let not_lhs = invert_boolean_expression(&ctx.sema, lhs);
 
     let rhs = expr.rhs()?;
     let rhs_range = rhs.syntax().text_range();
-    let not_rhs = invert_boolean_expression(rhs);
+    let not_rhs = invert_boolean_expression(&ctx.sema, rhs);
 
     acc.add(
         AssistId("apply_demorgan", AssistKind::RefactorRewrite),
@@ -62,10 +61,77 @@ fn opposite_logic_op(kind: ast::BinOp) -> Option<&'static str> {
 
 #[cfg(test)]
 mod tests {
+    use ide_db::helpers::FamousDefs;
+
     use super::*;
 
     use crate::tests::{check_assist, check_assist_not_applicable};
 
+    const ORDABLE_FIXTURE: &'static str = r"
+//- /lib.rs deps:core crate:ordable
+struct NonOrderable;
+struct Orderable;
+impl core::cmp::Ord for Orderable {}
+";
+
+    fn check(ra_fixture_before: &str, ra_fixture_after: &str) {
+        let before = &format!(
+            "//- /main.rs crate:main deps:core,ordable\n{}\n{}{}",
+            ra_fixture_before,
+            FamousDefs::FIXTURE,
+            ORDABLE_FIXTURE
+        );
+        check_assist(apply_demorgan, before, &format!("{}\n", ra_fixture_after));
+    }
+
+    #[test]
+    fn demorgan_handles_leq() {
+        check(
+            r"use ordable::Orderable;
+fn f() {
+    Orderable < Orderable &&$0 Orderable <= Orderable
+}",
+            r"use ordable::Orderable;
+fn f() {
+    !(Orderable >= Orderable || Orderable > Orderable)
+}",
+        );
+        check(
+            r"use ordable::NonOrderable;
+fn f() {
+    NonOrderable < NonOrderable &&$0 NonOrderable <= NonOrderable
+}",
+            r"use ordable::NonOrderable;
+fn f() {
+    !(!(NonOrderable < NonOrderable) || !(NonOrderable <= NonOrderable))
+}",
+        );
+    }
+
+    #[test]
+    fn demorgan_handles_geq() {
+        check(
+            r"use ordable::Orderable;
+fn f() {
+    Orderable > Orderable &&$0 Orderable >= Orderable
+}",
+            r"use ordable::Orderable;
+fn f() {
+    !(Orderable <= Orderable || Orderable < Orderable)
+}",
+        );
+        check(
+            r"use ordable::NonOrderable;
+fn f() {
+    Orderable > Orderable &&$0 Orderable >= Orderable
+}",
+            r"use ordable::NonOrderable;
+fn f() {
+    !(!(Orderable > Orderable) || !(Orderable >= Orderable))
+}",
+        );
+    }
+
     #[test]
     fn demorgan_turns_and_into_or() {
         check_assist(apply_demorgan, "fn f() { !x &&$0 !x }", "fn f() { !(x || x) }")

crates/ide_assists/src/handlers/early_return.rs

@@ -111,7 +111,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(cond_expr);
+                        let cond = invert_boolean_expression(&ctx.sema, cond_expr);
                         make::expr_if(make::condition(cond, None), then_branch, None)
                             .indent(if_indent_level)
                     };

crates/ide_assists/src/handlers/invert_if.rs

@@ -50,7 +50,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(cond.clone());
+        let flip_cond = invert_boolean_expression(&ctx.sema, cond.clone());
         edit.replace_ast(cond, flip_cond);
 
         let else_node = else_block.syntax();

crates/ide_assists/src/tests/generated.rs

@@ -147,12 +147,12 @@ fn doctest_apply_demorgan() {
         "apply_demorgan",
         r#####"
 fn main() {
-    if x != 4 ||$0 !y {}
+    if x != 4 ||$0 y < 3.14 {}
 }
 "#####,
         r#####"
 fn main() {
-    if !(x == 4 && y) {}
+    if !(x == 4 && !(y < 3.14)) {}
 }
 "#####,
     )

crates/ide_assists/src/utils.rs

@@ -3,8 +3,11 @@
 use std::ops;
 
 use ast::TypeBoundsOwner;
-use hir::{Adt, HasSource};
-use ide_db::{helpers::SnippetCap, RootDatabase};
+use hir::{Adt, HasSource, Semantics};
+use ide_db::{
+    helpers::{FamousDefs, SnippetCap},
+    RootDatabase,
+};
 use itertools::Itertools;
 use stdx::format_to;
 use syntax::{
@@ -205,23 +208,36 @@ pub(crate) fn vis_offset(node: &SyntaxNode) -> TextSize {
         .unwrap_or_else(|| node.text_range().start())
 }
 
-pub(crate) fn invert_boolean_expression(expr: ast::Expr) -> ast::Expr {
-    if let Some(expr) = invert_special_case(&expr) {
+pub(crate) fn invert_boolean_expression(
+    sema: &Semantics<RootDatabase>,
+    expr: ast::Expr,
+) -> ast::Expr {
+    if let Some(expr) = invert_special_case(sema, &expr) {
         return expr;
     }
     make::expr_prefix(T![!], expr)
 }
 
-fn invert_special_case(expr: &ast::Expr) -> Option<ast::Expr> {
+fn invert_special_case(sema: &Semantics<RootDatabase>, expr: &ast::Expr) -> Option<ast::Expr> {
     match expr {
         ast::Expr::BinExpr(bin) => match bin.op_kind()? {
             ast::BinOp::NegatedEqualityTest => bin.replace_op(T![==]).map(|it| it.into()),
             ast::BinOp::EqualityTest => bin.replace_op(T![!=]).map(|it| it.into()),
-            // Parenthesize composite boolean expressions before prefixing `!`
-            ast::BinOp::BooleanAnd | ast::BinOp::BooleanOr => {
-                Some(make::expr_prefix(T![!], make::expr_paren(expr.clone())))
+            // Swap `<` with `>=`, `<=` with `>`, ... if operands `impl Ord`
+            ast::BinOp::LesserTest if bin_impls_ord(sema, bin) => {
+                bin.replace_op(T![>=]).map(|it| it.into())
+            }
+            ast::BinOp::LesserEqualTest if bin_impls_ord(sema, bin) => {
+                bin.replace_op(T![>]).map(|it| it.into())
+            }
+            ast::BinOp::GreaterTest if bin_impls_ord(sema, bin) => {
+                bin.replace_op(T![<=]).map(|it| it.into())
+            }
+            ast::BinOp::GreaterEqualTest if bin_impls_ord(sema, bin) => {
+                bin.replace_op(T![<]).map(|it| it.into())
             }
-            _ => None,
+            // Parenthesize other expressions before prefixing `!`
+            _ => Some(make::expr_prefix(T![!], make::expr_paren(expr.clone()))),
         },
         ast::Expr::MethodCallExpr(mce) => {
             let receiver = mce.receiver()?;
@@ -250,6 +266,22 @@ fn invert_special_case(expr: &ast::Expr) -> Option<ast::Expr> {
     }
 }
 
+fn bin_impls_ord(sema: &Semantics<RootDatabase>, bin: &ast::BinExpr) -> bool {
+    match (
+        bin.lhs().and_then(|lhs| sema.type_of_expr(&lhs)),
+        bin.rhs().and_then(|rhs| sema.type_of_expr(&rhs)),
+    ) {
+        (Some(lhs_ty), Some(rhs_ty)) if lhs_ty == rhs_ty => {
+            let krate = sema.scope(bin.syntax()).module().map(|it| it.krate());
+            let ord_trait = FamousDefs(sema, krate).core_cmp_Ord();
+            ord_trait.map_or(false, |ord_trait| {
+                lhs_ty.autoderef(sema.db).any(|ty| ty.impls_trait(sema.db, ord_trait, &[]))
+            })
+        }
+        _ => false,
+    }
+}
+
 pub(crate) fn next_prev() -> impl Iterator<Item = Direction> {
     [Direction::Next, Direction::Prev].iter().copied()
 }

crates/ide_db/src/helpers.rs

@@ -45,6 +45,10 @@ impl FamousDefs<'_, '_> {
         self.find_crate("core")
     }
 
+    pub fn core_cmp_Ord(&self) -> Option<Trait> {
+        self.find_trait("core:cmp:Ord")
+    }
+
     pub fn core_convert_From(&self) -> Option<Trait> {
         self.find_trait("core:convert:From")
     }

crates/ide_db/src/helpers/famous_defs_fixture.rs

@@ -1,5 +1,15 @@
 //- /libcore.rs crate:core
 //! Signatures of traits, types and functions from the core lib for use in tests.
+pub mod cmp {
+
+    pub trait Ord {
+        fn cmp(&self, other: &Self) -> Ordering;
+        fn max(self, other: Self) -> Self;
+        fn min(self, other: Self) -> Self;
+        fn clamp(self, min: Self, max: Self) -> Self;
+    }
+}
+
 pub mod convert {
     pub trait From<T> {
         fn from(t: T) -> Self;
@@ -109,6 +119,7 @@ pub mod option {
 
 pub mod prelude {
     pub use crate::{
+        cmp::Ord,
         convert::From,
         default::Default,
         iter::{IntoIterator, Iterator},

crates/syntax/src/ast/make.rs

@@ -527,8 +527,11 @@ pub mod tokens {
 
     use crate::{ast, AstNode, Parse, SourceFile, SyntaxKind::*, SyntaxToken};
 
-    pub(super) static SOURCE_FILE: Lazy<Parse<SourceFile>> =
-        Lazy::new(|| SourceFile::parse("const C: <()>::Item = (1 != 1, 2 == 2, !true, *p)\n;\n\n"));
+    pub(super) static SOURCE_FILE: Lazy<Parse<SourceFile>> = Lazy::new(|| {
+        SourceFile::parse(
+            "const C: <()>::Item = (1 != 1, 2 == 2, 3 < 3, 4 <= 4, 5 > 5, 6 >= 6, !true, *p)\n;\n\n",
+        )
+    });
 
     pub fn single_space() -> SyntaxToken {
         SOURCE_FILE