Parenthesize composite if condition before inverting in invert-if assist #6894
Conversation
|
bors r=matklad |
| 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 } }", |
There was a problem hiding this comment.
Looking at this, I wonder if this ideally should be de-morganed? if x != 3 && x != 4 && x != 5?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) {}
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
6982: Remove parentheses when inverting `!(cond)` r=matklad a=Jesse-Bakker Followup to #6894 When inverting a composite condition twice, the parentheses were left. This also removes those unnecessary parentheses when applying the invert-if assist. Co-authored-by: Jesse Bakker <github@jessebakker.com>
3 participants
Fixes #6867