Skip to content

Commit 409c0f0

Browse files
ManishearthManishearth
committed
Merge pull request #468 from devonhollowood/option-methods
Lint `map(f).unwrap_or(a)` and `map(f).unwrap_or_else(g)`
2 parents 515b2af + 443e455 commit 409c0f0

File tree

5 files changed

+136
-14
lines changed

5 files changed

+136
-14
lines changed

‎README.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
66
[Jump to usage instructions](#usage)
77

88
##Lints
9-
There are 77 lints included in this crate:
9+
There are 79 lints included in this crate:
1010

1111
name | default | meaning
1212
---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -53,6 +53,8 @@ name
5353
[non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal) | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead
5454
[nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options) | warn | nonsensical combination of options for opening a file
5555
[ok_expect](https://github.com/Manishearth/rust-clippy/wiki#ok_expect) | warn | using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result
56+
[option_map_unwrap_or](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or) | warn | using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`)
57+
[option_map_unwrap_or_else](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else) | warn | using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)`)
5658
[option_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#option_unwrap_used) | allow | using `Option.unwrap()`, which should at least get a better message using `expect()`
5759
[precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence) | warn | catches operations where precedence may be unclear. See the wiki for a list of cases caught
5860
[ptr_arg](https://github.com/Manishearth/rust-clippy/wiki#ptr_arg) | warn | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively

‎src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,8 @@ pub fn plugin_registrar(reg: &mut Registry) {
156156
matches::MATCH_REF_PATS,
157157
matches::SINGLE_MATCH,
158158
methods::OK_EXPECT,
159+
methods::OPTION_MAP_UNWRAP_OR,
160+
methods::OPTION_MAP_UNWRAP_OR_ELSE,
159161
methods::SHOULD_IMPLEMENT_TRAIT,
160162
methods::STR_TO_STRING,
161163
methods::STRING_TO_STRING,

‎src/methods.rs

Lines changed: 69 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use rustc::middle::subst::{Subst, TypeSpace};
55
use std::iter;
66
use std::borrow::Cow;
77

8-
use utils::{snippet, span_lint, match_path, match_type, walk_ptrs_ty_depth,
8+
use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, walk_ptrs_ty_depth,
99
walk_ptrs_ty};
1010
use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH};
1111

@@ -34,12 +34,18 @@ declare_lint!(pub WRONG_PUB_SELF_CONVENTION, Allow,
3434
declare_lint!(pub OK_EXPECT, Warn,
3535
"using `ok().expect()`, which gives worse error messages than \
3636
calling `expect` directly on the Result");
37-
37+
declare_lint!(pub OPTION_MAP_UNWRAP_OR, Warn,
38+
"using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as \
39+
`map_or(a, f)`)");
40+
declare_lint!(pub OPTION_MAP_UNWRAP_OR_ELSE, Warn,
41+
"using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as \
42+
`map_or_else(g, f)`)");
3843

3944
impl LintPass for MethodsPass {
4045
fn get_lints(&self) -> LintArray {
4146
lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING,
42-
SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION, OK_EXPECT)
47+
SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION, OK_EXPECT, OPTION_MAP_UNWRAP_OR,
48+
OPTION_MAP_UNWRAP_OR_ELSE)
4349
}
4450
}
4551

@@ -92,6 +98,66 @@ impl LateLintPass for MethodsPass {
9298
}
9399
}
94100
}
101+
// check Option.map(_).unwrap_or(_)
102+
else if name.node.as_str() == "unwrap_or" {
103+
if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node {
104+
if inner_name.node.as_str() == "map"
105+
&& match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &OPTION_PATH) {
106+
// lint message
107+
let msg =
108+
"called `map(f).unwrap_or(a)` on an Option value. This can be done \
109+
more directly by calling `map_or(a, f)` instead";
110+
// get args to map() and unwrap_or()
111+
let map_arg = snippet(cx, inner_args[1].span, "..");
112+
let unwrap_arg = snippet(cx, args[1].span, "..");
113+
// lint, with note if neither arg is > 1 line and both map() and
114+
// unwrap_or() have the same span
115+
let multiline = map_arg.lines().count() > 1
116+
|| unwrap_arg.lines().count() > 1;
117+
let same_span = inner_args[1].span.expn_id == args[1].span.expn_id;
118+
if same_span && !multiline {
119+
span_note_and_lint(
120+
cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span,
121+
&format!("replace this with map_or({1}, {0})",
122+
map_arg, unwrap_arg)
123+
);
124+
}
125+
else if same_span && multiline {
126+
span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg);
127+
};
128+
}
129+
}
130+
}
131+
// check Option.map(_).unwrap_or_else(_)
132+
else if name.node.as_str() == "unwrap_or_else" {
133+
if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node {
134+
if inner_name.node.as_str() == "map"
135+
&& match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &OPTION_PATH) {
136+
// lint message
137+
let msg =
138+
"called `map(f).unwrap_or_else(g)` on an Option value. This can be \
139+
done more directly by calling `map_or_else(g, f)` instead";
140+
// get args to map() and unwrap_or_else()
141+
let map_arg = snippet(cx, inner_args[1].span, "..");
142+
let unwrap_arg = snippet(cx, args[1].span, "..");
143+
// lint, with note if neither arg is > 1 line and both map() and
144+
// unwrap_or_else() have the same span
145+
let multiline = map_arg.lines().count() > 1
146+
|| unwrap_arg.lines().count() > 1;
147+
let same_span = inner_args[1].span.expn_id == args[1].span.expn_id;
148+
if same_span && !multiline {
149+
span_note_and_lint(
150+
cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg, expr.span,
151+
&format!("replace this with map_or_else({1}, {0})",
152+
map_arg, unwrap_arg)
153+
);
154+
}
155+
else if same_span && multiline {
156+
span_lint(cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg);
157+
};
158+
}
159+
}
160+
}
95161
}
96162
}
97163

‎src/mut_mut.rs

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,17 +39,20 @@ fn check_expr_mut(cx: &LateContext, expr: &Expr) {
3939
}
4040

4141
unwrap_addr(expr).map_or((), |e| {
42-
unwrap_addr(e).map(|_| {
43-
span_lint(cx, MUT_MUT, expr.span,
44-
"generally you want to avoid `&mut &mut _` if possible")
45-
}).unwrap_or_else(|| {
46-
if let TyRef(_, TypeAndMut{ty: _, mutbl: MutMutable}) =
47-
cx.tcx.expr_ty(e).sty {
48-
span_lint(cx, MUT_MUT, expr.span,
49-
"this expression mutably borrows a mutable reference. \
50-
Consider reborrowing")
42+
unwrap_addr(e).map_or_else(
43+
|| {
44+
if let TyRef(_, TypeAndMut{ty: _, mutbl: MutMutable}) =
45+
cx.tcx.expr_ty(e).sty {
46+
span_lint(cx, MUT_MUT, expr.span,
47+
"this expression mutably borrows a mutable reference. \
48+
Consider reborrowing")
5149
}
52-
})
50+
},
51+
|_| {
52+
span_lint(cx, MUT_MUT, expr.span,
53+
"generally you want to avoid `&mut &mut _` if possible")
54+
}
55+
)
5356
})
5457
}
5558

‎tests/compile-fail/methods.rs

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,55 @@ impl Mul<T> for T {
3434
fn mul(self, other: T) -> T { self } // no error, obviously
3535
}
3636

37+
/// Utility macro to test linting behavior in `option_methods()`
38+
/// The lints included in `option_methods()` should not lint if the call to map is partially
39+
/// within a macro
40+
macro_rules! opt_map {
41+
($opt:expr, $map:expr) => {($opt).map($map)};
42+
}
43+
44+
/// Checks implementation of the following lints:
45+
/// OPTION_MAP_UNWRAP_OR
46+
/// OPTION_MAP_UNWRAP_OR_ELSE
47+
fn option_methods() {
48+
let opt = Some(1);
49+
50+
// Check OPTION_MAP_UNWRAP_OR
51+
// single line case
52+
let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or(a)`
53+
//~| NOTE replace this
54+
.unwrap_or(0); // should lint even though this call is on a separate line
55+
// multi line cases
56+
let _ = opt.map(|x| { //~ ERROR called `map(f).unwrap_or(a)`
57+
x + 1
58+
}
59+
).unwrap_or(0);
60+
let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or(a)`
61+
.unwrap_or({
62+
0
63+
});
64+
// macro case
65+
let _ = opt_map!(opt, |x| x + 1).unwrap_or(0); // should not lint
66+
67+
// Check OPTION_MAP_UNWRAP_OR_ELSE
68+
// single line case
69+
let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or_else(g)`
70+
//~| NOTE replace this
71+
.unwrap_or_else(|| 0); // should lint even though this call is on a separate line
72+
// multi line cases
73+
let _ = opt.map(|x| { //~ ERROR called `map(f).unwrap_or_else(g)`
74+
x + 1
75+
}
76+
).unwrap_or_else(|| 0);
77+
let _ = opt.map(|x| x + 1) //~ ERROR called `map(f).unwrap_or_else(g)`
78+
.unwrap_or_else(||
79+
0
80+
);
81+
// macro case
82+
let _ = opt_map!(opt, |x| x + 1).unwrap_or_else(|| 0); // should not lint
83+
84+
}
85+
3786
fn main() {
3887
use std::io;
3988

0 commit comments

Comments
 (0)