-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add lint unnecessary_option_map_or_else #14662
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
base: master
Are you sure you want to change the base?
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 looks good in general, I just have a few subtle suggestions. Please use snippet_with_applicability
instead of plain snippet
. Otherwise, we end up breaking cargo fix
. In addition, I'd like to see some tests for other scenarios.
let self_snippet = snippet(cx, recv.span, ".."); | ||
let err_snippet = snippet(cx, def_arg.span, ".."); |
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.
We should use snippet_with_applicability
instead here. Also the default for self_snippet
should be "_"
, not ".."
.
tmp | ||
}, | ||
); | ||
} |
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'd like to see a test case with std::convert::identity
. And one where the closure is put in a let
binding before the map_or_else
(we can get that with expr_or_init
).
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.
The test case with a let
binding is clear, but I am not sure if I understand correctly the test case with std::convert::identity
: is it be supposed to be marked as an error? Would it be actually possible to statically check if the provided function is doing something with a parameter before returning it if said function is from an external library?
changelog: [
unnecessary_option_map_or_else
]: Added lint unnecessary_option_map_or_else. As suggested in the issue description, the implementation takes as reference the issue #7328. The tests for lintsoption_if_let_else
andor_fun_call
needed to be adjusted to comply with new lint.fixes #14588