Skip to content

Suggest Option::map_or(_else) for if let Some { y } else { x } #5301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jul 6, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
20 changes: 9 additions & 11 deletions clippy_lints/src/attrs.rs
Original file line number Diff line number Diff line change
@@ -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 {
@@ -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
}
9 changes: 3 additions & 6 deletions clippy_lints/src/if_let_mutex.rs
Original file line number Diff line number Diff line change
@@ -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))
}
}

16 changes: 6 additions & 10 deletions clippy_lints/src/len_zero.rs
Original file line number Diff line number Diff line change
@@ -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,
4 changes: 4 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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;
@@ -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,
@@ -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);
@@ -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),
9 changes: 6 additions & 3 deletions clippy_lints/src/literal_representation.rs
Original file line number Diff line number Diff line change
@@ -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('_');
32 changes: 9 additions & 23 deletions clippy_lints/src/loops.rs
Original file line number Diff line number Diff line change
@@ -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()
@@ -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,
}
}
@@ -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
}
@@ -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
}
})
}
}
}
10 changes: 3 additions & 7 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
@@ -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,
}
11 changes: 4 additions & 7 deletions clippy_lints/src/methods/unnecessary_filter_map.rs
Original file line number Diff line number Diff line change
@@ -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;
25 changes: 12 additions & 13 deletions clippy_lints/src/minmax.rs
Original file line number Diff line number Diff line change
@@ -53,7 +53,7 @@ impl<'tcx> LateLintPass<'tcx> for MinMaxPass {
}
}

#[derive(PartialEq, Eq, Debug)]
#[derive(PartialEq, Eq, Debug, Clone, Copy)]
enum MinMax {
Min,
Max,
@@ -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
@@ -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
267 changes: 267 additions & 0 deletions clippy_lints/src/option_if_let_else.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,267 @@
use crate::utils;
use crate::utils::sugg::Sugg;
use crate::utils::{match_type, paths, span_lint_and_sugg};
use if_chain::if_chain;

use rustc_errors::Applicability;
use rustc_hir::intravisit::{NestedVisitorMap, Visitor};
use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, UnOp};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::hir::map::Map;
use rustc_session::{declare_lint_pass, declare_tool_lint};

declare_clippy_lint! {
/// **What it does:**
/// Lints usage of `if let Some(v) = ... { y } else { x }` which is more
/// idiomatically done with `Option::map_or` (if the else bit is a simple
/// expression) or `Option::map_or_else` (if the else bit is a longer
/// block).
///
/// **Why is this bad?**
/// Using the dedicated functions of the Option type is clearer and
/// more concise than an if let expression.
///
/// **Known problems:**
/// This lint uses whether the block is just an expression or if it has
/// more statements to decide whether to use `Option::map_or` or
/// `Option::map_or_else`. If you have a single expression which calls
/// an expensive function, then it would be more efficient to use
/// `Option::map_or_else`, but this lint would suggest `Option::map_or`.
///
/// Also, this lint uses a deliberately conservative metric for checking
/// if the inside of either body contains breaks or continues which will
/// cause it to not suggest a fix if either block contains a loop with
/// continues or breaks contained within the loop.
///
/// **Example:**
///
/// ```rust
/// # let optional: Option<u32> = Some(0);
/// # fn do_complicated_function() -> u32 { 5 };
/// let _ = if let Some(foo) = optional {
/// foo
/// } else {
/// 5
/// };
/// let _ = if let Some(foo) = optional {
/// foo
/// } else {
/// let y = do_complicated_function();
/// y*y
/// };
/// ```
///
/// should be
///
/// ```rust
/// # let optional: Option<u32> = Some(0);
/// # fn do_complicated_function() -> u32 { 5 };
/// let _ = optional.map_or(5, |foo| foo);
/// let _ = optional.map_or_else(||{
/// let y = do_complicated_function();
/// y*y
/// }, |foo| foo);
/// ```
pub OPTION_IF_LET_ELSE,
pedantic,
"reimplementation of Option::map_or"
}

declare_lint_pass!(OptionIfLetElse => [OPTION_IF_LET_ELSE]);

/// Returns true iff the given expression is the result of calling `Result::ok`
fn is_result_ok(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool {
if let ExprKind::MethodCall(ref path, _, &[ref receiver], _) = &expr.kind {
path.ident.name.to_ident_string() == "ok" && match_type(cx, &cx.tables().expr_ty(&receiver), &paths::RESULT)
} else {
false
}
}

/// A struct containing information about occurences of the
/// `if let Some(..) = .. else` construct that this lint detects.
struct OptionIfLetElseOccurence {
option: String,
method_sugg: String,
some_expr: String,
none_expr: String,
wrap_braces: bool,
}

struct ReturnBreakContinueMacroVisitor {
seen_return_break_continue: bool,
}
impl ReturnBreakContinueMacroVisitor {
fn new() -> ReturnBreakContinueMacroVisitor {
ReturnBreakContinueMacroVisitor {
seen_return_break_continue: false,
}
}
}
impl<'tcx> Visitor<'tcx> for ReturnBreakContinueMacroVisitor {
type Map = Map<'tcx>;
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
NestedVisitorMap::None
}

fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) {
if self.seen_return_break_continue {
// No need to look farther if we've already seen one of them
return;
}
match &ex.kind {
ExprKind::Ret(..) | ExprKind::Break(..) | ExprKind::Continue(..) => {
self.seen_return_break_continue = true;
},
// Something special could be done here to handle while or for loop
// desugaring, as this will detect a break if there's a while loop
// or a for loop inside the expression.
_ => {
if utils::in_macro(ex.span) {
self.seen_return_break_continue = true;
} else {
rustc_hir::intravisit::walk_expr(self, ex);
}
},
}
}
}

fn contains_return_break_continue_macro(expression: &Expr<'_>) -> bool {
let mut recursive_visitor = ReturnBreakContinueMacroVisitor::new();
recursive_visitor.visit_expr(expression);
recursive_visitor.seen_return_break_continue
}

/// Extracts the body of a given arm. If the arm contains only an expression,
/// then it returns the expression. Otherwise, it returns the entire block
fn extract_body_from_arm<'a>(arm: &'a Arm<'a>) -> Option<&'a Expr<'a>> {
if let ExprKind::Block(
Block {
stmts: statements,
expr: Some(expr),
..
},
_,
) = &arm.body.kind
{
if let [] = statements {
Some(&expr)
} else {
Some(&arm.body)
}
} else {
None
}
}

/// If this is the else body of an if/else expression, then we need to wrap
/// it in curcly braces. Otherwise, we don't.
fn should_wrap_in_braces(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| {
if let Some(Expr {
kind:
ExprKind::Match(
_,
arms,
MatchSource::IfDesugar {
contains_else_clause: true,
}
| MatchSource::IfLetDesugar {
contains_else_clause: true,
},
),
..
}) = parent.expr
{
expr.hir_id == arms[1].body.hir_id
} else {
false
}
})
}

fn format_option_in_sugg(cx: &LateContext<'_>, cond_expr: &Expr<'_>, as_ref: bool, as_mut: bool) -> String {
format!(
"{}{}",
Sugg::hir(cx, cond_expr, "..").maybe_par(),
if as_mut {
".as_mut()"
} else if as_ref {
".as_ref()"
} else {
""
}
)
}

/// If this expression is the option if let/else construct we're detecting, then
/// this function returns an `OptionIfLetElseOccurence` struct with details if
/// this construct is found, or None if this construct is not found.
fn detect_option_if_let_else(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<OptionIfLetElseOccurence> {
if_chain! {
if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly
if let ExprKind::Match(cond_expr, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind;
if arms.len() == 2;
if !is_result_ok(cx, cond_expr); // Don't lint on Result::ok because a different lint does it already
if let PatKind::TupleStruct(struct_qpath, &[inner_pat], _) = &arms[0].pat.kind;
if utils::match_qpath(struct_qpath, &paths::OPTION_SOME);
if let PatKind::Binding(bind_annotation, _, id, _) = &inner_pat.kind;
if !contains_return_break_continue_macro(arms[0].body);
if !contains_return_break_continue_macro(arms[1].body);
then {
let capture_mut = if bind_annotation == &BindingAnnotation::Mutable { "mut " } else { "" };
let some_body = extract_body_from_arm(&arms[0])?;
let none_body = extract_body_from_arm(&arms[1])?;
let method_sugg = match &none_body.kind {
ExprKind::Block(..) => "map_or_else",
_ => "map_or",
};
let capture_name = id.name.to_ident_string();
let wrap_braces = should_wrap_in_braces(cx, expr);
let (as_ref, as_mut) = match &cond_expr.kind {
ExprKind::AddrOf(_, Mutability::Not, _) => (true, false),
ExprKind::AddrOf(_, Mutability::Mut, _) => (false, true),
_ => (bind_annotation == &BindingAnnotation::Ref, bind_annotation == &BindingAnnotation::RefMut),
};
let cond_expr = match &cond_expr.kind {
// Pointer dereferencing happens automatically, so we can omit it in the suggestion
ExprKind::Unary(UnOp::UnDeref, expr) | ExprKind::AddrOf(_, _, expr) => expr,
_ => cond_expr,
};
Some(OptionIfLetElseOccurence {
option: format_option_in_sugg(cx, cond_expr, as_ref, as_mut),
method_sugg: method_sugg.to_string(),
some_expr: format!("|{}{}| {}", capture_mut, capture_name, Sugg::hir(cx, some_body, "..")),
none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, "..")),
wrap_braces,
})
} else {
None
}
}
}

impl<'a> LateLintPass<'a> for OptionIfLetElse {
fn check_expr(&mut self, cx: &LateContext<'a>, expr: &Expr<'_>) {
if let Some(detection) = detect_option_if_let_else(cx, expr) {
span_lint_and_sugg(
cx,
OPTION_IF_LET_ELSE,
expr.span,
format!("use Option::{} instead of an if let/else", detection.method_sugg).as_str(),
"try",
format!(
"{}{}.{}({}, {}){}",
if detection.wrap_braces { "{ " } else { "" },
detection.option,
detection.method_sugg,
detection.none_expr,
detection.some_expr,
if detection.wrap_braces { " }" } else { "" },
),
Applicability::MaybeIncorrect,
);
}
}
}
18 changes: 9 additions & 9 deletions clippy_lints/src/returns.rs
Original file line number Diff line number Diff line change
@@ -259,15 +259,15 @@ fn is_unit_expr(expr: &ast::Expr) -> bool {

fn lint_unneeded_unit_return(cx: &EarlyContext<'_>, ty: &ast::Ty, span: Span) {
let (ret_span, appl) = if let Ok(fn_source) = cx.sess().source_map().span_to_snippet(span.with_hi(ty.span.hi())) {
if let Some(rpos) = fn_source.rfind("->") {
#[allow(clippy::cast_possible_truncation)]
(
ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)),
Applicability::MachineApplicable,
)
} else {
(ty.span, Applicability::MaybeIncorrect)
}
fn_source
.rfind("->")
.map_or((ty.span, Applicability::MaybeIncorrect), |rpos| {
(
#[allow(clippy::cast_possible_truncation)]
ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)),
Applicability::MachineApplicable,
)
})
} else {
(ty.span, Applicability::MaybeIncorrect)
};
12 changes: 4 additions & 8 deletions clippy_lints/src/shadow.rs
Original file line number Diff line number Diff line change
@@ -165,14 +165,10 @@ fn check_local<'tcx>(cx: &LateContext<'tcx>, local: &'tcx Local<'_>, bindings: &

fn is_binding(cx: &LateContext<'_>, pat_id: HirId) -> bool {
let var_ty = cx.tables().node_type_opt(pat_id);
if let Some(var_ty) = var_ty {
match var_ty.kind {
ty::Adt(..) => false,
_ => true,
}
} else {
false
}
var_ty.map_or(false, |var_ty| match var_ty.kind {
ty::Adt(..) => false,
_ => true,
})
}

fn check_pat<'tcx>(
23 changes: 13 additions & 10 deletions clippy_lints/src/types.rs
Original file line number Diff line number Diff line change
@@ -1205,16 +1205,19 @@ fn span_lossless_lint(cx: &LateContext<'_>, expr: &Expr<'_>, op: &Expr<'_>, cast
// has parens on the outside, they are no longer needed.
let mut applicability = Applicability::MachineApplicable;
let opt = snippet_opt(cx, op.span);
let sugg = if let Some(ref snip) = opt {
if should_strip_parens(op, snip) {
&snip[1..snip.len() - 1]
} else {
snip.as_str()
}
} else {
applicability = Applicability::HasPlaceholders;
".."
};
let sugg = opt.as_ref().map_or_else(
|| {
applicability = Applicability::HasPlaceholders;
".."
},
|snip| {
if should_strip_parens(op, snip) {
&snip[1..snip.len() - 1]
} else {
snip.as_str()
}
},
);

span_lint_and_sugg(
cx,
9 changes: 4 additions & 5 deletions clippy_lints/src/use_self.rs
Original file line number Diff line number Diff line change
@@ -167,14 +167,13 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf {
if let TyKind::Path(QPath::Resolved(_, ref item_path)) = item_type.kind;
then {
let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).args;
let should_check = if let Some(ref params) = *parameters {
!params.parenthesized && !params.args.iter().any(|arg| match arg {
let should_check = parameters.as_ref().map_or(
true,
|params| !params.parenthesized && !params.args.iter().any(|arg| match arg {
GenericArg::Lifetime(_) => true,
_ => false,
})
} else {
true
};
);

if should_check {
let visitor = &mut UseSelfVisitor {
69 changes: 36 additions & 33 deletions clippy_lints/src/utils/attrs.rs
Original file line number Diff line number Diff line change
@@ -65,42 +65,45 @@ pub fn get_attr<'a>(
};
let attr_segments = &attr.path.segments;
if attr_segments.len() == 2 && attr_segments[0].ident.to_string() == "clippy" {
if let Some(deprecation_status) =
BUILTIN_ATTRIBUTES
.iter()
.find_map(|(builtin_name, deprecation_status)| {
if *builtin_name == attr_segments[1].ident.to_string() {
Some(deprecation_status)
} else {
None
}
})
{
let mut diag = sess.struct_span_err(attr_segments[1].ident.span, "Usage of deprecated attribute");
match *deprecation_status {
DeprecationStatus::Deprecated => {
diag.emit();
false
},
DeprecationStatus::Replaced(new_name) => {
diag.span_suggestion(
attr_segments[1].ident.span,
"consider using",
new_name.to_string(),
Applicability::MachineApplicable,
);
diag.emit();
BUILTIN_ATTRIBUTES
.iter()
.find_map(|(builtin_name, deprecation_status)| {
if *builtin_name == attr_segments[1].ident.to_string() {
Some(deprecation_status)
} else {
None
}
})
.map_or_else(
|| {
sess.span_err(attr_segments[1].ident.span, "Usage of unknown attribute");
false
},
DeprecationStatus::None => {
diag.cancel();
attr_segments[1].ident.to_string() == name
|deprecation_status| {
let mut diag =
sess.struct_span_err(attr_segments[1].ident.span, "Usage of deprecated attribute");
match *deprecation_status {
DeprecationStatus::Deprecated => {
diag.emit();
false
},
DeprecationStatus::Replaced(new_name) => {
diag.span_suggestion(
attr_segments[1].ident.span,
"consider using",
new_name.to_string(),
Applicability::MachineApplicable,
);
diag.emit();
false
},
DeprecationStatus::None => {
diag.cancel();
attr_segments[1].ident.to_string() == name
},
}
},
}
} else {
sess.span_err(attr_segments[1].ident.span, "Usage of unknown attribute");
false
}
)
} else {
false
}
62 changes: 20 additions & 42 deletions clippy_lints/src/utils/mod.rs
Original file line number Diff line number Diff line change
@@ -153,11 +153,7 @@ pub fn is_type_diagnostic_item(cx: &LateContext<'_>, ty: Ty<'_>, diag_item: Symb
pub fn match_trait_method(cx: &LateContext<'_>, expr: &Expr<'_>, path: &[&str]) -> bool {
let def_id = cx.tables().type_dependent_def_id(expr.hir_id).unwrap();
let trt_id = cx.tcx.trait_of_item(def_id);
if let Some(trt_id) = trt_id {
match_def_path(cx, trt_id, path)
} else {
false
}
trt_id.map_or(false, |trt_id| match_def_path(cx, trt_id, path))
}

/// Checks if an expression references a variable of the given name.
@@ -600,21 +596,15 @@ pub fn snippet_block_with_applicability<'a, T: LintContext>(
/// // ^^^^^^^^^^
/// ```
pub fn first_line_of_span<T: LintContext>(cx: &T, span: Span) -> Span {
if let Some(first_char_pos) = first_char_in_first_line(cx, span) {
span.with_lo(first_char_pos)
} else {
span
}
first_char_in_first_line(cx, span).map_or(span, |first_char_pos| span.with_lo(first_char_pos))
}

fn first_char_in_first_line<T: LintContext>(cx: &T, span: Span) -> Option<BytePos> {
let line_span = line_span(cx, span);
if let Some(snip) = snippet_opt(cx, line_span) {
snippet_opt(cx, line_span).and_then(|snip| {
snip.find(|c: char| !c.is_whitespace())
.map(|pos| line_span.lo() + BytePos::from_usize(pos))
} else {
None
}
})
}

/// Returns the indentation of the line of a span
@@ -626,11 +616,7 @@ fn first_char_in_first_line<T: LintContext>(cx: &T, span: Span) -> Option<BytePo
/// // ^^ -- will return 4
/// ```
pub fn indent_of<T: LintContext>(cx: &T, span: Span) -> Option<usize> {
if let Some(snip) = snippet_opt(cx, line_span(cx, span)) {
snip.find(|c: char| !c.is_whitespace())
} else {
None
}
snippet_opt(cx, line_span(cx, span)).and_then(|snip| snip.find(|c: char| !c.is_whitespace()))
}

/// Extends the span to the beginning of the spans line, incl. whitespaces.
@@ -738,25 +724,21 @@ pub fn get_enclosing_block<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio
let enclosing_node = map
.get_enclosing_scope(hir_id)
.and_then(|enclosing_id| map.find(enclosing_id));
if let Some(node) = enclosing_node {
match node {
Node::Block(block) => Some(block),
Node::Item(&Item {
kind: ItemKind::Fn(_, _, eid),
..
})
| Node::ImplItem(&ImplItem {
kind: ImplItemKind::Fn(_, eid),
..
}) => match cx.tcx.hir().body(eid).value.kind {
ExprKind::Block(ref block, _) => Some(block),
_ => None,
},
enclosing_node.and_then(|node| match node {
Node::Block(block) => Some(block),
Node::Item(&Item {
kind: ItemKind::Fn(_, _, eid),
..
})
| Node::ImplItem(&ImplItem {
kind: ImplItemKind::Fn(_, eid),
..
}) => match cx.tcx.hir().body(eid).value.kind {
ExprKind::Block(ref block, _) => Some(block),
_ => None,
}
} else {
None
}
},
_ => None,
})
}

/// Returns the base type for HIR references and pointers.
@@ -1328,11 +1310,7 @@ pub fn is_must_use_func_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
_ => None,
};

if let Some(did) = did {
must_use_attr(&cx.tcx.get_attrs(did)).is_some()
} else {
false
}
did.map_or(false, |did| must_use_attr(&cx.tcx.get_attrs(did)).is_some())
}

pub fn is_no_std_crate(krate: &Crate<'_>) -> bool {
6 changes: 2 additions & 4 deletions clippy_lints/src/utils/numeric_literal.rs
Original file line number Diff line number Diff line change
@@ -200,12 +200,10 @@ impl<'a> NumericLiteral<'a> {

fn split_suffix<'a>(src: &'a str, lit_kind: &LitKind) -> (&'a str, Option<&'a str>) {
debug_assert!(lit_kind.is_numeric());
if let Some(suffix_length) = lit_suffix_length(lit_kind) {
lit_suffix_length(lit_kind).map_or((src, None), |suffix_length| {
let (unsuffixed, suffix) = src.split_at(src.len() - suffix_length);
(unsuffixed, Some(suffix))
} else {
(src, None)
}
})
}

fn lit_suffix_length(lit_kind: &LitKind) -> Option<usize> {
22 changes: 11 additions & 11 deletions clippy_lints/src/utils/sugg.rs
Original file line number Diff line number Diff line change
@@ -492,20 +492,20 @@ fn astbinop2assignop(op: ast::BinOp) -> AssocOp {
/// before it on its line.
fn indentation<T: LintContext>(cx: &T, span: Span) -> Option<String> {
let lo = cx.sess().source_map().lookup_char_pos(span.lo());
if let Some(line) = lo.file.get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */) {
if let Some((pos, _)) = line.char_indices().find(|&(_, c)| c != ' ' && c != '\t') {
// We can mix char and byte positions here because we only consider `[ \t]`.
if lo.col == CharPos(pos) {
Some(line[..pos].into())
lo.file
.get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */)
.and_then(|line| {
if let Some((pos, _)) = line.char_indices().find(|&(_, c)| c != ' ' && c != '\t') {
// We can mix char and byte positions here because we only consider `[ \t]`.
if lo.col == CharPos(pos) {
Some(line[..pos].into())
} else {
None
}
} else {
None
}
} else {
None
}
} else {
None
}
})
}

/// Convenience extension trait for `DiagnosticBuilder`.
13 changes: 7 additions & 6 deletions clippy_lints/src/write.rs
Original file line number Diff line number Diff line change
@@ -297,12 +297,13 @@ impl EarlyLintPass for Write {
if let (Some(fmt_str), expr) = self.check_tts(cx, &mac.args.inner_tokens(), true) {
if fmt_str.symbol == Symbol::intern("") {
let mut applicability = Applicability::MachineApplicable;
let suggestion = if let Some(e) = expr {
snippet_with_applicability(cx, e.span, "v", &mut applicability)
} else {
applicability = Applicability::HasPlaceholders;
Cow::Borrowed("v")
};
let suggestion = expr.map_or_else(
|| {
applicability = Applicability::HasPlaceholders;
Cow::Borrowed("v")
},
|e| snippet_with_applicability(cx, e.span, "v", &mut Applicability::MachineApplicable),
);

span_lint_and_sugg(
cx,
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
@@ -1620,6 +1620,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "option_env_unwrap",
},
Lint {
name: "option_if_let_else",
group: "pedantic",
desc: "reimplementation of Option::map_or",
deprecation: None,
module: "option_if_let_else",
},
Lint {
name: "option_map_or_none",
group: "style",
12 changes: 2 additions & 10 deletions tests/compile-test.rs
Original file line number Diff line number Diff line change
@@ -12,19 +12,11 @@ use std::path::{Path, PathBuf};
mod cargo;

fn host_lib() -> PathBuf {
if let Some(path) = option_env!("HOST_LIBS") {
PathBuf::from(path)
} else {
cargo::CARGO_TARGET_DIR.join(env!("PROFILE"))
}
option_env!("HOST_LIBS").map_or(cargo::CARGO_TARGET_DIR.join(env!("PROFILE")), PathBuf::from)
}

fn clippy_driver_path() -> PathBuf {
if let Some(path) = option_env!("CLIPPY_DRIVER_PATH") {
PathBuf::from(path)
} else {
cargo::TARGET_LIB.join("clippy-driver")
}
option_env!("CLIPPY_DRIVER_PATH").map_or(cargo::TARGET_LIB.join("clippy-driver"), PathBuf::from)
}

// When we'll want to use `extern crate ..` for a dependency that is used
74 changes: 74 additions & 0 deletions tests/ui/option_if_let_else.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// run-rustfix
#![warn(clippy::option_if_let_else)]

fn bad1(string: Option<&str>) -> (bool, &str) {
string.map_or((false, "hello"), |x| (true, x))
}

fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
if string.is_none() {
None
} else { string.map_or(Some((false, "")), |x| Some((true, x))) }
}

fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
let _ = string.map_or(0, |s| s.len());
let _ = num.as_ref().map_or(&0, |s| s);
let _ = num.as_mut().map_or(&mut 0, |s| {
*s += 1;
s
});
let _ = num.as_ref().map_or(&0, |s| s);
let _ = num.map_or(0, |mut s| {
s += 1;
s
});
let _ = num.as_mut().map_or(&mut 0, |s| {
*s += 1;
s
});
}

fn longer_body(arg: Option<u32>) -> u32 {
arg.map_or(13, |x| {
let y = x * x;
y * y
})
}

fn test_map_or_else(arg: Option<u32>) {
let _ = arg.map_or_else(|| {
let mut y = 1;
y = (y + 2 / y) / 2;
y = (y + 2 / y) / 2;
y
}, |x| x * x * x * x);
}

fn negative_tests(arg: Option<u32>) -> u32 {
let _ = if let Some(13) = arg { "unlucky" } else { "lucky" };
for _ in 0..10 {
let _ = if let Some(x) = arg {
x
} else {
continue;
};
}
let _ = if let Some(x) = arg {
return x;
} else {
5
};
7
}

fn main() {
let optional = Some(5);
let _ = optional.map_or(5, |x| x + 2);
let _ = bad1(None);
let _ = else_if_option(None);
unop_bad(&None, None);
let _ = longer_body(None);
test_map_or_else(None);
let _ = negative_tests(None);
}
92 changes: 92 additions & 0 deletions tests/ui/option_if_let_else.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// run-rustfix
#![warn(clippy::option_if_let_else)]

fn bad1(string: Option<&str>) -> (bool, &str) {
if let Some(x) = string {
(true, x)
} else {
(false, "hello")
}
}

fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
if string.is_none() {
None
} else if let Some(x) = string {
Some((true, x))
} else {
Some((false, ""))
}
}

fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
let _ = if let Some(s) = *string { s.len() } else { 0 };
let _ = if let Some(s) = &num { s } else { &0 };
let _ = if let Some(s) = &mut num {
*s += 1;
s
} else {
&mut 0
};
let _ = if let Some(ref s) = num { s } else { &0 };
let _ = if let Some(mut s) = num {
s += 1;
s
} else {
0
};
let _ = if let Some(ref mut s) = num {
*s += 1;
s
} else {
&mut 0
};
}

fn longer_body(arg: Option<u32>) -> u32 {
if let Some(x) = arg {
let y = x * x;
y * y
} else {
13
}
}

fn test_map_or_else(arg: Option<u32>) {
let _ = if let Some(x) = arg {
x * x * x * x
} else {
let mut y = 1;
y = (y + 2 / y) / 2;
y = (y + 2 / y) / 2;
y
};
}

fn negative_tests(arg: Option<u32>) -> u32 {
let _ = if let Some(13) = arg { "unlucky" } else { "lucky" };
for _ in 0..10 {
let _ = if let Some(x) = arg {
x
} else {
continue;
};
}
let _ = if let Some(x) = arg {
return x;
} else {
5
};
7
}

fn main() {
let optional = Some(5);
let _ = if let Some(x) = optional { x + 2 } else { 5 };
let _ = bad1(None);
let _ = else_if_option(None);
unop_bad(&None, None);
let _ = longer_body(None);
test_map_or_else(None);
let _ = negative_tests(None);
}
151 changes: 151 additions & 0 deletions tests/ui/option_if_let_else.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:5:5
|
LL | / if let Some(x) = string {
LL | | (true, x)
LL | | } else {
LL | | (false, "hello")
LL | | }
| |_____^ help: try: `string.map_or((false, "hello"), |x| (true, x))`
|
= note: `-D clippy::option-if-let-else` implied by `-D warnings`

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:15:12
|
LL | } else if let Some(x) = string {
| ____________^
LL | | Some((true, x))
LL | | } else {
LL | | Some((false, ""))
LL | | }
| |_____^ help: try: `{ string.map_or(Some((false, "")), |x| Some((true, x))) }`

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:23:13
|
LL | let _ = if let Some(s) = *string { s.len() } else { 0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())`

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:24:13
|
LL | let _ = if let Some(s) = &num { s } else { &0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:25:13
|
LL | let _ = if let Some(s) = &mut num {
| _____________^
LL | | *s += 1;
LL | | s
LL | | } else {
LL | | &mut 0
LL | | };
| |_____^
|
help: try
|
LL | let _ = num.as_mut().map_or(&mut 0, |s| {
LL | *s += 1;
LL | s
LL | });
|

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:31:13
|
LL | let _ = if let Some(ref s) = num { s } else { &0 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)`

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:32:13
|
LL | let _ = if let Some(mut s) = num {
| _____________^
LL | | s += 1;
LL | | s
LL | | } else {
LL | | 0
LL | | };
| |_____^
|
help: try
|
LL | let _ = num.map_or(0, |mut s| {
LL | s += 1;
LL | s
LL | });
|

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:38:13
|
LL | let _ = if let Some(ref mut s) = num {
| _____________^
LL | | *s += 1;
LL | | s
LL | | } else {
LL | | &mut 0
LL | | };
| |_____^
|
help: try
|
LL | let _ = num.as_mut().map_or(&mut 0, |s| {
LL | *s += 1;
LL | s
LL | });
|

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:47:5
|
LL | / if let Some(x) = arg {
LL | | let y = x * x;
LL | | y * y
LL | | } else {
LL | | 13
LL | | }
| |_____^
|
help: try
|
LL | arg.map_or(13, |x| {
LL | let y = x * x;
LL | y * y
LL | })
|

error: use Option::map_or_else instead of an if let/else
--> $DIR/option_if_let_else.rs:56:13
|
LL | let _ = if let Some(x) = arg {
| _____________^
LL | | x * x * x * x
LL | | } else {
LL | | let mut y = 1;
... |
LL | | y
LL | | };
| |_____^
|
help: try
|
LL | let _ = arg.map_or_else(|| {
LL | let mut y = 1;
LL | y = (y + 2 / y) / 2;
LL | y = (y + 2 / y) / 2;
LL | y
LL | }, |x| x * x * x * x);
|

error: use Option::map_or instead of an if let/else
--> $DIR/option_if_let_else.rs:85:13
|
LL | let _ = if let Some(x) = optional { x + 2 } else { 5 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)`

error: aborting due to 11 previous errors