Skip to content

Commit beca92b

Browse files
matkladmatklad
committed
internal: make invert binary op more robust
Previously, we only inverted comparison operators (< and the like) if the type implemented Ord. This doesn't make sense: if `<` works, then `>=` will work as well! Extra semantic checks greatly reduce robustness and predictability of the assist, it's better to keep things simple.
1 parent 49dbb74 commit beca92b

File tree

6 files changed

+22
-109
lines changed

6 files changed

+22
-109
lines changed

‎crates/ide_assists/src/handlers/apply_demorgan.rs

Lines changed: 9 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ use crate::{utils::invert_boolean_expression, AssistContext, AssistId, AssistKin
1919
// ->
2020
// ```
2121
// fn main() {
22-
// if !(x == 4 && !(y < 3.14)) {}
22+
// if !(x == 4 && y >= 3.14) {}
2323
// }
2424
// ```
2525
pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
@@ -99,7 +99,7 @@ pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<(
9999
if let Some(paren_expr) = paren_expr {
100100
for term in terms {
101101
let range = term.syntax().text_range();
102-
let not_term = invert_boolean_expression(&ctx.sema, term);
102+
let not_term = invert_boolean_expression(term);
103103

104104
edit.replace(range, not_term.syntax().text());
105105
}
@@ -114,21 +114,21 @@ pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext) -> Option<(
114114
} else {
115115
if let Some(lhs) = terms.pop_front() {
116116
let lhs_range = lhs.syntax().text_range();
117-
let not_lhs = invert_boolean_expression(&ctx.sema, lhs);
117+
let not_lhs = invert_boolean_expression(lhs);
118118

119119
edit.replace(lhs_range, format!("!({}", not_lhs.syntax().text()));
120120
}
121121

122122
if let Some(rhs) = terms.pop_back() {
123123
let rhs_range = rhs.syntax().text_range();
124-
let not_rhs = invert_boolean_expression(&ctx.sema, rhs);
124+
let not_rhs = invert_boolean_expression(rhs);
125125

126126
edit.replace(rhs_range, format!("{})", not_rhs.syntax().text()));
127127
}
128128

129129
for term in terms {
130130
let term_range = term.syntax().text_range();
131-
let not_term = invert_boolean_expression(&ctx.sema, term);
131+
let not_term = invert_boolean_expression(term);
132132
edit.replace(term_range, not_term.syntax().text());
133133
}
134134
}
@@ -156,40 +156,12 @@ mod tests {
156156
check_assist(
157157
apply_demorgan,
158158
r#"
159-
//- minicore: ord, derive
160-
#[derive(PartialEq, Eq, PartialOrd, Ord)]
161159
struct S;
162-
163-
fn f() {
164-
S < S &&$0 S <= S
165-
}
166-
"#,
167-
r#"
168-
#[derive(PartialEq, Eq, PartialOrd, Ord)]
169-
struct S;
170-
171-
fn f() {
172-
!(S >= S || S > S)
173-
}
174-
"#,
175-
);
176-
177-
check_assist(
178-
apply_demorgan,
179-
r#"
180-
//- minicore: ord, derive
181-
struct S;
182-
183-
fn f() {
184-
S < S &&$0 S <= S
185-
}
160+
fn f() { S < S &&$0 S <= S }
186161
"#,
187162
r#"
188163
struct S;
189-
190-
fn f() {
191-
!(!(S < S) || !(S <= S))
192-
}
164+
fn f() { !(S >= S || S > S) }
193165
"#,
194166
);
195167
}
@@ -199,39 +171,12 @@ fn f() {
199171
check_assist(
200172
apply_demorgan,
201173
r#"
202-
//- minicore: ord, derive
203-
#[derive(PartialEq, Eq, PartialOrd, Ord)]
204174
struct S;
205-
206-
fn f() {
207-
S > S &&$0 S >= S
208-
}
175+
fn f() { S > S &&$0 S >= S }
209176
"#,
210177
r#"
211-
#[derive(PartialEq, Eq, PartialOrd, Ord)]
212178
struct S;
213-
214-
fn f() {
215-
!(S <= S || S < S)
216-
}
217-
"#,
218-
);
219-
check_assist(
220-
apply_demorgan,
221-
r#"
222-
//- minicore: ord, derive
223-
struct S;
224-
225-
fn f() {
226-
S > S &&$0 S >= S
227-
}
228-
"#,
229-
r#"
230-
struct S;
231-
232-
fn f() {
233-
!(!(S > S) || !(S >= S))
234-
}
179+
fn f() { !(S <= S || S < S) }
235180
"#,
236181
);
237182
}

‎crates/ide_assists/src/handlers/convert_bool_then.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ pub(crate) fn convert_if_to_bool_then(acc: &mut Assists, ctx: &AssistContext) ->
9797
e => e,
9898
};
9999

100-
let cond = if invert_cond { invert_boolean_expression(&ctx.sema, cond) } else { cond };
100+
let cond = if invert_cond { invert_boolean_expression(cond) } else { cond };
101101
let arg_list = make::arg_list(Some(make::expr_closure(None, closure_body)));
102102
let mcall = make::expr_method_call(cond, make::name_ref("then"), arg_list);
103103
builder.replace(target, mcall.to_string());

‎crates/ide_assists/src/handlers/early_return.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ pub(crate) fn convert_to_guarded_return(acc: &mut Assists, ctx: &AssistContext)
115115
let new_expr = {
116116
let then_branch =
117117
make::block_expr(once(make::expr_stmt(early_expression).into()), None);
118-
let cond = invert_boolean_expression(&ctx.sema, cond_expr);
118+
let cond = invert_boolean_expression(cond_expr);
119119
make::expr_if(make::condition(cond, None), then_branch, None)
120120
.indent(if_indent_level)
121121
};

‎crates/ide_assists/src/handlers/invert_if.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ pub(crate) fn invert_if(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
4747
};
4848

4949
acc.add(AssistId("invert_if", AssistKind::RefactorRewrite), "Invert if", if_range, |edit| {
50-
let flip_cond = invert_boolean_expression(&ctx.sema, cond.clone());
50+
let flip_cond = invert_boolean_expression(cond.clone());
5151
edit.replace_ast(cond, flip_cond);
5252

5353
let else_node = else_block.syntax();

‎crates/ide_assists/src/tests/generated.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ fn main() {
151151
"#####,
152152
r#####"
153153
fn main() {
154-
if !(x == 4 && !(y < 3.14)) {}
154+
if !(x == 4 && y >= 3.14) {}
155155
}
156156
"#####,
157157
)

‎crates/ide_assists/src/utils.rs

Lines changed: 9 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,8 @@ mod gen_trait_fn_body;
55

66
use std::ops;
77

8-
use hir::{Adt, HasSource, Semantics};
9-
use ide_db::{
10-
helpers::{FamousDefs, SnippetCap},
11-
path_transform::PathTransform,
12-
RootDatabase,
13-
};
8+
use hir::{Adt, HasSource};
9+
use ide_db::{helpers::SnippetCap, path_transform::PathTransform, RootDatabase};
1410
use itertools::Itertools;
1511
use stdx::format_to;
1612
use syntax::{
@@ -207,31 +203,19 @@ pub(crate) fn vis_offset(node: &SyntaxNode) -> TextSize {
207203
.unwrap_or_else(|| node.text_range().start())
208204
}
209205

210-
pub(crate) fn invert_boolean_expression(
211-
sema: &Semantics<RootDatabase>,
212-
expr: ast::Expr,
213-
) -> ast::Expr {
214-
invert_special_case(sema, &expr).unwrap_or_else(|| make::expr_prefix(T![!], expr))
206+
pub(crate) fn invert_boolean_expression(expr: ast::Expr) -> ast::Expr {
207+
invert_special_case(&expr).unwrap_or_else(|| make::expr_prefix(T![!], expr))
215208
}
216209

217-
fn invert_special_case(sema: &Semantics<RootDatabase>, expr: &ast::Expr) -> Option<ast::Expr> {
210+
fn invert_special_case(expr: &ast::Expr) -> Option<ast::Expr> {
218211
match expr {
219212
ast::Expr::BinExpr(bin) => match bin.op_kind()? {
220213
ast::BinOp::NegatedEqualityTest => bin.replace_op(T![==]).map(|it| it.into()),
221214
ast::BinOp::EqualityTest => bin.replace_op(T![!=]).map(|it| it.into()),
222-
// Swap `<` with `>=`, `<=` with `>`, ... if operands `impl Ord`
223-
ast::BinOp::LesserTest if bin_impls_ord(sema, bin) => {
224-
bin.replace_op(T![>=]).map(|it| it.into())
225-
}
226-
ast::BinOp::LesserEqualTest if bin_impls_ord(sema, bin) => {
227-
bin.replace_op(T![>]).map(|it| it.into())
228-
}
229-
ast::BinOp::GreaterTest if bin_impls_ord(sema, bin) => {
230-
bin.replace_op(T![<=]).map(|it| it.into())
231-
}
232-
ast::BinOp::GreaterEqualTest if bin_impls_ord(sema, bin) => {
233-
bin.replace_op(T![<]).map(|it| it.into())
234-
}
215+
ast::BinOp::LesserTest => bin.replace_op(T![>=]).map(|it| it.into()),
216+
ast::BinOp::LesserEqualTest => bin.replace_op(T![>]).map(|it| it.into()),
217+
ast::BinOp::GreaterTest => bin.replace_op(T![<=]).map(|it| it.into()),
218+
ast::BinOp::GreaterEqualTest => bin.replace_op(T![<]).map(|it| it.into()),
235219
// Parenthesize other expressions before prefixing `!`
236220
_ => Some(make::expr_prefix(T![!], make::expr_paren(expr.clone()))),
237221
},
@@ -267,22 +251,6 @@ fn invert_special_case(sema: &Semantics<RootDatabase>, expr: &ast::Expr) -> Opti
267251
}
268252
}
269253

270-
fn bin_impls_ord(sema: &Semantics<RootDatabase>, bin: &ast::BinExpr) -> bool {
271-
match (
272-
bin.lhs().and_then(|lhs| sema.type_of_expr(&lhs)).map(hir::TypeInfo::adjusted),
273-
bin.rhs().and_then(|rhs| sema.type_of_expr(&rhs)).map(hir::TypeInfo::adjusted),
274-
) {
275-
(Some(lhs_ty), Some(rhs_ty)) if lhs_ty == rhs_ty => {
276-
let krate = sema.scope(bin.syntax()).module().map(|it| it.krate());
277-
let ord_trait = FamousDefs(sema, krate).core_cmp_Ord();
278-
ord_trait.map_or(false, |ord_trait| {
279-
lhs_ty.autoderef(sema.db).any(|ty| ty.impls_trait(sema.db, ord_trait, &[]))
280-
})
281-
}
282-
_ => false,
283-
}
284-
}
285-
286254
pub(crate) fn next_prev() -> impl Iterator<Item = Direction> {
287255
[Direction::Next, Direction::Prev].iter().copied()
288256
}

0 commit comments

Comments
 (0)