Github Lint: filter(Option::is_some).map(Option::unwrap) by bbqbaron · Pull Requ...
source link: https://github.com/rust-lang/rust-clippy/pull/6342
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Fixes #6061
Please write a short comment explaining your change (or "none" for internal only changes)
changelog:
- add new lint for filter(Option::is_some).map(Option::unwrap)
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 against rustc. 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 for Option::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 to Option::is_some by eta reduction.
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.
LGTM. Some code cleanup left to do.
To also have this pass locally: rebase on master and rerun setup-toolchain.sh
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:
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author
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:
- now handling closures that call the appropriate method. I cribbed from code with a similar need; I hope I inferred the methodology right.
- handle options as well via
is_type_diagnostic_item - switch to suggestions and test fixes
- add some negative tests, as it were, that
.map(blah).flatten()comes out of this stage (presumably to be linted by another stage)
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.
I wouldn't add a new lint for this, but just reuse the FILTER_MAP lint.
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:
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author
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 filter_map_more as discussed there.
@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 manual_filter_map and manual_find_map which you can extend on to improve both lints. It would be nice to cover any combination of closure and non-closure args to filter and map.
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 filter(Option::is_some) and call the lint filter_some? To me that seems lint-able without looking at what comes after filter (or find).
@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
I have another thought. Would it make more sense to simply lint filter(Option::is_some) and call the lint filter_some? To me that seems lint-able without looking at what comes after filter (or find).
...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 map(Option::unwrap), which seems to me more likely to be a bad idea that could be replaced by another methodology (although i'm not a Rust veteran). what do you think?
@camsteffen thanks for the ping and sorry for the delay.
No worries!
i suppose there are legitimate use cases for partitioning optionals into present and absent cases...
Maybe this is just semantics, but filtering is different from partitioning. With filter(Option::is_some), the user is just throwing away Nones without observing them in any way. So there is no information lost by changing to flatten().
The only somewhat questionable case I can think of is filter(Option::is_some).for_each(fn_takes_option). But even then I think flatten().for_each(|x| fn_takes_option(Some(x)) is better for clarity.
the more suspicious expression is the
map(Option::unwrap)
I disagree. It could be bad error handling (just like any usage of unwrap), but there may be a perfectly valid reason to expect values to be Some (again, just like any usage of unwrap). Example:
// 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:
-
discussion on #6591 suggests that
option_filter_mapshould be a new lint, counter to suggestion above, so i left it in, asfilter_mapseems to be getting decomposed. -
so i could get changes up and confirm scope/intended behavior, i've just shoehorned
option_filter_mapinto the middle of thelint_filter_mapfunction introduced in #6591. tests pass, although it's kind of crude. they could be integrated more cleanly. however, given all the back-and-forth on #6591 and its ancestors/tickets, i wanted to make sure i hadn't missed/misinterpreted important movement/discussion:
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 filter->map that is exactly is_some into unwrap (either via direct method reference or trivial closure) should become flatten, which is a little cleaner than an identity flat_map. we already have a lint that tries to reject trivial closures (|x| some_fn(x)), so depending on whether we assume that lint is active, we could literally just check for method references (Option::is_some) and nothing else, delegating anything more complicated to the excellent work on #6591?
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 E.g. always use rebase when bringing the latest changes from the master branch to your feature branch. sure. i think i can...back out the merge commit and force-push a rebase. will give it a shot.
The latest upstream changes (presumably #6826) made this pull request unmergeable. Please resolve the merge conflicts.
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!
Right, so, crawled out from under my rock and applied the suggestions as I understand them. I put the new functionality in filter_map on the logic that it's a special case of that lint's domain, and tried to adopt the suggested methods of detecting interesting functions.
Great to hear that you're back! This looks great now. One small clean up left to done, one squash of your commits and this is good to go.
@bors r+
Thanks!
Commit 56fbbf7 has been approved by flip1995
Testing commit Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK