Lint: filter(Option::is_some).map(Option::unwrap) #6342
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
Consider supporting/testing these cases: // Option<Option<_>>
let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap);
// lambdas
let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap()); |
|
Still alive and will do these extra cases/cleanups. Big work thing came up, but soon! |
|
☔ The latest upstream changes (presumably #6389) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
|
Made several of the required changes, but conflict resolution against master seems to have gone sideways. Cleaning up. EDIT: Github may just be behind; local diff doesn't have 3800 lines. Giving it a few minutes for rebase to resolve. |
|
Right, so, assuming the diff view fixes itself:
The extra scope meant I had to guess at best practices for a few things; I hope I picked the right methods. I'm used to techniques from simpler languages where you can just beta-reduce and compare normal forms, which I understand can't work easily in Rust. |
flip1995
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #6316) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
|
I'm noticing #6453 overlaps heavily with this, due to #3188 being...maybe a superset of #6061. I'm not sure which of the reviewers/submitters have more background here, or if I should just sort it out with #6453's author? @flip1995 what do you think? I'll also tag @camsteffen as the author of #6453. (I have no attachment to being the one to fix it, and I don't want to fall victim to any sunk-cost fallacies. If I should just close this narrower solution, that's fine.) |
|
@bbqbaron Sorry I didn't anticipate that since my last comment. I think we haven't done any overlapping work yet. But I agree, my PR will supercede most of this, as it is currently planned. I think you could either A) just lint the non-closure case since that is the one non-overlapping piece or B) extend this PR to do what is currently planned in #6453. I have no problem with that since I haven't started that work yet and you opened this PR first. With option B, #6453 can just become |
|
@camsteffen @bbqbaron It doesn't seem that there is any overlapping work yet. But #6453 will change the lint significantly, especially once mikerites suggestion is implemented (which I think we want to do?). So I'd encourage you to work together to improve this lint in general or to at least wait for the changes in #6453 before continue working on this. That will avoid rebase/merge conflicts between the two PRs. I think the changes made here can be used as-is (with my comments addressed) in #6453 to implement the first case in mikerites suggestion. |
|
Ok, so if in doubt I'll at least hold off for #6453, or once the holidays are over, if for some reason we need to intermingle the changes, I'm around to collaborate. |
|
Thanks @giraffate! I forgot to follow up. @bbqbaron You may pick this back up if you'd like. My PR did not cover the non-closure case. FYI, I have a shared implementation for |
|
Thought about this a little more. This is still a new lint and doesn't overlap a whole lot with the implementation of my PR. So just want to remove the expectation that this should "extend" on #6591. I have another thought. Would it make more sense to simply lint |
|
@camsteffen thanks for the ping and sorry for the delay. as a first-timer on the repo i'm a little unsure who's ultimately responsible for decisions, but
...makes some sense to me. i suppose there are legitimate use cases for partitioning optionals into present and absent cases, though i can't come up with an immediately obvious one. hm. to me, the more suspicious expression is the |
No worries!
Maybe this is just semantics, but filtering is different from partitioning. With The only somewhat questionable case I can think of is
I disagree. It could be bad error handling (just like any usage of // I can safely unwrap here
"bad".chars().map(|c| "abcd".find(c)).map(Option::unwrap)With that said, I don't want to put up a road block if you would like to go ahead with the original intent of this PR. |
|
ping from triage @bbqbaron. Do you need help to move this forward? If so, feel free to ask reviewers questions. |
Thanks for pinging me. Last time I picked this up, I ran into some build errors, probably due to movement in the toolchain underneath me. I took too long before coming back. I've got it building now and will resume (hopefully not restart!) implementation, targeting an updated PR this weekend. |
|
i've updated to incorporate all the changes to master that this branch has missed. i think there are a couple of items to confirm:
it sounds like, to repeat it back and make sure i have it right, the only real new functionality now covered by #6061 is that a sorry, this ticket feels like it's cost a lot more discussion/effort than the ultimately quite limited change it makes, if i have all this right. apologies! new to non-toy Rust and the repo, so hopefully any future changes will be smoother. |
|
Can you do a rebase, not merge? We follow a no-merge policy: https://github.com/rust-lang/rust-clippy/blob/master/doc/basics.md#pr. |
oh, sorry, even when merging against my own branch? i guess i over-interpreted |
|
☔ The latest upstream changes (presumably #6826) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Sorry, I totally forgot about this PR.
The methods module is now split up even further and each lint is now in its own module. Also in the last 1-2 weeks much refactoring was done in Clippy, so you probably will have to rebase this PR and fix some imports etc.
|
Thanks for the notes. I got moderately severe covid, so I'll be gone
for...well, who knows.
I'm worried there'll be yet another new round of major conflicts when I
return, but I guess we'll see!
…On Fri, Mar 19, 2021, 5:12 AM Philipp Krones ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Sorry, I totally forgot about this PR.
The methods module is now split up even further and each lint is now in
its own module. Also in the last 1-2 weeks much refactoring was done in
Clippy, so you probably will have to rebase this PR and fix some imports
etc.
------------------------------
In clippy_utils/src/paths.rs
<#6342 (comment)>
:
> @@ -87,7 +87,9 @@ pub const OPEN_OPTIONS: [&str; 3] = ["std", "fs", "OpenOptions"];
pub const OPS_MODULE: [&str; 2] = ["core", "ops"];
pub const OPTION: [&str; 3] = ["core", "option", "Option"];
pub const OPTION_NONE: [&str; 4] = ["core", "option", "Option", "None"];
+pub const OPTION_IS_SOME: [&str; 4] = ["core", "option", "Option", "is_some"];
Those shouldn't be necessary. See the unwrap module on how to check for
this:
1. Get the type of the expression unwrap is called on and make sure it
is an option.
2. Just compare the method name with sym::unwrap/sym::is_some
------------------------------
In tests/ui/option_filter_map.stderr
<#6342 (comment)>
:
> +LL | let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap());
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `Some(Some(1)).flatten()`
For a nicer suggestion with longer method chains, you shouldn't include
the iterator filter and map are called on, but just the span of
filter().map()
------------------------------
In clippy_lints/src/methods/mod.rs
<#6342 (comment)>
:
> if_chain! {
if let ExprKind::MethodCall(_, _, [map_recv, map_arg], map_span) = expr.kind;
- if let ExprKind::MethodCall(_, _, [_, filter_arg], filter_span) = map_recv.kind;
+ if let ExprKind::MethodCall(_, _, [filter_recv, filter_arg], filter_span) = map_recv.kind;
+ let _ = lint_filter_some_map_unwrap(cx, expr, filter_recv, filter_arg, map_arg, target_span);
Sneaking this into the if_chain seems very wrong to me.
------------------------------
In clippy_lints/src/methods/bind_instead_of_map.rs
<#6342 (comment)>
:
> @@ -1,7 +1,8 @@
-use super::{contains_return, BIND_INSTEAD_OF_MAP};
+use super::BIND_INSTEAD_OF_MAP;
Those changes seem unrelated
------------------------------
In clippy_lints/src/methods/mod.rs
<#6342 (comment)>
:
> @@ -2908,18 +2932,85 @@ fn lint_skip_while_next<'tcx>(
}
}
-/// lint use of `filter().map()` for `Iterators`
+fn is_method<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, method_path: &[&str], method_name: Symbol) -> bool {
I think my previous comment
<https://github.com/rust-lang/rust-clippy/pull/6342/files#r545862641>
still applies here. This closure unpacking now appears in 4 lints across 4
files. This should probably be moved to the methods/utils module. But this
should be done in another PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6342 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABWFX7OFJ3TJ5PXZOS6NSR3TEMIOTANCNFSM4TZKLJ2A>
.
|
|
Right, so, crawled out from under my rock and applied the suggestions as I understand them. I put the new functionality in |
|
@bors r+ Thanks! |
|
📌 Commit 56fbbf7 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #6061
Please write a short comment explaining your change (or "none" for internal only changes)
changelog:
First Rust PR, so I'm sure I've violated some idioms. Happy to change anything.
I'm getting one test failure locally -- a stderr diff for
compile_test. I'm having a hard time seeing how I could be causing it, so I'm tentatively opening this in the hopes that it's an artifact of my local setup againstrustc. Hoping it can at least still be reviewed in the meantime.I'm gathering that since this is a method lint, and
.filter(...).map(...)is already checked, the means of implementation needs to be a little different, so I didn't exactly follow the setup boilerplate. My way of checking for method calls seems a little too direct (ie, "is the second element of the expression literally the path forOption::is_some?"), but it seems like that's how some other lints work, so I went with it. I'm assuming we're not concerned about, eg, closures that just end up equivalent toOption::is_someby eta reduction.