Skip to content

Commit

Permalink
Auto merge of #5301 - JarredAllen:option_if_let_else, r=flip1995
Browse files Browse the repository at this point in the history
Suggest `Option::map_or`(_else) for `if let Some { y } else { x }`

Fixes #5203

There are two issues with this code that I have noticed:

- Use of `Option::map_or` causes it to always evaluate the code in the else block. If that block is computationally expensive or if it updates some state (such as getting the next value from an iterator), then this change would cause the code to behave differently. In either of those circumstances, it should suggest Option::map_or_else, which takes both cases as a closure and runs one. However, I don't know how to check if the expression would change some state, so I left the lint's applicability as MaybeIncorrect.

- There are lints which can trigger on specific sub-cases of this lint (`if_let_some_result`, `question_mark`, and `while_let_loop`) and suggest different changes (usually better ones because they're more specific). Is this acceptable for clippy to give multiple suggestions, or should I have the code check if those other lints trigger and then not trigger this lint if they do?

changelog: Add lint [`option_if_let_else`]
  • Loading branch information
bors committed Jul 6, 2020
2 parents 57cdf2d + c8f700e commit ac85692
Show file tree
Hide file tree
Showing 26 changed files with 760 additions and 228 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1577,6 +1577,7 @@ Released 2018-09-13
[`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
[`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref
[`option_env_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_env_unwrap
[`option_if_let_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_if_let_else
[`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none
[`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn
[`option_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_option
Expand Down
20 changes: 9 additions & 11 deletions clippy_lints/src/attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -481,15 +481,14 @@ fn is_relevant_trait(cx: &LateContext<'_>, item: &TraitItem<'_>) -> bool {
}

fn is_relevant_block(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, block: &Block<'_>) -> bool {
if let Some(stmt) = block.stmts.first() {
match &stmt.kind {
block.stmts.first().map_or(
block.expr.as_ref().map_or(false, |e| is_relevant_expr(cx, tables, e)),
|stmt| match &stmt.kind {
StmtKind::Local(_) => true,
StmtKind::Expr(expr) | StmtKind::Semi(expr) => is_relevant_expr(cx, tables, expr),
_ => false,
}
} else {
block.expr.as_ref().map_or(false, |e| is_relevant_expr(cx, tables, e))
}
},
)
}

fn is_relevant_expr(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, expr: &Expr<'_>) -> bool {
Expand All @@ -499,11 +498,10 @@ fn is_relevant_expr(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, expr: &
ExprKind::Ret(None) | ExprKind::Break(_, None) => false,
ExprKind::Call(path_expr, _) => {
if let ExprKind::Path(qpath) = &path_expr.kind {
if let Some(fun_id) = tables.qpath_res(qpath, path_expr.hir_id).opt_def_id() {
!match_def_path(cx, fun_id, &paths::BEGIN_PANIC)
} else {
true
}
tables
.qpath_res(qpath, path_expr.hir_id)
.opt_def_id()
.map_or(true, |fun_id| !match_def_path(cx, fun_id, &paths::BEGIN_PANIC))
} else {
true
}
Expand Down
9 changes: 3 additions & 6 deletions clippy_lints/src/if_let_mutex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,10 @@ impl<'tcx> Visitor<'tcx> for ArmVisitor<'_, 'tcx> {
}
}

impl<'tcx> ArmVisitor<'_, 'tcx> {
impl<'tcx, 'l> ArmVisitor<'tcx, 'l> {
fn same_mutex(&self, cx: &LateContext<'_>, op_mutex: &Expr<'_>) -> bool {
if let Some(arm_mutex) = self.found_mutex {
SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex)
} else {
false
}
self.found_mutex
.map_or(false, |arm_mutex| SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex))
}
}

Expand Down
16 changes: 6 additions & 10 deletions clippy_lints/src/len_zero.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,16 +302,12 @@ fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {

let ty = &walk_ptrs_ty(cx.tables().expr_ty(expr));
match ty.kind {
ty::Dynamic(ref tt, ..) => {
if let Some(principal) = tt.principal() {
cx.tcx
.associated_items(principal.def_id())
.in_definition_order()
.any(|item| is_is_empty(cx, &item))
} else {
false
}
},
ty::Dynamic(ref tt, ..) => tt.principal().map_or(false, |principal| {
cx.tcx
.associated_items(principal.def_id())
.in_definition_order()
.any(|item| is_is_empty(cx, &item))
}),
ty::Projection(ref proj) => has_is_empty_impl(cx, proj.item_def_id),
ty::Adt(id, _) => has_is_empty_impl(cx, id.did),
ty::Array(..) | ty::Slice(..) | ty::Str => true,
Expand Down
4 changes: 4 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ mod non_copy_const;
mod non_expressive_names;
mod open_options;
mod option_env_unwrap;
mod option_if_let_else;
mod overflow_check_conditional;
mod panic_unimplemented;
mod partialeq_ne_impl;
Expand Down Expand Up @@ -734,6 +735,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&non_expressive_names::SIMILAR_NAMES,
&open_options::NONSENSICAL_OPEN_OPTIONS,
&option_env_unwrap::OPTION_ENV_UNWRAP,
&option_if_let_else::OPTION_IF_LET_ELSE,
&overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
&panic_unimplemented::PANIC,
&panic_unimplemented::PANIC_PARAMS,
Expand Down Expand Up @@ -1052,6 +1054,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default());
store.register_late_pass(|| box unnamed_address::UnnamedAddress);
store.register_late_pass(|| box dereference::Dereferencing);
store.register_late_pass(|| box option_if_let_else::OptionIfLetElse);
store.register_late_pass(|| box future_not_send::FutureNotSend);
store.register_late_pass(|| box utils::internal_lints::CollapsibleCalls);
store.register_late_pass(|| box if_let_mutex::IfLetMutex);
Expand Down Expand Up @@ -1158,6 +1161,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&needless_continue::NEEDLESS_CONTINUE),
LintId::of(&needless_pass_by_value::NEEDLESS_PASS_BY_VALUE),
LintId::of(&non_expressive_names::SIMILAR_NAMES),
LintId::of(&option_if_let_else::OPTION_IF_LET_ELSE),
LintId::of(&ranges::RANGE_PLUS_ONE),
LintId::of(&shadow::SHADOW_UNRELATED),
LintId::of(&strings::STRING_ADD_ASSIGN),
Expand Down
9 changes: 6 additions & 3 deletions clippy_lints/src/literal_representation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,13 @@ impl LiteralDigitGrouping {

let (part, mistyped_suffixes, missing_char) = if let Some((_, exponent)) = &mut num_lit.exponent {
(exponent, &["32", "64"][..], 'f')
} else if let Some(fraction) = &mut num_lit.fraction {
(fraction, &["32", "64"][..], 'f')
} else {
(&mut num_lit.integer, &["8", "16", "32", "64"][..], 'i')
num_lit
.fraction
.as_mut()
.map_or((&mut num_lit.integer, &["8", "16", "32", "64"][..], 'i'), |fraction| {
(fraction, &["32", "64"][..], 'f')
})
};

let mut split = part.rsplit('_');
Expand Down
32 changes: 9 additions & 23 deletions clippy_lints/src/loops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -686,13 +686,9 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
NeverLoopResult::AlwaysBreak
}
},
ExprKind::Break(_, ref e) | ExprKind::Ret(ref e) => {
if let Some(ref e) = *e {
combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak)
} else {
NeverLoopResult::AlwaysBreak
}
},
ExprKind::Break(_, ref e) | ExprKind::Ret(ref e) => e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| {
combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak)
}),
ExprKind::InlineAsm(ref asm) => asm
.operands
.iter()
Expand Down Expand Up @@ -1881,13 +1877,9 @@ fn is_ref_iterable_type(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
fn is_iterable_array<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool {
// IntoIterator is currently only implemented for array sizes <= 32 in rustc
match ty.kind {
ty::Array(_, n) => {
if let Some(val) = n.try_eval_usize(cx.tcx, cx.param_env) {
(0..=32).contains(&val)
} else {
false
}
},
ty::Array(_, n) => n
.try_eval_usize(cx.tcx, cx.param_env)
.map_or(false, |val| (0..=32).contains(&val)),
_ => false,
}
}
Expand All @@ -1899,11 +1891,7 @@ fn extract_expr_from_first_stmt<'tcx>(block: &Block<'tcx>) -> Option<&'tcx Expr<
return None;
}
if let StmtKind::Local(ref local) = block.stmts[0].kind {
if let Some(expr) = local.init {
Some(expr)
} else {
None
}
local.init //.map(|expr| expr)
} else {
None
}
Expand Down Expand Up @@ -2023,15 +2011,13 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> {
if let PatKind::Binding(.., ident, _) = local.pat.kind {
self.name = Some(ident.name);

self.state = if let Some(ref init) = local.init {
self.state = local.init.as_ref().map_or(VarState::Declared, |init| {
if is_integer_const(&self.cx, init, 0) {
VarState::Warn
} else {
VarState::Declared
}
} else {
VarState::Declared
}
})
}
}
}
Expand Down
10 changes: 3 additions & 7 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2460,13 +2460,9 @@ fn derefs_to_slice<'tcx>(
ty::Slice(_) => true,
ty::Adt(def, _) if def.is_box() => may_slice(cx, ty.boxed_ty()),
ty::Adt(..) => is_type_diagnostic_item(cx, ty, sym!(vec_type)),
ty::Array(_, size) => {
if let Some(size) = size.try_eval_usize(cx.tcx, cx.param_env) {
size < 32
} else {
false
}
},
ty::Array(_, size) => size
.try_eval_usize(cx.tcx, cx.param_env)
.map_or(false, |size| size < 32),
ty::Ref(_, inner, _) => may_slice(cx, inner),
_ => false,
}
Expand Down
11 changes: 4 additions & 7 deletions clippy_lints/src/methods/unnecessary_filter_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,10 @@ fn check_expression<'tcx>(cx: &LateContext<'tcx>, arg_id: hir::HirId, expr: &'tc
}
(true, true)
},
hir::ExprKind::Block(ref block, _) => {
if let Some(expr) = &block.expr {
check_expression(cx, arg_id, &expr)
} else {
(false, false)
}
},
hir::ExprKind::Block(ref block, _) => block
.expr
.as_ref()
.map_or((false, false), |expr| check_expression(cx, arg_id, &expr)),
hir::ExprKind::Match(_, arms, _) => {
let mut found_mapping = false;
let mut found_filtering = false;
Expand Down
25 changes: 12 additions & 13 deletions clippy_lints/src/minmax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl<'tcx> LateLintPass<'tcx> for MinMaxPass {
}
}

#[derive(PartialEq, Eq, Debug)]
#[derive(PartialEq, Eq, Debug, Clone, Copy)]
enum MinMax {
Min,
Max,
Expand Down Expand Up @@ -86,16 +86,15 @@ fn fetch_const<'a>(cx: &LateContext<'_>, args: &'a [Expr<'a>], m: MinMax) -> Opt
if args.len() != 2 {
return None;
}
if let Some(c) = constant_simple(cx, cx.tables(), &args[0]) {
if constant_simple(cx, cx.tables(), &args[1]).is_none() {
// otherwise ignore
Some((m, c, &args[1]))
} else {
None
}
} else if let Some(c) = constant_simple(cx, cx.tables(), &args[1]) {
Some((m, c, &args[0]))
} else {
None
}
constant_simple(cx, cx.tables(), &args[0]).map_or_else(
|| constant_simple(cx, cx.tables(), &args[1]).map(|c| (m, c, &args[0])),
|c| {
if constant_simple(cx, cx.tables(), &args[1]).is_none() {
// otherwise ignore
Some((m, c, &args[1]))
} else {
None
}
},
)
}
14 changes: 4 additions & 10 deletions clippy_lints/src/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -682,16 +682,10 @@ fn check_to_owned(cx: &LateContext<'_>, expr: &Expr<'_>, other: &Expr<'_>, left:
/// `unused_variables`'s idea
/// of what it means for an expression to be "used".
fn is_used(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
if let Some(parent) = get_parent_expr(cx, expr) {
match parent.kind {
ExprKind::Assign(_, ref rhs, _) | ExprKind::AssignOp(_, _, ref rhs) => {
SpanlessEq::new(cx).eq_expr(rhs, expr)
},
_ => is_used(cx, parent),
}
} else {
true
}
get_parent_expr(cx, expr).map_or(true, |parent| match parent.kind {
ExprKind::Assign(_, ref rhs, _) | ExprKind::AssignOp(_, _, ref rhs) => SpanlessEq::new(cx).eq_expr(rhs, expr),
_ => is_used(cx, parent),
})
}

/// Tests whether an expression is in a macro expansion (e.g., something
Expand Down
Loading

0 comments on commit ac85692

Please sign in to comment.