Skip to content

Commit ff27988

Browse files
committed
Auto merge of #3666 - detrumi:map-or-on-non-copy, r=flip1995
Only suggest map_or for copy types Fixes #2686
2 parents 58c0dc1 + eb70a72 commit ff27988

File tree

4 files changed

+173
-77
lines changed

4 files changed

+173
-77
lines changed

clippy_lints/src/methods/mod.rs

+2-43
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ use syntax::ast;
2222
use syntax::source_map::{BytePos, Span};
2323
use syntax::symbol::LocalInternedString;
2424

25+
mod option_map_unwrap_or;
2526
mod unnecessary_filter_map;
2627

2728
#[derive(Clone)]
@@ -836,7 +837,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
836837
["unwrap", "get_mut"] => lint_get_unwrap(cx, expr, arg_lists[1], true),
837838
["unwrap", ..] => lint_unwrap(cx, expr, arg_lists[0]),
838839
["expect", "ok"] => lint_ok_expect(cx, expr, arg_lists[1]),
839-
["unwrap_or", "map"] => lint_map_unwrap_or(cx, expr, arg_lists[1], arg_lists[0]),
840+
["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0]),
840841
["unwrap_or_else", "map"] => lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]),
841842
["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]),
842843
["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
@@ -1769,48 +1770,6 @@ fn lint_ok_expect(cx: &LateContext<'_, '_>, expr: &hir::Expr, ok_args: &[hir::Ex
17691770
}
17701771
}
17711772

1772-
/// lint use of `map().unwrap_or()` for `Option`s
1773-
fn lint_map_unwrap_or(cx: &LateContext<'_, '_>, expr: &hir::Expr, map_args: &[hir::Expr], unwrap_args: &[hir::Expr]) {
1774-
// lint if the caller of `map()` is an `Option`
1775-
if match_type(cx, cx.tables.expr_ty(&map_args[0]), &paths::OPTION) {
1776-
// get snippets for args to map() and unwrap_or()
1777-
let map_snippet = snippet(cx, map_args[1].span, "..");
1778-
let unwrap_snippet = snippet(cx, unwrap_args[1].span, "..");
1779-
// lint message
1780-
// comparing the snippet from source to raw text ("None") below is safe
1781-
// because we already have checked the type.
1782-
let arg = if unwrap_snippet == "None" { "None" } else { "a" };
1783-
let suggest = if unwrap_snippet == "None" {
1784-
"and_then(f)"
1785-
} else {
1786-
"map_or(a, f)"
1787-
};
1788-
let msg = &format!(
1789-
"called `map(f).unwrap_or({})` on an Option value. \
1790-
This can be done more directly by calling `{}` instead",
1791-
arg, suggest
1792-
);
1793-
// lint, with note if neither arg is > 1 line and both map() and
1794-
// unwrap_or() have the same span
1795-
let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1;
1796-
let same_span = map_args[1].span.ctxt() == unwrap_args[1].span.ctxt();
1797-
if same_span && !multiline {
1798-
let suggest = if unwrap_snippet == "None" {
1799-
format!("and_then({})", map_snippet)
1800-
} else {
1801-
format!("map_or({}, {})", unwrap_snippet, map_snippet)
1802-
};
1803-
let note = format!(
1804-
"replace `map({}).unwrap_or({})` with `{}`",
1805-
map_snippet, unwrap_snippet, suggest
1806-
);
1807-
span_note_and_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span, &note);
1808-
} else if same_span && multiline {
1809-
span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg);
1810-
};
1811-
}
1812-
}
1813-
18141773
/// lint use of `map().flatten()` for `Iterators`
18151774
fn lint_map_flatten<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, map_args: &'tcx [hir::Expr]) {
18161775
// lint if caller of `.map().flatten()` is an Iterator
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
use crate::utils::paths;
2+
use crate::utils::{is_copy, match_type, snippet, span_lint, span_note_and_lint};
3+
use rustc::hir::intravisit::{walk_path, NestedVisitorMap, Visitor};
4+
use rustc::hir::{self, *};
5+
use rustc::lint::LateContext;
6+
use rustc_data_structures::fx::FxHashSet;
7+
use syntax::symbol::Symbol;
8+
9+
use super::OPTION_MAP_UNWRAP_OR;
10+
11+
/// lint use of `map().unwrap_or()` for `Option`s
12+
pub(super) fn lint<'a, 'tcx>(
13+
cx: &LateContext<'a, 'tcx>,
14+
expr: &hir::Expr,
15+
map_args: &'tcx [hir::Expr],
16+
unwrap_args: &'tcx [hir::Expr],
17+
) {
18+
// lint if the caller of `map()` is an `Option`
19+
if match_type(cx, cx.tables.expr_ty(&map_args[0]), &paths::OPTION) {
20+
if !is_copy(cx, cx.tables.expr_ty(&unwrap_args[1])) {
21+
// Do not lint if the `map` argument uses identifiers in the `map`
22+
// argument that are also used in the `unwrap_or` argument
23+
24+
let mut unwrap_visitor = UnwrapVisitor {
25+
cx,
26+
identifiers: FxHashSet::default(),
27+
};
28+
unwrap_visitor.visit_expr(&unwrap_args[1]);
29+
30+
let mut map_expr_visitor = MapExprVisitor {
31+
cx,
32+
identifiers: unwrap_visitor.identifiers,
33+
found_identifier: false,
34+
};
35+
map_expr_visitor.visit_expr(&map_args[1]);
36+
37+
if map_expr_visitor.found_identifier {
38+
return;
39+
}
40+
}
41+
42+
// get snippets for args to map() and unwrap_or()
43+
let map_snippet = snippet(cx, map_args[1].span, "..");
44+
let unwrap_snippet = snippet(cx, unwrap_args[1].span, "..");
45+
// lint message
46+
// comparing the snippet from source to raw text ("None") below is safe
47+
// because we already have checked the type.
48+
let arg = if unwrap_snippet == "None" { "None" } else { "a" };
49+
let suggest = if unwrap_snippet == "None" {
50+
"and_then(f)"
51+
} else {
52+
"map_or(a, f)"
53+
};
54+
let msg = &format!(
55+
"called `map(f).unwrap_or({})` on an Option value. \
56+
This can be done more directly by calling `{}` instead",
57+
arg, suggest
58+
);
59+
// lint, with note if neither arg is > 1 line and both map() and
60+
// unwrap_or() have the same span
61+
let multiline = map_snippet.lines().count() > 1 || unwrap_snippet.lines().count() > 1;
62+
let same_span = map_args[1].span.ctxt() == unwrap_args[1].span.ctxt();
63+
if same_span && !multiline {
64+
let suggest = if unwrap_snippet == "None" {
65+
format!("and_then({})", map_snippet)
66+
} else {
67+
format!("map_or({}, {})", unwrap_snippet, map_snippet)
68+
};
69+
let note = format!(
70+
"replace `map({}).unwrap_or({})` with `{}`",
71+
map_snippet, unwrap_snippet, suggest
72+
);
73+
span_note_and_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span, &note);
74+
} else if same_span && multiline {
75+
span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg);
76+
};
77+
}
78+
}
79+
80+
struct UnwrapVisitor<'a, 'tcx: 'a> {
81+
cx: &'a LateContext<'a, 'tcx>,
82+
identifiers: FxHashSet<Symbol>,
83+
}
84+
85+
impl<'a, 'tcx: 'a> Visitor<'tcx> for UnwrapVisitor<'a, 'tcx> {
86+
fn visit_path(&mut self, path: &'tcx Path, _id: HirId) {
87+
self.identifiers.insert(ident(path));
88+
walk_path(self, path);
89+
}
90+
91+
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
92+
NestedVisitorMap::All(&self.cx.tcx.hir())
93+
}
94+
}
95+
96+
struct MapExprVisitor<'a, 'tcx: 'a> {
97+
cx: &'a LateContext<'a, 'tcx>,
98+
identifiers: FxHashSet<Symbol>,
99+
found_identifier: bool,
100+
}
101+
102+
impl<'a, 'tcx: 'a> Visitor<'tcx> for MapExprVisitor<'a, 'tcx> {
103+
fn visit_path(&mut self, path: &'tcx Path, _id: HirId) {
104+
if self.identifiers.contains(&ident(path)) {
105+
self.found_identifier = true;
106+
return;
107+
}
108+
walk_path(self, path);
109+
}
110+
111+
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
112+
NestedVisitorMap::All(&self.cx.tcx.hir())
113+
}
114+
}
115+
116+
fn ident(path: &Path) -> Symbol {
117+
path.segments
118+
.last()
119+
.expect("segments should be composed of at least 1 element")
120+
.ident
121+
.name
122+
}

tests/ui/methods.rs

+7
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,13 @@ fn option_methods() {
179179
// macro case
180180
let _ = opt_map!(opt, |x| x + 1).unwrap_or(0); // should not lint
181181

182+
// Should not lint if not copyable
183+
let id: String = "identifier".to_string();
184+
let _ = Some("prefix").map(|p| format!("{}.{}", p, id)).unwrap_or(id);
185+
// ...but DO lint if the `unwrap_or` argument is not used in the `map`
186+
let id: String = "identifier".to_string();
187+
let _ = Some("prefix").map(|p| format!("{}.", p)).unwrap_or(id);
188+
182189
// Check OPTION_MAP_UNWRAP_OR_ELSE
183190
// single line case
184191
let _ = opt.map(|x| x + 1)

0 commit comments

Comments
 (0)