Add lint manual_option_folding
#11581
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @giraffate (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #11527) made this pull request unmergeable. Please resolve the merge conflicts. |
sjwang05
force-pushed
the
manual-option-folding
branch
from
6af6808 to
93f2de3
Compare
|
☔ The latest upstream changes (presumably #11791) made this pull request unmergeable. Please resolve the merge conflicts. |
sjwang05
force-pushed
the
manual-option-folding
branch
from
3014bcf to
486cac5
Compare
|
☔ The latest upstream changes (presumably #11597) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Hey @sjwang05, sorry for the long wait. @giraffate is currently sadly busy and it went by unnoticed 🙈 Are you still interested in continuing this PR? r? @xFrednet |
xFrednet
added
S-waiting-on-author
|
Sorry, I totally forgot about this PR, since it wasn't in my inbox anymore 🙈 I'll add it to my todo list. You'll get a review next week! |
There was a problem hiding this comment.
Again, sorry for the long delay, just commenting r? @xFrednet didn't add it to my inbox 🙈
Anyways, the code is well written. There are several edge cases that need to be considered and might sadly not be solvable with this structure. I left some hints how this can be addressed. If you have any questions, please reach out!
|
|
||
| fn main() { | ||
| let mut opt: Option<i32> = Some(42); | ||
| opt.get_or_insert_with(|| 21); |
There was a problem hiding this comment.
get_or_insert returns &mut T. The lint has to check, that this value is not used. It would also be good to have tests for this
| self.get_call = Some(expr); | ||
| self.recv = Some(recv); | ||
| self.get_method_name = Some(path.ident.name); | ||
| } else if let Some(get_call) = self.get_call.take() |
There was a problem hiding this comment.
This take is only executed on method expressions that fulfill the prerequisites at the beginning of this function.
This means that any number of expressions could between this and the previous insert. This can sadly lead to false positives, as in this example:
fn magic(condition: bool) -> u32 {
let mut option = None;
if condition {
option.insert(19);
}
option.unwrap()
}I think the best solution would be to check for block expressions and then iterate over the statements using a window of 2. That would only catch consequitve option.insert & option.unwrap things, but anything else would be way harder to implement and could result in false positives. Maybe you can take a look at [clippy::vec_init_then_push] here: clippy_lints/src/vec_init_then_push.rs. It's definitly a more complicated implementation, but that would be the best way AFAIK.
| && let ty = cx.typeck_results().expr_ty(recv).peel_refs() | ||
| && is_type_diagnostic_item(cx, ty, sym::Option) | ||
| { | ||
| if path.ident.name == sym!(get_or_insert) |
There was a problem hiding this comment.
This should also catch usages of insert(), which has been added in Rust 1.53. We have some docs for how you can check for the specified MSRV: https://doc.rust-lang.org/clippy/development/adding_lints.html#specifying-the-lints-minimum-supported-rust-version-msrv
Closes #10646
This lint checks for panicking unwrap methods (
unwrap,unwrap_unchecked,expect) used afterget_or_insert_with.changelog: new lint: [
manual_option_folding]