Skip to content

Commit 06f1902

Browse files
borsbors
committed
Auto merge of rust-lang#5937 - montrivo:option_if_let_else, r=flip1995
option_if_let_else - distinguish pure from impure else expressions Addresses partially rust-lang#5821. changelog: improve the lint `option_if_let_else`. Suggest `map_or` or `map_or_else` based on the else expression purity.
2 parents 44d6439 + 79da747 commit 06f1902

14 files changed

+421
-213
lines changed

‎clippy_lints/src/methods/bind_instead_of_map.rs

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ use rustc_middle::hir::map::Map;
1212
use rustc_span::Span;
1313

1414
pub(crate) struct OptionAndThenSome;
15+
1516
impl BindInsteadOfMap for OptionAndThenSome {
1617
const TYPE_NAME: &'static str = "Option";
1718
const TYPE_QPATH: &'static [&'static str] = &paths::OPTION;
@@ -24,6 +25,7 @@ impl BindInsteadOfMap for OptionAndThenSome {
2425
}
2526

2627
pub(crate) struct ResultAndThenOk;
28+
2729
impl BindInsteadOfMap for ResultAndThenOk {
2830
const TYPE_NAME: &'static str = "Result";
2931
const TYPE_QPATH: &'static [&'static str] = &paths::RESULT;
@@ -36,6 +38,7 @@ impl BindInsteadOfMap for ResultAndThenOk {
3638
}
3739

3840
pub(crate) struct ResultOrElseErrInfo;
41+
3942
impl BindInsteadOfMap for ResultOrElseErrInfo {
4043
const TYPE_NAME: &'static str = "Result";
4144
const TYPE_QPATH: &'static [&'static str] = &paths::RESULT;
@@ -120,9 +123,9 @@ pub(crate) trait BindInsteadOfMap {
120123
}
121124
}
122125

123-
fn lint_closure(cx: &LateContext<'_>, expr: &hir::Expr<'_>, closure_expr: &hir::Expr<'_>) {
126+
fn lint_closure(cx: &LateContext<'_>, expr: &hir::Expr<'_>, closure_expr: &hir::Expr<'_>) -> bool {
124127
let mut suggs = Vec::new();
125-
let can_sugg = find_all_ret_expressions(cx, closure_expr, |ret_expr| {
128+
let can_sugg: bool = find_all_ret_expressions(cx, closure_expr, |ret_expr| {
126129
if_chain! {
127130
if !in_macro(ret_expr.span);
128131
if let hir::ExprKind::Call(ref func_path, ref args) = ret_expr.kind;
@@ -153,21 +156,24 @@ pub(crate) trait BindInsteadOfMap {
153156
)
154157
});
155158
}
159+
can_sugg
156160
}
157161

158162
/// Lint use of `_.and_then(|x| Some(y))` for `Option`s
159-
fn lint(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
163+
fn lint(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) -> bool {
160164
if !match_type(cx, cx.typeck_results().expr_ty(&args[0]), Self::TYPE_QPATH) {
161-
return;
165+
return false;
162166
}
163167

164168
match args[1].kind {
165169
hir::ExprKind::Closure(_, _, body_id, closure_args_span, _) => {
166170
let closure_body = cx.tcx.hir().body(body_id);
167171
let closure_expr = remove_blocks(&closure_body.value);
168172

169-
if !Self::lint_closure_autofixable(cx, expr, args, closure_expr, closure_args_span) {
170-
Self::lint_closure(cx, expr, closure_expr);
173+
if Self::lint_closure_autofixable(cx, expr, args, closure_expr, closure_args_span) {
174+
true
175+
} else {
176+
Self::lint_closure(cx, expr, closure_expr)
171177
}
172178
},
173179
// `_.and_then(Some)` case, which is no-op.
@@ -181,8 +187,9 @@ pub(crate) trait BindInsteadOfMap {
181187
snippet(cx, args[0].span, "..").into(),
182188
Applicability::MachineApplicable,
183189
);
190+
true
184191
},
185-
_ => {},
192+
_ => false,
186193
}
187194
}
188195
}

‎clippy_lints/src/methods/mod.rs

Lines changed: 19 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,15 @@ use rustc_span::source_map::Span;
2525
use rustc_span::symbol::{sym, SymbolStr};
2626

2727
use crate::consts::{constant, Constant};
28+
use crate::utils::eager_or_lazy::is_lazyness_candidate;
2829
use crate::utils::usage::mutated_variables;
2930
use crate::utils::{
3031
contains_ty, get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro,
31-
is_copy, is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats,
32-
last_path_segment, match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls,
33-
method_chain_args, paths, remove_blocks, return_ty, single_segment_path, snippet, snippet_with_applicability,
34-
snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg,
35-
span_lint_and_then, sugg, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
32+
is_copy, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment, match_def_path, match_qpath,
33+
match_trait_method, match_type, match_var, method_calls, method_chain_args, paths, remove_blocks, return_ty,
34+
single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
35+
span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, sugg, walk_ptrs_ty,
36+
walk_ptrs_ty_depth, SpanlessEq,
3637
};
3738

3839
declare_clippy_lint! {
@@ -1454,18 +1455,21 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
14541455
["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0], method_spans[1]),
14551456
["unwrap_or_else", "map"] => {
14561457
if !lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]) {
1457-
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], true, "unwrap_or");
1458+
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "unwrap_or");
14581459
}
14591460
},
14601461
["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]),
14611462
["and_then", ..] => {
1462-
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], false, "and");
1463-
bind_instead_of_map::OptionAndThenSome::lint(cx, expr, arg_lists[0]);
1464-
bind_instead_of_map::ResultAndThenOk::lint(cx, expr, arg_lists[0]);
1463+
let biom_option_linted = bind_instead_of_map::OptionAndThenSome::lint(cx, expr, arg_lists[0]);
1464+
let biom_result_linted = bind_instead_of_map::ResultAndThenOk::lint(cx, expr, arg_lists[0]);
1465+
if !biom_option_linted && !biom_result_linted {
1466+
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "and");
1467+
}
14651468
},
14661469
["or_else", ..] => {
1467-
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], false, "or");
1468-
bind_instead_of_map::ResultOrElseErrInfo::lint(cx, expr, arg_lists[0]);
1470+
if !bind_instead_of_map::ResultOrElseErrInfo::lint(cx, expr, arg_lists[0]) {
1471+
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "or");
1472+
}
14691473
},
14701474
["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
14711475
["next", "skip_while"] => lint_skip_while_next(cx, expr, arg_lists[1]),
@@ -1508,9 +1512,9 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
15081512
["is_file", ..] => lint_filetype_is_file(cx, expr, arg_lists[0]),
15091513
["map", "as_ref"] => lint_option_as_ref_deref(cx, expr, arg_lists[1], arg_lists[0], false),
15101514
["map", "as_mut"] => lint_option_as_ref_deref(cx, expr, arg_lists[1], arg_lists[0], true),
1511-
["unwrap_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], true, "unwrap_or"),
1512-
["get_or_insert_with", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], true, "get_or_insert"),
1513-
["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], true, "ok_or"),
1515+
["unwrap_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "unwrap_or"),
1516+
["get_or_insert_with", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "get_or_insert"),
1517+
["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "ok_or"),
15141518
_ => {},
15151519
}
15161520

@@ -1714,37 +1718,6 @@ fn lint_or_fun_call<'tcx>(
17141718
name: &str,
17151719
args: &'tcx [hir::Expr<'_>],
17161720
) {
1717-
// Searches an expression for method calls or function calls that aren't ctors
1718-
struct FunCallFinder<'a, 'tcx> {
1719-
cx: &'a LateContext<'tcx>,
1720-
found: bool,
1721-
}
1722-
1723-
impl<'a, 'tcx> intravisit::Visitor<'tcx> for FunCallFinder<'a, 'tcx> {
1724-
type Map = Map<'tcx>;
1725-
1726-
fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) {
1727-
let call_found = match &expr.kind {
1728-
// ignore enum and struct constructors
1729-
hir::ExprKind::Call(..) => !is_ctor_or_promotable_const_function(self.cx, expr),
1730-
hir::ExprKind::MethodCall(..) => true,
1731-
_ => false,
1732-
};
1733-
1734-
if call_found {
1735-
self.found |= true;
1736-
}
1737-
1738-
if !self.found {
1739-
intravisit::walk_expr(self, expr);
1740-
}
1741-
}
1742-
1743-
fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> {
1744-
intravisit::NestedVisitorMap::None
1745-
}
1746-
}
1747-
17481721
/// Checks for `unwrap_or(T::new())` or `unwrap_or(T::default())`.
17491722
fn check_unwrap_or_default(
17501723
cx: &LateContext<'_>,
@@ -1825,8 +1798,7 @@ fn lint_or_fun_call<'tcx>(
18251798
if_chain! {
18261799
if know_types.iter().any(|k| k.2.contains(&name));
18271800

1828-
let mut finder = FunCallFinder { cx: &cx, found: false };
1829-
if { finder.visit_expr(&arg); finder.found };
1801+
if is_lazyness_candidate(cx, arg);
18301802
if !contains_return(&arg);
18311803

18321804
let self_ty = cx.typeck_results().expr_ty(self_expr);

‎clippy_lints/src/methods/unnecessary_lazy_eval.rs

Lines changed: 9 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -1,78 +1,17 @@
1-
use crate::utils::{is_type_diagnostic_item, match_qpath, snippet, span_lint_and_sugg};
2-
use if_chain::if_chain;
1+
use crate::utils::{eager_or_lazy, usage};
2+
use crate::utils::{is_type_diagnostic_item, snippet, span_lint_and_sugg};
33
use rustc_errors::Applicability;
44
use rustc_hir as hir;
55
use rustc_lint::LateContext;
66

77
use super::UNNECESSARY_LAZY_EVALUATIONS;
88

9-
// Return true if the expression is an accessor of any of the arguments
10-
fn expr_uses_argument(expr: &hir::Expr<'_>, params: &[hir::Param<'_>]) -> bool {
11-
params.iter().any(|arg| {
12-
if_chain! {
13-
if let hir::PatKind::Binding(_, _, ident, _) = arg.pat.kind;
14-
if let hir::ExprKind::Path(hir::QPath::Resolved(_, ref path)) = expr.kind;
15-
if let [p, ..] = path.segments;
16-
then {
17-
ident.name == p.ident.name
18-
} else {
19-
false
20-
}
21-
}
22-
})
23-
}
24-
25-
fn match_any_qpath(path: &hir::QPath<'_>, paths: &[&[&str]]) -> bool {
26-
paths.iter().any(|candidate| match_qpath(path, candidate))
27-
}
28-
29-
fn can_simplify(expr: &hir::Expr<'_>, params: &[hir::Param<'_>], variant_calls: bool) -> bool {
30-
match expr.kind {
31-
// Closures returning literals can be unconditionally simplified
32-
hir::ExprKind::Lit(_) => true,
33-
34-
hir::ExprKind::Index(ref object, ref index) => {
35-
// arguments are not being indexed into
36-
if expr_uses_argument(object, params) {
37-
false
38-
} else {
39-
// arguments are not used as index
40-
!expr_uses_argument(index, params)
41-
}
42-
},
43-
44-
// Reading fields can be simplified if the object is not an argument of the closure
45-
hir::ExprKind::Field(ref object, _) => !expr_uses_argument(object, params),
46-
47-
// Paths can be simplified if the root is not the argument, this also covers None
48-
hir::ExprKind::Path(_) => !expr_uses_argument(expr, params),
49-
50-
// Calls to Some, Ok, Err can be considered literals if they don't derive an argument
51-
hir::ExprKind::Call(ref func, ref args) => if_chain! {
52-
if variant_calls; // Disable lint when rules conflict with bind_instead_of_map
53-
if let hir::ExprKind::Path(ref path) = func.kind;
54-
if match_any_qpath(path, &[&["Some"], &["Ok"], &["Err"]]);
55-
then {
56-
// Recursively check all arguments
57-
args.iter().all(|arg| can_simplify(arg, params, variant_calls))
58-
} else {
59-
false
60-
}
61-
},
62-
63-
// For anything more complex than the above, a closure is probably the right solution,
64-
// or the case is handled by an other lint
65-
_ => false,
66-
}
67-
}
68-
699
/// lint use of `<fn>_else(simple closure)` for `Option`s and `Result`s that can be
7010
/// replaced with `<fn>(return value of simple closure)`
7111
pub(super) fn lint<'tcx>(
7212
cx: &LateContext<'tcx>,
7313
expr: &'tcx hir::Expr<'_>,
7414
args: &'tcx [hir::Expr<'_>],
75-
allow_variant_calls: bool,
7615
simplify_using: &str,
7716
) {
7817
let is_option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(&args[0]), sym!(option_type));
@@ -81,10 +20,13 @@ pub(super) fn lint<'tcx>(
8120
if is_option || is_result {
8221
if let hir::ExprKind::Closure(_, _, eid, _, _) = args[1].kind {
8322
let body = cx.tcx.hir().body(eid);
84-
let ex = &body.value;
85-
let params = &body.params;
23+
let body_expr = &body.value;
24+
25+
if usage::BindingUsageFinder::are_params_used(cx, body) {
26+
return;
27+
}
8628

87-
if can_simplify(ex, params, allow_variant_calls) {
29+
if eager_or_lazy::is_eagerness_candidate(cx, body_expr) {
8830
let msg = if is_option {
8931
"unnecessary closure used to substitute value for `Option::None`"
9032
} else {
@@ -101,7 +43,7 @@ pub(super) fn lint<'tcx>(
10143
"{0}.{1}({2})",
10244
snippet(cx, args[0].span, ".."),
10345
simplify_using,
104-
snippet(cx, ex.span, ".."),
46+
snippet(cx, body_expr.span, ".."),
10547
),
10648
Applicability::MachineApplicable,
10749
);

‎clippy_lints/src/option_if_let_else.rs

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
use crate::utils;
2+
use crate::utils::eager_or_lazy;
23
use crate::utils::sugg::Sugg;
34
use crate::utils::{is_type_diagnostic_item, paths, span_lint_and_sugg};
45
use if_chain::if_chain;
@@ -13,22 +14,16 @@ use rustc_session::{declare_lint_pass, declare_tool_lint};
1314
declare_clippy_lint! {
1415
/// **What it does:**
1516
/// Lints usage of `if let Some(v) = ... { y } else { x }` which is more
16-
/// idiomatically done with `Option::map_or` (if the else bit is a simple
17-
/// expression) or `Option::map_or_else` (if the else bit is a longer
18-
/// block).
17+
/// idiomatically done with `Option::map_or` (if the else bit is a pure
18+
/// expression) or `Option::map_or_else` (if the else bit is an impure
19+
/// expresion).
1920
///
2021
/// **Why is this bad?**
2122
/// Using the dedicated functions of the Option type is clearer and
2223
/// more concise than an if let expression.
2324
///
2425
/// **Known problems:**
25-
/// This lint uses whether the block is just an expression or if it has
26-
/// more statements to decide whether to use `Option::map_or` or
27-
/// `Option::map_or_else`. If you have a single expression which calls
28-
/// an expensive function, then it would be more efficient to use
29-
/// `Option::map_or_else`, but this lint would suggest `Option::map_or`.
30-
///
31-
/// Also, this lint uses a deliberately conservative metric for checking
26+
/// This lint uses a deliberately conservative metric for checking
3227
/// if the inside of either body contains breaks or continues which will
3328
/// cause it to not suggest a fix if either block contains a loop with
3429
/// continues or breaks contained within the loop.
@@ -92,13 +87,15 @@ struct OptionIfLetElseOccurence {
9287
struct ReturnBreakContinueMacroVisitor {
9388
seen_return_break_continue: bool,
9489
}
90+
9591
impl ReturnBreakContinueMacroVisitor {
9692
fn new() -> ReturnBreakContinueMacroVisitor {
9793
ReturnBreakContinueMacroVisitor {
9894
seen_return_break_continue: false,
9995
}
10096
}
10197
}
98+
10299
impl<'tcx> Visitor<'tcx> for ReturnBreakContinueMacroVisitor {
103100
type Map = Map<'tcx>;
104101
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
@@ -157,7 +154,7 @@ fn extract_body_from_arm<'a>(arm: &'a Arm<'a>) -> Option<&'a Expr<'a>> {
157154
}
158155

159156
/// If this is the else body of an if/else expression, then we need to wrap
160-
/// it in curcly braces. Otherwise, we don't.
157+
/// it in curly braces. Otherwise, we don't.
161158
fn should_wrap_in_braces(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
162159
utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| {
163160
if let Some(Expr {
@@ -199,7 +196,10 @@ fn format_option_in_sugg(cx: &LateContext<'_>, cond_expr: &Expr<'_>, as_ref: boo
199196
/// If this expression is the option if let/else construct we're detecting, then
200197
/// this function returns an `OptionIfLetElseOccurence` struct with details if
201198
/// this construct is found, or None if this construct is not found.
202-
fn detect_option_if_let_else(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<OptionIfLetElseOccurence> {
199+
fn detect_option_if_let_else<'tcx>(
200+
cx: &'_ LateContext<'tcx>,
201+
expr: &'_ Expr<'tcx>,
202+
) -> Option<OptionIfLetElseOccurence> {
203203
if_chain! {
204204
if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly
205205
if let ExprKind::Match(cond_expr, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind;
@@ -214,10 +214,7 @@ fn detect_option_if_let_else(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<Op
214214
let capture_mut = if bind_annotation == &BindingAnnotation::Mutable { "mut " } else { "" };
215215
let some_body = extract_body_from_arm(&arms[0])?;
216216
let none_body = extract_body_from_arm(&arms[1])?;
217-
let method_sugg = match &none_body.kind {
218-
ExprKind::Block(..) => "map_or_else",
219-
_ => "map_or",
220-
};
217+
let method_sugg = if eager_or_lazy::is_eagerness_candidate(cx, none_body) { "map_or" } else { "map_or_else" };
221218
let capture_name = id.name.to_ident_string();
222219
let wrap_braces = should_wrap_in_braces(cx, expr);
223220
let (as_ref, as_mut) = match &cond_expr.kind {
@@ -243,8 +240,8 @@ fn detect_option_if_let_else(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<Op
243240
}
244241
}
245242

246-
impl<'a> LateLintPass<'a> for OptionIfLetElse {
247-
fn check_expr(&mut self, cx: &LateContext<'a>, expr: &Expr<'_>) {
243+
impl<'tcx> LateLintPass<'tcx> for OptionIfLetElse {
244+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
248245
if let Some(detection) = detect_option_if_let_else(cx, expr) {
249246
span_lint_and_sugg(
250247
cx,

0 commit comments

Comments
 (0)