From f7a4a87de2840789e12722afc7df9f4db2db013c Mon Sep 17 00:00:00 2001 From: lbrande Date: Fri, 19 Feb 2021 14:48:07 +0100 Subject: De Morgan's Law assist now correctly parenthesizes binary expressions. --- crates/ide_assists/src/handlers/apply_demorgan.rs | 7 +++---- crates/ide_assists/src/tests/generated.rs | 4 ++-- crates/ide_assists/src/utils.rs | 7 ++----- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/crates/ide_assists/src/handlers/apply_demorgan.rs b/crates/ide_assists/src/handlers/apply_demorgan.rs index ed4d11455..e478ff2ce 100644 --- a/crates/ide_assists/src/handlers/apply_demorgan.rs +++ b/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 {} // } // ``` // -> // ``` // fn main() { -// if !(x == 4 && y) {} +// if !(x == 4 && !(y < 3)) {} // } // ``` pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { diff --git a/crates/ide_assists/src/tests/generated.rs b/crates/ide_assists/src/tests/generated.rs index 0516deaff..ee9c870e8 100644 --- a/crates/ide_assists/src/tests/generated.rs +++ b/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 {} } "#####, r#####" fn main() { - if !(x == 4 && y) {} + if !(x == 4 && !(y < 3)) {} } "#####, ) diff --git a/crates/ide_assists/src/utils.rs b/crates/ide_assists/src/utils.rs index 0074da741..cd026d432 100644 --- a/crates/ide_assists/src/utils.rs +++ b/crates/ide_assists/src/utils.rs @@ -217,11 +217,8 @@ fn invert_special_case(expr: &ast::Expr) -> Option { 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()))) - } - _ => None, + // Parenthesize other expressions before prefixing `!` + _ => Some(make::expr_prefix(T![!], make::expr_paren(expr.clone()))), }, ast::Expr::MethodCallExpr(mce) => { let receiver = mce.receiver()?; -- cgit v1.2.3 From 9db970ee082e315cfa04db163fe2e0268b618531 Mon Sep 17 00:00:00 2001 From: lbrande Date: Mon, 22 Feb 2021 16:23:42 +0100 Subject: De Morgan's Law assist now correctly inverts <, <=, >, >=. --- crates/ide_assists/src/handlers/apply_demorgan.rs | 8 ++-- crates/ide_assists/src/handlers/early_return.rs | 2 +- crates/ide_assists/src/handlers/invert_if.rs | 2 +- crates/ide_assists/src/tests/generated.rs | 4 +- crates/ide_assists/src/utils.rs | 50 ++++++++++++++++++++--- crates/ide_db/src/helpers.rs | 4 ++ crates/ide_db/src/helpers/famous_defs_fixture.rs | 11 +++++ 7 files changed, 68 insertions(+), 13 deletions(-) diff --git a/crates/ide_assists/src/handlers/apply_demorgan.rs b/crates/ide_assists/src/handlers/apply_demorgan.rs index e478ff2ce..3cd6699c3 100644 --- a/crates/ide_assists/src/handlers/apply_demorgan.rs +++ b/crates/ide_assists/src/handlers/apply_demorgan.rs @@ -11,13 +11,13 @@ use crate::{utils::invert_boolean_expression, AssistContext, AssistId, AssistKin // // ``` // fn main() { -// if x != 4 ||$0 y < 3 {} +// if x != 4 ||$0 y < 3.14 {} // } // ``` // -> // ``` // fn main() { -// if !(x == 4 && !(y < 3)) {} +// if !(x == 4 && !(y < 3.14)) {} // } // ``` pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { @@ -32,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), diff --git a/crates/ide_assists/src/handlers/early_return.rs b/crates/ide_assists/src/handlers/early_return.rs index 6b87c3c05..9e0918477 100644 --- a/crates/ide_assists/src/handlers/early_return.rs +++ b/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) }; diff --git a/crates/ide_assists/src/handlers/invert_if.rs b/crates/ide_assists/src/handlers/invert_if.rs index 5b69dafd4..b131dc205 100644 --- a/crates/ide_assists/src/handlers/invert_if.rs +++ b/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(); diff --git a/crates/ide_assists/src/tests/generated.rs b/crates/ide_assists/src/tests/generated.rs index ee9c870e8..701091a6b 100644 --- a/crates/ide_assists/src/tests/generated.rs +++ b/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 < 3 {} + if x != 4 ||$0 y < 3.14 {} } "#####, r#####" fn main() { - if !(x == 4 && !(y < 3)) {} + if !(x == 4 && !(y < 3.14)) {} } "#####, ) diff --git a/crates/ide_assists/src/utils.rs b/crates/ide_assists/src/utils.rs index cd026d432..38ed74673 100644 --- a/crates/ide_assists/src/utils.rs +++ b/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,18 +208,34 @@ 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, + 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 { +fn invert_special_case(sema: &Semantics, expr: &ast::Expr) -> Option { 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()), + // 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()) + } // Parenthesize other expressions before prefixing `!` _ => Some(make::expr_prefix(T![!], make::expr_paren(expr.clone()))), }, @@ -247,6 +266,27 @@ fn invert_special_case(expr: &ast::Expr) -> Option { } } +fn bin_impls_ord(sema: &Semantics, bin: &ast::BinExpr) -> bool { + if let (Some(lhs), Some(rhs)) = (bin.lhs(), bin.rhs()) { + return sema.type_of_expr(&lhs) == sema.type_of_expr(&rhs) + && impls_ord(sema, &lhs) + && impls_ord(sema, &rhs); + } + false +} + +fn impls_ord(sema: &Semantics, expr: &ast::Expr) -> bool { + let krate = sema.scope(expr.syntax()).module().map(|it| it.krate()); + let famous_defs = FamousDefs(&sema, krate); + + if let Some(ty) = sema.type_of_expr(expr) { + if let Some(ord_trait) = famous_defs.core_cmp_Ord() { + return ty.autoderef(sema.db).any(|ty| ty.impls_trait(sema.db, ord_trait, &[])); + } + } + false +} + pub(crate) fn next_prev() -> impl Iterator { [Direction::Next, Direction::Prev].iter().copied() } diff --git a/crates/ide_db/src/helpers.rs b/crates/ide_db/src/helpers.rs index bc7aee110..f9de8ce0e 100644 --- a/crates/ide_db/src/helpers.rs +++ b/crates/ide_db/src/helpers.rs @@ -45,6 +45,10 @@ impl FamousDefs<'_, '_> { self.find_crate("core") } + pub fn core_cmp_Ord(&self) -> Option { + self.find_trait("core:cmp:Ord") + } + pub fn core_convert_From(&self) -> Option { self.find_trait("core:convert:From") } diff --git a/crates/ide_db/src/helpers/famous_defs_fixture.rs b/crates/ide_db/src/helpers/famous_defs_fixture.rs index 5e88de64d..bb4e9666b 100644 --- a/crates/ide_db/src/helpers/famous_defs_fixture.rs +++ b/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 { 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}, -- cgit v1.2.3 From 694f7a7e9f90cc435afcaade23b5908728d17ed2 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 24 Feb 2021 11:42:32 +0100 Subject: Add tests for apply_demorgan --- crates/ide_assists/src/handlers/apply_demorgan.rs | 67 +++++++++++++++++++++++ crates/ide_assists/src/utils.rs | 27 ++++----- crates/syntax/src/ast/make.rs | 7 ++- 3 files changed, 83 insertions(+), 18 deletions(-) diff --git a/crates/ide_assists/src/handlers/apply_demorgan.rs b/crates/ide_assists/src/handlers/apply_demorgan.rs index 3cd6699c3..6997ea048 100644 --- a/crates/ide_assists/src/handlers/apply_demorgan.rs +++ b/crates/ide_assists/src/handlers/apply_demorgan.rs @@ -61,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) }") diff --git a/crates/ide_assists/src/utils.rs b/crates/ide_assists/src/utils.rs index 38ed74673..276792bc1 100644 --- a/crates/ide_assists/src/utils.rs +++ b/crates/ide_assists/src/utils.rs @@ -267,24 +267,19 @@ fn invert_special_case(sema: &Semantics, expr: &ast::Expr) -> Opti } fn bin_impls_ord(sema: &Semantics, bin: &ast::BinExpr) -> bool { - if let (Some(lhs), Some(rhs)) = (bin.lhs(), bin.rhs()) { - return sema.type_of_expr(&lhs) == sema.type_of_expr(&rhs) - && impls_ord(sema, &lhs) - && impls_ord(sema, &rhs); - } - false -} - -fn impls_ord(sema: &Semantics, expr: &ast::Expr) -> bool { - let krate = sema.scope(expr.syntax()).module().map(|it| it.krate()); - let famous_defs = FamousDefs(&sema, krate); - - if let Some(ty) = sema.type_of_expr(expr) { - if let Some(ord_trait) = famous_defs.core_cmp_Ord() { - return ty.autoderef(sema.db).any(|ty| ty.impls_trait(sema.db, ord_trait, &[])); + 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, } - false } pub(crate) fn next_prev() -> impl Iterator { diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index 5eee33545..b6c5de658 100644 --- a/crates/syntax/src/ast/make.rs +++ b/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> = - Lazy::new(|| SourceFile::parse("const C: <()>::Item = (1 != 1, 2 == 2, !true, *p)\n;\n\n")); + pub(super) static SOURCE_FILE: Lazy> = 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 -- cgit v1.2.3