Add lint for opt.map_or(None, f) #2128

lukasstevens · 2017-10-10T11:43:14Z

I don't know if my formatting is correct, since running rustfmt would have changed code which was not written by me. Furthermore, I hope that the implementation is correct, since this is the first time I had a look at the clippy internals.
Also, I saw that there is a move towards splitting the methods.rs into mutliple files. Maybe it would be sensible to move the code from this commit into another file.

Change to Warn and add multiline support Fix typo Update reference

oli-obk · 2017-10-10T12:50:49Z

clippy_lints/src/methods.rs

+    pub OPTION_MAP_OR_NONE,
+    Warn,
+    "using `Option.map_or(None, f)`, which is more succinctly expressed as \
+     `map_or_else(g, f)`"


copy paste error

oli-obk · 2017-10-10T12:53:08Z

clippy_lints/src/methods.rs

+/// lint use of `_.map_or(None, _)` for `Option`s
+fn lint_map_or_none<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, map_or_args: &'tcx [hir::Expr]) {
+    // check if the first non-self argument to map_or() is None
+    let map_or_arg_is_none = if let hir::Expr_::ExprPath(ref qpath) = map_or_args[1].node {


You need to ensure that the type is Option before doing this check. Otherwise a random type with a map_or function can cause this lint to panic if the map_or has zero arguments (so just the self).

oli-obk · 2017-10-10T12:54:45Z

clippy_lints/src/methods.rs

+                   `and_then(f)` instead";
+        let map_or_none_snippet = snippet(cx, map_or_args[1].span, "..");
+        let map_or_func_snippet = snippet(cx, map_or_args[2].span, "..");
+        let multiline = map_or_func_snippet.lines().count() > 1 || map_or_none_snippet.lines().count() > 1;


Please get rid of the multiline check (I know all the others still do it, but they are rather old lints). Use span_lint_and_then + span_suggestion to produce the suggestion. Rustc will take care of the rest.

oli-obk · 2017-10-10T12:56:04Z

Also, I saw that there is a move towards splitting the methods.rs into mutliple files. Maybe it would be sensible to move the code from this commit into another file.

I think we can move out a whole chunk of tests. But don't worry about it in this PR.

- Fix copy-paste error - Check for opt.map_or argument after ensuring that opt is an Option - Use span_lint_and_then and span_suggestion - Update reference

oli-obk · 2017-10-10T14:16:34Z

Thanks for addressing my comments so fast!

lukasstevens · 2017-10-10T14:24:56Z

You are welcome. Thanks for reviewing the pull request.

… -> Option Result<T, E> has an `ok()` method that adapts a Result<T,E> into an Option<T>. It's possible to get around this adapter by writing Result<T,E>.map_or(None, Some). This lint is implemented as a new variant of the existing [`option_map_none` lint](rust-lang#2128)

Add new lint for `Result<T, E>.map_or(None, Some(T))` Fixes rust-lang#5414 PR Checklist --- - [x] Followed lint naming conventions (the name is a bit awkward, but it seems to conform) - [x] Added passing UI tests (including committed .stderr file) - [x] cargo test passes locally - [x] Executed cargo dev update_lints - [x] Added lint documentation - [x] Run cargo dev fmt `Result<T, E>` has an [`ok()`](https://doc.rust-lang.org/std/result/enum.Result.html#method.ok) method that adapts a `Result<T,E>` into an `Option<T>`. It's possible to get around this adapter by writing `Result<T,E>.map_or(None, Some)`. This lint is implemented as a new variant of the existing [`option_map_none` lint](rust-lang#2128)

… -> Option Result<T, E> has an `ok()` method that adapts a Result<T,E> into an Option<T>. It's possible to get around this adapter by writing Result<T,E>.map_or(None, Some). This lint is implemented as a new variant of the existing [`option_map_none` lint](rust-lang#2128)

lukasstevens added 2 commits October 10, 2017 13:37

Add lint for opt.map_or(None, f)

eb53cca

Change to Warn and add multiline support Fix typo Update reference

Remove unnecessary borrow

c0fac7c

oli-obk suggested changes Oct 10, 2017

View reviewed changes

Make suggested changes

4438c41

- Fix copy-paste error - Check for opt.map_or argument after ensuring that opt is an Option - Use span_lint_and_then and span_suggestion - Update reference

oli-obk approved these changes Oct 10, 2017

View reviewed changes

oli-obk merged commit 2b491d5 into rust-lang:master Oct 10, 2017

nickrtorres mentioned this pull request Apr 4, 2020

Add new lint for Result<T, E>.map_or(None, Some(T)) #5415

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint for opt.map_or(None, f) #2128

Add lint for opt.map_or(None, f) #2128

lukasstevens commented Oct 10, 2017 •

edited by oli-obk

Loading

oli-obk Oct 10, 2017

oli-obk Oct 10, 2017

oli-obk Oct 10, 2017

oli-obk commented Oct 10, 2017

oli-obk commented Oct 10, 2017

lukasstevens commented Oct 10, 2017

Add lint for opt.map_or(None, f) #2128

Add lint for opt.map_or(None, f) #2128

Conversation

lukasstevens commented Oct 10, 2017 • edited by oli-obk Loading

oli-obk Oct 10, 2017

Choose a reason for hiding this comment

oli-obk Oct 10, 2017

Choose a reason for hiding this comment

oli-obk Oct 10, 2017

Choose a reason for hiding this comment

oli-obk commented Oct 10, 2017

oli-obk commented Oct 10, 2017

lukasstevens commented Oct 10, 2017

lukasstevens commented Oct 10, 2017 •

edited by oli-obk

Loading