user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Auto merge of #5937 - montrivo:option_if_let_else, r=flip1995

Browse source 
option_if_let_else - distinguish pure from impure else expressions

Addresses partially #5821.

changelog: improve the lint `option_if_let_else`. Suggest `map_or` or `map_or_else` based on the else expression purity.
This commit is contained in:
bors 2020-09-16 19:36:49 +00:00
commit 06f1902878
14 changed files with 421 additions and 213 deletions
Download patch file Download diff file Expand all files Collapse all files

21
clippy_lints/src/methods/bind_instead_of_map.rs
View file

@ -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;
@ -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;
@ -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;
@ -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;
@ -153,12 +156,13 @@ 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 {
@ -166,8 +170,10 @@ pub(crate) trait BindInsteadOfMap {
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.
@ -181,8 +187,9 @@ pub(crate) trait BindInsteadOfMap {
snippet(cx, args[0].span, "..").into(),
Applicability::MachineApplicable,
);
true
},
_ => {},
_ => false,
}
}
}

66
clippy_lints/src/methods/mod.rs
View file

@ -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! {
@ -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]),
@ -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"),
_ => {},
}
@ -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<'_>,
@ -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);

76
clippy_lints/src/methods/unnecessary_lazy_eval.rs
View file

@ -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));
@ -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 can_simplify(ex, params, allow_variant_calls) {
if usage::BindingUsageFinder::are_params_used(cx, body) {
return;
}
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 {
@ -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,
);