-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
feature: Add Replace if let else with Option::map_or_else
assist
#16031
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works fine, however we don't check for control flow like break
, return
or continue
. I think that's fine.
I want to also implement the reverse but I had trouble implementing it.
if scrutinee_to_be_expr.syntax().text() != expr.syntax().text() { | ||
return None; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is needed, stolen from
rust-analyzer/crates/ide-assists/src/handlers/replace_if_let_with_match.rs
Lines 82 to 88 in b2f6fd4
// FIXME: If one `let` is wrapped in parentheses and the second is not, | |
// we'll exit here. | |
if scrutinee_to_be_expr.syntax().text() != expr.syntax().text() { | |
// Only if all condition expressions are equal we can merge them into a match | |
return None; | |
} | |
pat_seen = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this applies here
Replace if let else with
Option::map_or_else` assistReplace if let else with
Option::map_or_else` assist
this doesn't seem to handle if let Some(ref field) = self.field {
field.eq_ignore_ascii_case(other)
} else $0if let Some(field2) = self.field2 {
self.something()
} else {
false
} well, it becomes if let Some(ref field) = self.field {
field.eq_ignore_ascii_case(other)
} else self.field2.map_or_else(|| {
false
}, |field2| {
self.something()
}) which is obviously wrong. |
We also need to prevent let _ = if $0let Some(ref mut field) = self.field2 {
*field = "".to_owned();
field == other
} else {
true
} or make it change to let _ = self.field2.as_mut().map_or_else(|| {
true
}, |field| {
*field = "".to_owned();
field == other
}) however I can't figure out how to get the bindings inside the pattern |
Replace if let else with
Option::map_or_else` assistReplace if let else with Option::map_or_else
assist
} | ||
pat = tuple_pat.fields().next()?; | ||
} | ||
None => return None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we can just ?
on the result of single_let
if scrutinee_to_be_expr.syntax().text() != expr.syntax().text() { | ||
return None; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this applies here
It's fine if we just don't offer the assist in that case for the time being. better than creating these syntax errors. |
You can get the binding for that here https://github.com/rust-lang/rust-analyzer/pull/16031/files#diff-74f54460a4549ebe1f552b2513663b9eac476eef5399254c8895d6177094baa0R70-R72
|
Closing due to inactivity, feel free to come back to this if you find the time |
Implements the
option_if_let_else
clippy lint.Screen.Recording.2023-12-05.at.18.30.03.webm