Skip to content
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

Add lint for mem::replace(.., None). #3195

Merged
merged 5 commits into from Sep 23, 2018
Merged

Add lint for mem::replace(.., None). #3195

merged 5 commits into from Sep 23, 2018

Conversation

@JayKickliter
Copy link
Contributor

@JayKickliter JayKickliter commented Sep 17, 2018

This PR adds a lint suggesting Option::take() as an alternative to mem::replace(.., None):

error: replacing an `Option` with `None`
 --> $DIR/mem_replace.rs:8:13
  |
8 |     let _ = mem::replace(&mut an_option, None);
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `an_option.take()`
  |
  = note: `-D clippy::mem-replace-option-with-none` implied by `-D warnings`

error: aborting due to previous error

I have run across this in both my company's and third party codebases. At least one guide suggests favoring take() as well:

Note, however, that if we are using an Option and want to replace its value with a None, Option’s take() method provides a shorter and more idiomatic alternative.

Since this is my first Clippy contribution, I'm all ears to any suggestions to improve this PR.

@JayKickliter
Copy link
Contributor Author

@JayKickliter JayKickliter commented Sep 17, 2018

One thing I’m not sure about is this early check to make sure the function call has two arguments. It seems like a low-cost check, but maybe it’s premature optimization?

@mikerite
Copy link
Contributor

@mikerite mikerite commented Sep 18, 2018

I came across this same suggestion in a reddit thread a while ago: https://www.reddit.com/r/rust/comments/94elrg/blog_some_slight_improvements/

clippy_lints/src/mem_replace.rs Outdated Show resolved Hide resolved
clippy_lints/src/mem_replace.rs Outdated Show resolved Hide resolved
@JayKickliter
Copy link
Contributor Author

@JayKickliter JayKickliter commented Sep 18, 2018

It just occurred to me that this lint will match:

    let mut an_option = Some(1);
    let _ = mem::replace(&mut an_option, None);

but not:

    let mut_ref_an_option = &mut an_option;
    let _ = mem::replace(mut_ref_an_option, None);

Let figure out how to do that and update this PR...

@flip1995
Copy link
Member

@flip1995 flip1995 commented Sep 18, 2018

Oh good catch! I missed that one. The problem is that match_type(cx, expr_ty(replaced), path) fails due to the reference. There's already a function to solve this in utils.

@JayKickliter
Copy link
Contributor Author

@JayKickliter JayKickliter commented Sep 18, 2018

There's already a function to solve this in utils

Thanks for the suggestion. I looked through utils but nothing stood out. Any idea what the function is called?

@flip1995
Copy link
Member

@flip1995 flip1995 commented Sep 18, 2018

@JayKickliter
Copy link
Contributor Author

@JayKickliter JayKickliter commented Sep 19, 2018

I updated the lint to match both &mut an_option and an_option. I think this solution is sound:
https://github.com/rust-lang-nursery/rust-clippy/blob/79cda3bb1ef7f4261bf0fb18bbb141127d31c5a5/clippy_lints/src/mem_replace.rs#L55-L59

@flip1995
Copy link
Member

@flip1995 flip1995 commented Sep 20, 2018

Oh the problem was something else. Oops, sorry for the wrong pointers! LGTM

@JayKickliter
Copy link
Contributor Author

@JayKickliter JayKickliter commented Sep 20, 2018

@flip1995 no problem, you pointed me to something useful I didn't know about. Thanks!

@JayKickliter
Copy link
Contributor Author

@JayKickliter JayKickliter commented Sep 22, 2018

@Manishearth not sure if necessary, but:

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to chose either at their option.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Sep 22, 2018

Sweet, thanks. I was going to open a separate issue for the new contributors once the other issues closed.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Sep 22, 2018

but this works too

@flip1995 flip1995 merged commit b5c4342 into rust-lang:master Sep 23, 2018
1 of 2 checks passed
@Manishearth
Copy link
Member

@Manishearth Manishearth commented Oct 5, 2018

@JayKickliter could you also leave this comment on #3230 for completeness' sake?

@JayKickliter
Copy link
Contributor Author

@JayKickliter JayKickliter commented Oct 5, 2018

Done

@JayKickliter JayKickliter deleted the jsk/mem_replace_opt_w_none branch Oct 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants