Skip to content

Parenthesize composite if condition before inverting in invert-if assist #6894

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Parenthesize composite if condition before inverting in invert-if assist #6894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions crates/assists/src/handlers/invert_if.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ mod tests {

use crate::tests::{check_assist, check_assist_not_applicable};

#[test]
fn invert_if_composite_condition() {
check_assist(
invert_if,
"fn f() { i<|>f x == 3 || x == 4 || x == 5 { 1 } else { 3 * 2 } }",
"fn f() { if !(x == 3 || x == 4 || x == 5) { 3 * 2 } else { 1 } }",
Copy link
Contributor

@matklad matklad Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this, I wonder if this ideally should be de-morganed? if x != 3 && x != 4 && x != 5?

Copy link
Contributor Author

@Jesse-Bakker Jesse-Bakker Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think whether de-morganing is desired depends heavily on the situation. One could argue that, when most (more than half?) of the parts of a composite condition are negative (if x != 3 && x != 4 && y == 5), it would be better to de-morgan when inverting (if x== 3 || x == 4 && y != 5). The situation in this test might also benefit from de-morganing, as it makes it clearer that this is a "x not in [3, 4, 5]" case. There might be other cases where de-morganing might make the code worse though. And in the end it also depends on the user's preferences.

I think it would be good to support it, but I'm not sure about what the best defaults are. I think adding parens might be less surprising to the user and automatic de-morganing could be opt-in through a config option, while manual de-morganing can be achieved through a separate code action (called "distribute ! over operands" or something like that). Not sure if that conforms to being the "least surprising" and "enable all features for better discovery" principles though.

Copy link
Contributor

@matklad matklad Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah, agree with the reasoning that distributing ! should be a separate action. We actually have it, but it it is a bit naive:

fn foo() {
    if !(x == 3 |<|>| y == 5 || z == 6) {}
}

fn foo() {
    if !(!(x != 3 && y != 5) || z == 6) {}
}

Copy link
Contributor Author

@Jesse-Bakker Jesse-Bakker Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case you cite, because you invoke the assist on the && operator, the current behaviour is actually what I would expect to happen. It would be nice if, when invoked on the prefixed !, the ! could be distributed over the entire parenthesized expression. Maybe the assist could even trigger on the if keyword, to allow applying De Morgan's law on the entire condition without the need for parentheses or losing the current behaviour.

Copy link
Contributor Author

@Jesse-Bakker Jesse-Bakker Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing that would be nice to have is if applying De Morgan's law twice would result in the initial expression:

Currently:
if x == 4 || y == 5 {} -> if !(x != 3 && y != 4) {} -> if !(!(x == 3 || y ==4)) {}

That simplification is something we would probably want to do in the invert-if assist as well.

)
}

#[test]
fn invert_if_remove_inequality() {
check_assist(
Expand Down
4 changes: 4 additions & 0 deletions crates/assists/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,10 @@ fn invert_special_case(expr: &ast::Expr) -> Option<ast::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())))
}
_ => None,
},
ast::Expr::MethodCallExpr(mce) => {
Expand Down
3 changes: 3 additions & 0 deletions crates/syntax/src/ast/make.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,9 @@ pub fn expr_method_call(receiver: ast::Expr, method: &str, arg_list: ast::ArgLis
pub fn expr_ref(expr: ast::Expr, exclusive: bool) -> ast::Expr {
expr_from_text(&if exclusive { format!("&mut {}", expr) } else { format!("&{}", expr) })
}
pub fn expr_paren(expr: ast::Expr) -> ast::Expr {
expr_from_text(&format!("({})", expr))
}
fn expr_from_text(text: &str) -> ast::Expr {
ast_from_text(&format!("const C: () = {};", text))
}
Expand Down