Skip to content
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.

Sign up for GitHub

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

option_if_let_else - distinguish pure from impure else expressions #5937

Merged
merged 2 commits into from
Sep 16, 2020
Merged

option_if_let_else - distinguish pure from impure else expressions #5937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ba36bc
option_if_let_else - distinguish pure from impure else expressions
tnielens Aug 21, 2020
79da747
option_if_let_else - change misleading test file section
tnielens Sep 16, 2020
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
21 changes: 14 additions & 7 deletions clippy_lints/src/methods/bind_instead_of_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use rustc_middle::hir::map::Map;
use rustc_span::Span;

pub(crate) struct OptionAndThenSome;

impl BindInsteadOfMap for OptionAndThenSome {
const TYPE_NAME: &'static str = "Option";
const TYPE_QPATH: &'static [&'static str] = &paths::OPTION;
Expand All @@ -24,6 +25,7 @@ impl BindInsteadOfMap for OptionAndThenSome {
}

pub(crate) struct ResultAndThenOk;

impl BindInsteadOfMap for ResultAndThenOk {
const TYPE_NAME: &'static str = "Result";
const TYPE_QPATH: &'static [&'static str] = &paths::RESULT;
Expand All @@ -36,6 +38,7 @@ impl BindInsteadOfMap for ResultAndThenOk {
}

pub(crate) struct ResultOrElseErrInfo;

impl BindInsteadOfMap for ResultOrElseErrInfo {
const TYPE_NAME: &'static str = "Result";
const TYPE_QPATH: &'static [&'static str] = &paths::RESULT;
Expand Down Expand Up @@ -120,9 +123,9 @@ pub(crate) trait BindInsteadOfMap {
}
}

fn lint_closure(cx: &LateContext<'_>, expr: &hir::Expr<'_>, closure_expr: &hir::Expr<'_>) {
fn lint_closure(cx: &LateContext<'_>, expr: &hir::Expr<'_>, closure_expr: &hir::Expr<'_>) -> bool {
let mut suggs = Vec::new();
let can_sugg = find_all_ret_expressions(cx, closure_expr, |ret_expr| {
let can_sugg: bool = find_all_ret_expressions(cx, closure_expr, |ret_expr| {
if_chain! {
if !in_macro(ret_expr.span);
if let hir::ExprKind::Call(ref func_path, ref args) = ret_expr.kind;
Expand Down Expand Up @@ -153,21 +156,24 @@ pub(crate) trait BindInsteadOfMap {
)
});
}
can_sugg
}

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

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

if !Self::lint_closure_autofixable(cx, expr, args, closure_expr, closure_args_span) {
Self::lint_closure(cx, expr, closure_expr);
if Self::lint_closure_autofixable(cx, expr, args, closure_expr, closure_args_span) {
true
} else {
Self::lint_closure(cx, expr, closure_expr)
}
},
// `_.and_then(Some)` case, which is no-op.
Expand All @@ -181,8 +187,9 @@ pub(crate) trait BindInsteadOfMap {
snippet(cx, args[0].span, "..").into(),
Applicability::MachineApplicable,
);
true
},
_ => {},
_ => false,
}
}
}
Expand Down
66 changes: 19 additions & 47 deletions clippy_lints/src/methods/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,15 @@ use rustc_span::source_map::Span;
use rustc_span::symbol::{sym, SymbolStr};

use crate::consts::{constant, Constant};
use crate::utils::eager_or_lazy::is_lazyness_candidate;
use crate::utils::usage::mutated_variables;
use crate::utils::{
contains_ty, get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro,
is_copy, is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats,
last_path_segment, match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls,
method_chain_args, paths, remove_blocks, return_ty, single_segment_path, snippet, snippet_with_applicability,
snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_note, span_lint_and_sugg,
span_lint_and_then, sugg, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
is_copy, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment, match_def_path, match_qpath,
match_trait_method, match_type, match_var, method_calls, method_chain_args, paths, remove_blocks, return_ty,
single_segment_path, snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint,
span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, sugg, walk_ptrs_ty,
walk_ptrs_ty_depth, SpanlessEq,
};

declare_clippy_lint! {
Expand Down Expand Up @@ -1454,18 +1455,21 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0], method_spans[1]),
["unwrap_or_else", "map"] => {
if !lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]) {
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], true, "unwrap_or");
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "unwrap_or");
}
},
["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]),
["and_then", ..] => {
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], false, "and");
bind_instead_of_map::OptionAndThenSome::lint(cx, expr, arg_lists[0]);
bind_instead_of_map::ResultAndThenOk::lint(cx, expr, arg_lists[0]);
let biom_option_linted = bind_instead_of_map::OptionAndThenSome::lint(cx, expr, arg_lists[0]);
let biom_result_linted = bind_instead_of_map::ResultAndThenOk::lint(cx, expr, arg_lists[0]);
if !biom_option_linted && !biom_result_linted {
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "and");
}
},
["or_else", ..] => {
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], false, "or");
bind_instead_of_map::ResultOrElseErrInfo::lint(cx, expr, arg_lists[0]);
if !bind_instead_of_map::ResultOrElseErrInfo::lint(cx, expr, arg_lists[0]) {
unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "or");
}
},
["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
["next", "skip_while"] => lint_skip_while_next(cx, expr, arg_lists[1]),
Expand Down Expand Up @@ -1508,9 +1512,9 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
["is_file", ..] => lint_filetype_is_file(cx, expr, arg_lists[0]),
["map", "as_ref"] => lint_option_as_ref_deref(cx, expr, arg_lists[1], arg_lists[0], false),
["map", "as_mut"] => lint_option_as_ref_deref(cx, expr, arg_lists[1], arg_lists[0], true),
["unwrap_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], true, "unwrap_or"),
["get_or_insert_with", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], true, "get_or_insert"),
["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], true, "ok_or"),
["unwrap_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "unwrap_or"),
["get_or_insert_with", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "get_or_insert"),
["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "ok_or"),
_ => {},
}

Expand Down Expand Up @@ -1714,37 +1718,6 @@ fn lint_or_fun_call<'tcx>(
name: &str,
args: &'tcx [hir::Expr<'_>],
) {
// Searches an expression for method calls or function calls that aren't ctors
struct FunCallFinder<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
found: bool,
}

impl<'a, 'tcx> intravisit::Visitor<'tcx> for FunCallFinder<'a, 'tcx> {
type Map = Map<'tcx>;

fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) {
let call_found = match &expr.kind {
// ignore enum and struct constructors
hir::ExprKind::Call(..) => !is_ctor_or_promotable_const_function(self.cx, expr),
hir::ExprKind::MethodCall(..) => true,
_ => false,
};

if call_found {
self.found |= true;
}

if !self.found {
intravisit::walk_expr(self, expr);
}
}

fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> {
intravisit::NestedVisitorMap::None
}
}

/// Checks for `unwrap_or(T::new())` or `unwrap_or(T::default())`.
fn check_unwrap_or_default(
cx: &LateContext<'_>,
Expand Down Expand Up @@ -1825,8 +1798,7 @@ fn lint_or_fun_call<'tcx>(
if_chain! {
if know_types.iter().any(|k| k.2.contains(&name));

let mut finder = FunCallFinder { cx: &cx, found: false };
if { finder.visit_expr(&arg); finder.found };
if is_lazyness_candidate(cx, arg);
if !contains_return(&arg);

let self_ty = cx.typeck_results().expr_ty(self_expr);
Expand Down
76 changes: 9 additions & 67 deletions clippy_lints/src/methods/unnecessary_lazy_eval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,78 +1,17 @@
use crate::utils::{is_type_diagnostic_item, match_qpath, snippet, span_lint_and_sugg};
use if_chain::if_chain;
use crate::utils::{eager_or_lazy, usage};
use crate::utils::{is_type_diagnostic_item, snippet, span_lint_and_sugg};
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::LateContext;

use super::UNNECESSARY_LAZY_EVALUATIONS;

// Return true if the expression is an accessor of any of the arguments
fn expr_uses_argument(expr: &hir::Expr<'_>, params: &[hir::Param<'_>]) -> bool {
params.iter().any(|arg| {
if_chain! {
if let hir::PatKind::Binding(_, _, ident, _) = arg.pat.kind;
if let hir::ExprKind::Path(hir::QPath::Resolved(_, ref path)) = expr.kind;
if let [p, ..] = path.segments;
then {
ident.name == p.ident.name
} else {
false
}
}
})
}

fn match_any_qpath(path: &hir::QPath<'_>, paths: &[&[&str]]) -> bool {
paths.iter().any(|candidate| match_qpath(path, candidate))
}

fn can_simplify(expr: &hir::Expr<'_>, params: &[hir::Param<'_>], variant_calls: bool) -> bool {
match expr.kind {
// Closures returning literals can be unconditionally simplified
hir::ExprKind::Lit(_) => true,

hir::ExprKind::Index(ref object, ref index) => {
// arguments are not being indexed into
if expr_uses_argument(object, params) {
false
} else {
// arguments are not used as index
!expr_uses_argument(index, params)
}
},

// Reading fields can be simplified if the object is not an argument of the closure
hir::ExprKind::Field(ref object, _) => !expr_uses_argument(object, params),

// Paths can be simplified if the root is not the argument, this also covers None
hir::ExprKind::Path(_) => !expr_uses_argument(expr, params),

// Calls to Some, Ok, Err can be considered literals if they don't derive an argument
hir::ExprKind::Call(ref func, ref args) => if_chain! {
if variant_calls; // Disable lint when rules conflict with bind_instead_of_map
if let hir::ExprKind::Path(ref path) = func.kind;
if match_any_qpath(path, &[&["Some"], &["Ok"], &["Err"]]);
then {
// Recursively check all arguments
args.iter().all(|arg| can_simplify(arg, params, variant_calls))
} else {
false
}
},

// For anything more complex than the above, a closure is probably the right solution,
// or the case is handled by an other lint
_ => false,
}
}

/// lint use of `<fn>_else(simple closure)` for `Option`s and `Result`s that can be
/// replaced with `<fn>(return value of simple closure)`
pub(super) fn lint<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx hir::Expr<'_>,
args: &'tcx [hir::Expr<'_>],
allow_variant_calls: bool,
simplify_using: &str,
) {
let is_option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(&args[0]), sym!(option_type));
Expand All @@ -81,10 +20,13 @@ pub(super) fn lint<'tcx>(
if is_option || is_result {
if let hir::ExprKind::Closure(_, _, eid, _, _) = args[1].kind {
let body = cx.tcx.hir().body(eid);
let ex = &body.value;
let params = &body.params;
let body_expr = &body.value;

if usage::BindingUsageFinder::are_params_used(cx, body) {
return;
}

if can_simplify(ex, params, allow_variant_calls) {
if eager_or_lazy::is_eagerness_candidate(cx, body_expr) {
let msg = if is_option {
"unnecessary closure used to substitute value for `Option::None`"
} else {
Expand All @@ -101,7 +43,7 @@ pub(super) fn lint<'tcx>(
"{0}.{1}({2})",
snippet(cx, args[0].span, ".."),
simplify_using,
snippet(cx, ex.span, ".."),
snippet(cx, body_expr.span, ".."),
),
Applicability::MachineApplicable,
);
Expand Down
33 changes: 15 additions & 18 deletions clippy_lints/src/option_if_let_else.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::utils;
use crate::utils::eager_or_lazy;
use crate::utils::sugg::Sugg;
use crate::utils::{match_type, paths, span_lint_and_sugg};
use if_chain::if_chain;
Expand All @@ -13,22 +14,16 @@ 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).
/// idiomatically done with `Option::map_or` (if the else bit is a pure
/// expression) or `Option::map_or_else` (if the else bit is an impure
/// expresion).
///
/// **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
/// 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.
Expand Down Expand Up @@ -92,13 +87,15 @@ struct OptionIfLetElseOccurence {
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> {
Expand Down Expand Up @@ -157,7 +154,7 @@ fn extract_body_from_arm<'a>(arm: &'a Arm<'a>) -> Option<&'a Expr<'a>> {
}

/// If this is the else body of an if/else expression, then we need to wrap
/// it in curcly braces. Otherwise, we don't.
/// it in curly 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 {
Expand Down Expand Up @@ -199,7 +196,10 @@ fn format_option_in_sugg(cx: &LateContext<'_>, cond_expr: &Expr<'_>, as_ref: boo
/// 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> {
fn detect_option_if_let_else<'tcx>(
cx: &'_ LateContext<'tcx>,
expr: &'_ Expr<'tcx>,
) -> 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;
Expand All @@ -214,10 +214,7 @@ fn detect_option_if_let_else(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<Op
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 method_sugg = if eager_or_lazy::is_eagerness_candidate(cx, none_body) { "map_or" } else { "map_or_else" };
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 {
Expand All @@ -243,8 +240,8 @@ fn detect_option_if_let_else(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<Op
}
}

impl<'a> LateLintPass<'a> for OptionIfLetElse {
fn check_expr(&mut self, cx: &LateContext<'a>, expr: &Expr<'_>) {
impl<'tcx> LateLintPass<'tcx> for OptionIfLetElse {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
if let Some(detection) = detect_option_if_let_else(cx, expr) {
span_lint_and_sugg(
cx,
Expand Down
Loading