From 2938ffd0d94d93893ca32202cb3b6a6b69559bfb Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Tue, 17 Aug 2021 16:22:48 -0400 Subject: [PATCH] Improve heuristics for determining whether eager of lazy evaluation is preferred --- clippy_lints/src/functions/too_many_lines.rs | 4 +- .../methods/from_iter_instead_of_collect.rs | 2 +- clippy_lints/src/methods/or_fun_call.rs | 59 ++-- .../src/methods/unnecessary_lazy_eval.rs | 2 +- clippy_lints/src/option_if_let_else.rs | 6 +- clippy_utils/src/eager_or_lazy.rs | 313 ++++++++++++------ clippy_utils/src/paths.rs | 1 + clippy_utils/src/ty.rs | 21 +- clippy_utils/src/visitors.rs | 94 +++++- tests/ui/needless_return.fixed | 1 + tests/ui/needless_return.rs | 1 + tests/ui/needless_return.stderr | 36 +- tests/ui/option_if_let_else.fixed | 2 +- tests/ui/option_if_let_else.stderr | 4 +- tests/ui/or_fun_call.fixed | 30 +- tests/ui/or_fun_call.rs | 18 +- tests/ui/or_fun_call.stderr | 70 +--- 17 files changed, 416 insertions(+), 248 deletions(-) diff --git a/clippy_lints/src/functions/too_many_lines.rs b/clippy_lints/src/functions/too_many_lines.rs index 008ef661b..65efbbab4 100644 --- a/clippy_lints/src/functions/too_many_lines.rs +++ b/clippy_lints/src/functions/too_many_lines.rs @@ -56,8 +56,8 @@ pub(super) fn check_fn( continue; } } else { - let multi_idx = line.find("/*").unwrap_or_else(|| line.len()); - let single_idx = line.find("//").unwrap_or_else(|| line.len()); + let multi_idx = line.find("/*").unwrap_or(line.len()); + let single_idx = line.find("//").unwrap_or(line.len()); code_in_line |= multi_idx > 0 && single_idx > 0; // Implies multi_idx is below line.len() if multi_idx < single_idx { diff --git a/clippy_lints/src/methods/from_iter_instead_of_collect.rs b/clippy_lints/src/methods/from_iter_instead_of_collect.rs index 99c03844f..8ea9312c0 100644 --- a/clippy_lints/src/methods/from_iter_instead_of_collect.rs +++ b/clippy_lints/src/methods/from_iter_instead_of_collect.rs @@ -69,7 +69,7 @@ fn extract_turbofish(cx: &LateContext<'_>, expr: &hir::Expr<'_>, ty: Ty<'tcx>) - // i.e.: 2 wildcards in `std::collections::BTreeMap<&i32, &char>` let ty_str = ty.to_string(); let start = ty_str.find('<').unwrap_or(0); - let end = ty_str.find('>').unwrap_or_else(|| ty_str.len()); + let end = ty_str.find('>').unwrap_or(ty_str.len()); let nb_wildcard = ty_str[start..end].split(',').count(); let wildcards = format!("_{}", ", _".repeat(nb_wildcard - 1)); format!("{}<{}>", elements.join("::"), wildcards) diff --git a/clippy_lints/src/methods/or_fun_call.rs b/clippy_lints/src/methods/or_fun_call.rs index fe9ffde0d..4e4653dad 100644 --- a/clippy_lints/src/methods/or_fun_call.rs +++ b/clippy_lints/src/methods/or_fun_call.rs @@ -1,16 +1,12 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::eager_or_lazy::is_lazyness_candidate; -use clippy_utils::is_trait_item; +use clippy_utils::eager_or_lazy::switch_to_lazy_eval; use clippy_utils::source::{snippet, snippet_with_applicability, snippet_with_macro_callsite}; -use clippy_utils::ty::implements_trait; -use clippy_utils::ty::{is_type_diagnostic_item, match_type}; -use clippy_utils::{contains_return, last_path_segment, paths}; +use clippy_utils::ty::{implements_trait, match_type}; +use clippy_utils::{contains_return, is_trait_item, last_path_segment, paths}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::{BlockCheckMode, UnsafeSource}; use rustc_lint::LateContext; -use rustc_middle::ty; use rustc_span::source_map::Span; use rustc_span::symbol::{kw, sym}; use std::borrow::Cow; @@ -96,25 +92,10 @@ pub(super) fn check<'tcx>( (&paths::RESULT, true, &["or", "unwrap_or"], "else"), ]; - if let hir::ExprKind::MethodCall(path, _, [self_arg, ..], _) = &arg.kind { - if path.ident.name == sym::len { - let ty = cx.typeck_results().expr_ty(self_arg).peel_refs(); - - match ty.kind() { - ty::Slice(_) | ty::Array(_, _) | ty::Str => return, - _ => (), - } - - if is_type_diagnostic_item(cx, ty, sym::Vec) { - return; - } - } - } - if_chain! { if KNOW_TYPES.iter().any(|k| k.2.contains(&name)); - if is_lazyness_candidate(cx, arg); + if switch_to_lazy_eval(cx, arg); if !contains_return(arg); let self_ty = cx.typeck_results().expr_ty(self_expr); @@ -166,26 +147,30 @@ pub(super) fn check<'tcx>( } } - if args.len() == 2 { - match args[1].kind { + if let [self_arg, arg] = args { + let inner_arg = if let hir::ExprKind::Block( + hir::Block { + stmts: [], + expr: Some(expr), + .. + }, + _, + ) = arg.kind + { + expr + } else { + arg + }; + match inner_arg.kind { hir::ExprKind::Call(fun, or_args) => { let or_has_args = !or_args.is_empty(); - if !check_unwrap_or_default(cx, name, fun, &args[0], &args[1], or_has_args, expr.span) { + if !check_unwrap_or_default(cx, name, fun, self_arg, arg, or_has_args, expr.span) { let fun_span = if or_has_args { None } else { Some(fun.span) }; - check_general_case(cx, name, method_span, &args[0], &args[1], expr.span, fun_span); + check_general_case(cx, name, method_span, self_arg, arg, expr.span, fun_span); } }, hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => { - check_general_case(cx, name, method_span, &args[0], &args[1], expr.span, None); - }, - hir::ExprKind::Block(block, _) - if block.rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) => - { - if let Some(block_expr) = block.expr { - if let hir::ExprKind::MethodCall(..) = block_expr.kind { - check_general_case(cx, name, method_span, &args[0], &args[1], expr.span, None); - } - } + check_general_case(cx, name, method_span, self_arg, arg, expr.span, None); }, _ => (), } diff --git a/clippy_lints/src/methods/unnecessary_lazy_eval.rs b/clippy_lints/src/methods/unnecessary_lazy_eval.rs index 740af750b..1e2765263 100644 --- a/clippy_lints/src/methods/unnecessary_lazy_eval.rs +++ b/clippy_lints/src/methods/unnecessary_lazy_eval.rs @@ -30,7 +30,7 @@ pub(super) fn check<'tcx>( return; } - if eager_or_lazy::is_eagerness_candidate(cx, body_expr) { + if eager_or_lazy::switch_to_eager_eval(cx, body_expr) { let msg = if is_option { "unnecessary closure used to substitute value for `Option::None`" } else { diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index f7c19baf7..262be17f6 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -147,11 +147,7 @@ fn detect_option_if_let_else<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> let capture_mut = if bind_annotation == &BindingAnnotation::Mutable { "mut " } else { "" }; let some_body = extract_body_from_expr(if_then)?; let none_body = extract_body_from_expr(if_else)?; - let method_sugg = if eager_or_lazy::is_eagerness_candidate(cx, none_body) { - "map_or" - } else { - "map_or_else" - }; + let method_sugg = if eager_or_lazy::switch_to_eager_eval(cx, none_body) { "map_or" } else { "map_or_else" }; let capture_name = id.name.to_ident_string(); let (as_ref, as_mut) = match &let_expr.kind { ExprKind::AddrOf(_, Mutability::Not, _) => (true, false), diff --git a/clippy_utils/src/eager_or_lazy.rs b/clippy_utils/src/eager_or_lazy.rs index 1ea7ccfb7..c2645ac73 100644 --- a/clippy_utils/src/eager_or_lazy.rs +++ b/clippy_utils/src/eager_or_lazy.rs @@ -9,128 +9,227 @@ //! - or-fun-call //! - option-if-let-else -use crate::is_ctor_or_promotable_const_function; -use crate::ty::is_type_diagnostic_item; +use crate::ty::{all_predicates_of, is_copy}; +use crate::visitors::is_const_evaluatable; use rustc_hir::def::{DefKind, Res}; - -use rustc_hir::intravisit; -use rustc_hir::intravisit::{NestedVisitorMap, Visitor}; - -use rustc_hir::{Block, Expr, ExprKind, Path, QPath}; +use rustc_hir::intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor}; +use rustc_hir::{def_id::DefId, Block, Expr, ExprKind, QPath, UnOp}; use rustc_lint::LateContext; -use rustc_middle::hir::map::Map; -use rustc_span::sym; +use rustc_middle::ty::{self, PredicateKind}; +use rustc_span::{sym, Symbol}; +use std::cmp; +use std::ops; -/// Is the expr pure (is it free from side-effects)? -/// This function is named so to stress that it isn't exhaustive and returns FNs. -fn identify_some_pure_patterns(expr: &Expr<'_>) -> bool { - match expr.kind { - ExprKind::Lit(..) | ExprKind::ConstBlock(..) | ExprKind::Path(..) | ExprKind::Field(..) => true, - ExprKind::AddrOf(_, _, addr_of_expr) => identify_some_pure_patterns(addr_of_expr), - ExprKind::Tup(tup_exprs) => tup_exprs.iter().all(identify_some_pure_patterns), - ExprKind::Struct(_, fields, expr) => { - fields.iter().all(|f| identify_some_pure_patterns(f.expr)) && expr.map_or(true, identify_some_pure_patterns) - }, - ExprKind::Call( - &Expr { - kind: - ExprKind::Path(QPath::Resolved( - _, - Path { - res: Res::Def(DefKind::Ctor(..) | DefKind::Variant, ..), - .. - }, - )), - .. - }, - args, - ) => args.iter().all(identify_some_pure_patterns), - ExprKind::Block( - &Block { - stmts, - expr: Some(expr), - .. - }, - _, - ) => stmts.is_empty() && identify_some_pure_patterns(expr), - ExprKind::Box(..) - | ExprKind::Array(..) - | ExprKind::Call(..) - | ExprKind::MethodCall(..) - | ExprKind::Binary(..) - | ExprKind::Unary(..) - | ExprKind::Let(..) - | ExprKind::Cast(..) - | ExprKind::Type(..) - | ExprKind::DropTemps(..) - | ExprKind::Loop(..) - | ExprKind::If(..) - | ExprKind::Match(..) - | ExprKind::Closure(..) - | ExprKind::Block(..) - | ExprKind::Assign(..) - | ExprKind::AssignOp(..) - | ExprKind::Index(..) - | ExprKind::Break(..) - | ExprKind::Continue(..) - | ExprKind::Ret(..) - | ExprKind::InlineAsm(..) - | ExprKind::LlvmInlineAsm(..) - | ExprKind::Repeat(..) - | ExprKind::Yield(..) - | ExprKind::Err => false, +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +enum EagernessSuggestion { + // The expression is cheap and should be evaluated eagerly + Eager, + // The expression may be cheap, so don't suggested lazy evaluation; or the expression may not be safe to switch to + // eager evaluation. + NoChange, + // The expression is likely expensive and should be evaluated lazily. + Lazy, + // The expression cannot be placed into a closure. + ForceNoChange, +} +impl ops::BitOr for EagernessSuggestion { + type Output = Self; + fn bitor(self, rhs: Self) -> Self { + cmp::max(self, rhs) + } +} +impl ops::BitOrAssign for EagernessSuggestion { + fn bitor_assign(&mut self, rhs: Self) { + *self = *self | rhs; } } -/// Identify some potentially computationally expensive patterns. -/// This function is named so to stress that its implementation is non-exhaustive. -/// It returns FNs and FPs. -fn identify_some_potentially_expensive_patterns<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { - // Searches an expression for method calls or function calls that aren't ctors - struct FunCallFinder<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - found: bool, +/// Determine the eagerness of the given function call. +fn fn_eagerness(cx: &LateContext<'tcx>, fn_id: DefId, name: Symbol, args: &'tcx [Expr<'_>]) -> EagernessSuggestion { + use EagernessSuggestion::{Eager, Lazy, NoChange}; + let name = &*name.as_str(); + + let ty = match cx.tcx.impl_of_method(fn_id) { + Some(id) => cx.tcx.type_of(id), + None => return Lazy, + }; + + if (name.starts_with("as_") || name == "len" || name == "is_empty") && args.len() == 1 { + if matches!( + cx.tcx.crate_name(fn_id.krate), + sym::std | sym::core | sym::alloc | sym::proc_macro + ) { + Eager + } else { + NoChange + } + } else if let ty::Adt(def, subs) = ty.kind() { + // Types where the only fields are generic types (or references to) with no trait bounds other + // than marker traits. + // Due to the limited operations on these types functions should be fairly cheap. + if def + .variants + .iter() + .flat_map(|v| v.fields.iter()) + .any(|x| matches!(cx.tcx.type_of(x.did).peel_refs().kind(), ty::Param(_))) + && all_predicates_of(cx.tcx, fn_id).all(|(pred, _)| match pred.kind().skip_binder() { + PredicateKind::Trait(pred) => cx.tcx.trait_def(pred.trait_ref.def_id).is_marker, + _ => true, + }) + && subs.types().all(|x| matches!(x.peel_refs().kind(), ty::Param(_))) + { + // Limit the function to either `(self) -> bool` or `(&self) -> bool` + match &**cx.tcx.fn_sig(fn_id).skip_binder().inputs_and_output { + [arg, res] if !arg.is_mutable_ptr() && arg.peel_refs() == ty && res.is_bool() => NoChange, + _ => Lazy, + } + } else { + Lazy + } + } else { + Lazy + } +} + +#[allow(clippy::too_many_lines)] +fn expr_eagerness(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessSuggestion { + struct V<'cx, 'tcx> { + cx: &'cx LateContext<'tcx>, + eagerness: EagernessSuggestion, } - impl<'a, 'tcx> intravisit::Visitor<'tcx> for FunCallFinder<'a, 'tcx> { - type Map = Map<'tcx>; - - fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { - let call_found = match &expr.kind { - // ignore enum and struct constructors - ExprKind::Call(..) => !is_ctor_or_promotable_const_function(self.cx, expr), - ExprKind::Index(obj, _) => { - let ty = self.cx.typeck_results().expr_ty(obj); - is_type_diagnostic_item(self.cx, ty, sym::HashMap) - || is_type_diagnostic_item(self.cx, ty, sym::BTreeMap) - }, - ExprKind::MethodCall(..) => true, - _ => false, - }; - - if call_found { - self.found |= true; - } - - if !self.found { - intravisit::walk_expr(self, expr); - } - } - + impl<'cx, 'tcx> Visitor<'tcx> for V<'cx, 'tcx> { + type Map = ErasedMap<'tcx>; fn nested_visit_map(&mut self) -> NestedVisitorMap { NestedVisitorMap::None } + + fn visit_expr(&mut self, e: &'tcx Expr<'_>) { + use EagernessSuggestion::{ForceNoChange, Lazy, NoChange}; + if self.eagerness == ForceNoChange { + return; + } + match e.kind { + ExprKind::Call( + &Expr { + kind: ExprKind::Path(ref path), + hir_id, + .. + }, + args, + ) => match self.cx.qpath_res(path, hir_id) { + Res::Def(DefKind::Ctor(..) | DefKind::Variant, _) | Res::SelfCtor(_) => (), + Res::Def(_, id) if self.cx.tcx.is_promotable_const_fn(id) => (), + // No need to walk the arguments here, `is_const_evaluatable` already did + Res::Def(..) if is_const_evaluatable(self.cx, e) => { + self.eagerness |= NoChange; + return; + }, + Res::Def(_, id) => match path { + QPath::Resolved(_, p) => { + self.eagerness |= fn_eagerness(self.cx, id, p.segments.last().unwrap().ident.name, args); + }, + QPath::TypeRelative(_, name) => { + self.eagerness |= fn_eagerness(self.cx, id, name.ident.name, args); + }, + QPath::LangItem(..) => self.eagerness = Lazy, + }, + _ => self.eagerness = Lazy, + }, + // No need to walk the arguments here, `is_const_evaluatable` already did + ExprKind::MethodCall(..) if is_const_evaluatable(self.cx, e) => { + self.eagerness |= NoChange; + return; + }, + ExprKind::MethodCall(name, _, args, _) => { + self.eagerness |= self + .cx + .typeck_results() + .type_dependent_def_id(e.hir_id) + .map_or(Lazy, |id| fn_eagerness(self.cx, id, name.ident.name, args)); + }, + ExprKind::Index(_, e) => { + let ty = self.cx.typeck_results().expr_ty_adjusted(e); + if is_copy(self.cx, ty) && !ty.is_ref() { + self.eagerness |= NoChange; + } else { + self.eagerness = Lazy; + } + }, + + // Dereferences should be cheap, but dereferencing a raw pointer earlier may not be safe. + ExprKind::Unary(UnOp::Deref, e) if !self.cx.typeck_results().expr_ty(e).is_unsafe_ptr() => (), + ExprKind::Unary(UnOp::Deref, _) => self.eagerness |= NoChange, + + ExprKind::Unary(_, e) + if matches!( + self.cx.typeck_results().expr_ty(e).kind(), + ty::Bool | ty::Int(_) | ty::Uint(_), + ) => {}, + ExprKind::Binary(_, lhs, rhs) + if self.cx.typeck_results().expr_ty(lhs).is_primitive() + && self.cx.typeck_results().expr_ty(rhs).is_primitive() => {}, + + // Can't be moved into a closure + ExprKind::Break(..) + | ExprKind::Continue(_) + | ExprKind::Ret(_) + | ExprKind::InlineAsm(_) + | ExprKind::LlvmInlineAsm(_) + | ExprKind::Yield(..) + | ExprKind::Err => { + self.eagerness = ForceNoChange; + return; + }, + + // Memory allocation, custom operator, loop, or call to an unknown function + ExprKind::Box(_) + | ExprKind::Unary(..) + | ExprKind::Binary(..) + | ExprKind::Loop(..) + | ExprKind::Call(..) => self.eagerness = Lazy, + + ExprKind::ConstBlock(_) + | ExprKind::Array(_) + | ExprKind::Tup(_) + | ExprKind::Lit(_) + | ExprKind::Cast(..) + | ExprKind::Type(..) + | ExprKind::DropTemps(_) + | ExprKind::Let(..) + | ExprKind::If(..) + | ExprKind::Match(..) + | ExprKind::Closure(..) + | ExprKind::Field(..) + | ExprKind::Path(_) + | ExprKind::AddrOf(..) + | ExprKind::Struct(..) + | ExprKind::Repeat(..) + | ExprKind::Block(Block { stmts: [], .. }, _) => (), + + // Assignment might be to a local defined earlier, so don't eagerly evaluate. + // Blocks with multiple statements might be expensive, so don't eagerly evaluate. + // TODO: Actually check if either of these are true here. + ExprKind::Assign(..) | ExprKind::AssignOp(..) | ExprKind::Block(..) => self.eagerness |= NoChange, + } + walk_expr(self, e); + } } - let mut finder = FunCallFinder { cx, found: false }; - finder.visit_expr(expr); - finder.found + let mut v = V { + cx, + eagerness: EagernessSuggestion::Eager, + }; + v.visit_expr(e); + v.eagerness } -pub fn is_eagerness_candidate<'a, 'tcx>(cx: &'a LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { - !identify_some_potentially_expensive_patterns(cx, expr) && identify_some_pure_patterns(expr) +/// Whether the given expression should be changed to evaluate eagerly +pub fn switch_to_eager_eval(cx: &'_ LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { + expr_eagerness(cx, expr) == EagernessSuggestion::Eager } -pub fn is_lazyness_candidate<'a, 'tcx>(cx: &'a LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { - identify_some_potentially_expensive_patterns(cx, expr) +/// Whether the given expression should be changed to evaluate lazily +pub fn switch_to_lazy_eval(cx: &'_ LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool { + expr_eagerness(cx, expr) == EagernessSuggestion::Lazy } diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 480923bbb..b04d17364 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -28,6 +28,7 @@ pub const ASREF_TRAIT: [&str; 3] = ["core", "convert", "AsRef"]; pub(super) const BEGIN_PANIC: [&str; 3] = ["std", "panicking", "begin_panic"]; /// Preferably use the diagnostic item `sym::Borrow` where possible pub const BORROW_TRAIT: [&str; 3] = ["core", "borrow", "Borrow"]; +pub const BORROW_MUT_TRAIT: [&str; 3] = ["core", "borrow", "BorrowMut"]; pub const BTREEMAP_CONTAINS_KEY: [&str; 6] = ["alloc", "collections", "btree", "map", "BTreeMap", "contains_key"]; pub const BTREEMAP_ENTRY: [&str; 6] = ["alloc", "collections", "btree", "map", "entry", "Entry"]; pub const BTREEMAP_INSERT: [&str; 6] = ["alloc", "collections", "btree", "map", "BTreeMap", "insert"]; diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index babab07ea..438c39bea 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -10,12 +10,12 @@ use rustc_hir::{TyKind, Unsafety}; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::ty::subst::{GenericArg, GenericArgKind}; -use rustc_middle::ty::{self, AdtDef, IntTy, Ty, TyCtxt, TypeFoldable, UintTy}; -use rustc_span::sym; -use rustc_span::symbol::{Ident, Symbol}; -use rustc_span::DUMMY_SP; +use rustc_middle::ty::{self, AdtDef, IntTy, Predicate, Ty, TyCtxt, TypeFoldable, UintTy}; +use rustc_span::symbol::Ident; +use rustc_span::{sym, Span, Symbol, DUMMY_SP}; use rustc_trait_selection::infer::InferCtxtExt; use rustc_trait_selection::traits::query::normalize::AtExt; +use std::iter; use crate::{match_def_path, must_use_attr}; @@ -391,3 +391,16 @@ pub fn is_uninit_value_valid_for_ty(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { _ => false, } } + +/// Gets an iterator over all predicates which apply to the given item. +pub fn all_predicates_of(tcx: TyCtxt<'_>, id: DefId) -> impl Iterator, Span)> { + let mut next_id = Some(id); + iter::from_fn(move || { + next_id.take().map(|id| { + let preds = tcx.predicates_of(id); + next_id = preds.parent; + preds.predicates.iter() + }) + }) + .flatten() +} diff --git a/clippy_utils/src/visitors.rs b/clippy_utils/src/visitors.rs index 988f6cf1d..823df5cb7 100644 --- a/clippy_utils/src/visitors.rs +++ b/clippy_utils/src/visitors.rs @@ -1,9 +1,11 @@ use crate::path_to_local_id; use rustc_hir as hir; +use rustc_hir::def::{DefKind, Res}; use rustc_hir::intravisit::{self, walk_expr, NestedVisitorMap, Visitor}; -use rustc_hir::{def::Res, Arm, Block, Body, BodyId, Expr, ExprKind, HirId, Stmt}; +use rustc_hir::{Arm, Block, Body, BodyId, Expr, ExprKind, HirId, Stmt, UnOp}; use rustc_lint::LateContext; use rustc_middle::hir::map::Map; +use rustc_middle::ty; /// Convenience method for creating a `Visitor` with just `visit_expr` overridden and nested /// bodies (i.e. closures) are visited. @@ -225,3 +227,93 @@ pub fn is_local_used(cx: &LateContext<'tcx>, visitable: impl Visitable<'tcx>, id drop(visitor); is_used } + +/// Checks if the given expression is a constant. +pub fn is_const_evaluatable(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool { + struct V<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + is_const: bool, + } + impl<'tcx> Visitor<'tcx> for V<'_, 'tcx> { + type Map = Map<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) + } + + fn visit_expr(&mut self, e: &'tcx Expr<'_>) { + if !self.is_const { + return; + } + match e.kind { + ExprKind::ConstBlock(_) => return, + ExprKind::Call( + &Expr { + kind: ExprKind::Path(ref p), + hir_id, + .. + }, + _, + ) if self + .cx + .qpath_res(p, hir_id) + .opt_def_id() + .map_or(false, |id| self.cx.tcx.is_const_fn_raw(id)) => {}, + ExprKind::MethodCall(..) + if self + .cx + .typeck_results() + .type_dependent_def_id(e.hir_id) + .map_or(false, |id| self.cx.tcx.is_const_fn_raw(id)) => {}, + ExprKind::Binary(_, lhs, rhs) + if self.cx.typeck_results().expr_ty(lhs).peel_refs().is_primitive_ty() + && self.cx.typeck_results().expr_ty(rhs).peel_refs().is_primitive_ty() => {}, + ExprKind::Unary(UnOp::Deref, e) if self.cx.typeck_results().expr_ty(e).is_ref() => (), + ExprKind::Unary(_, e) if self.cx.typeck_results().expr_ty(e).peel_refs().is_primitive_ty() => (), + ExprKind::Index(base, _) + if matches!( + self.cx.typeck_results().expr_ty(base).peel_refs().kind(), + ty::Slice(_) | ty::Array(..) + ) => {}, + ExprKind::Path(ref p) + if matches!( + self.cx.qpath_res(p, e.hir_id), + Res::Def( + DefKind::Const + | DefKind::AssocConst + | DefKind::AnonConst + | DefKind::ConstParam + | DefKind::Ctor(..) + | DefKind::Fn + | DefKind::AssocFn, + _ + ) | Res::SelfCtor(_) + ) => {}, + + ExprKind::AddrOf(..) + | ExprKind::Array(_) + | ExprKind::Block(..) + | ExprKind::Cast(..) + | ExprKind::DropTemps(_) + | ExprKind::Field(..) + | ExprKind::If(..) + | ExprKind::Let(..) + | ExprKind::Lit(_) + | ExprKind::Match(..) + | ExprKind::Repeat(..) + | ExprKind::Struct(..) + | ExprKind::Tup(_) + | ExprKind::Type(..) => (), + + _ => { + self.is_const = false; + return; + }, + } + walk_expr(self, e); + } + } + + let mut v = V { cx, is_const: true }; + v.visit_expr(e); + v.is_const +} diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index 812ce7163..83f467c84 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -108,6 +108,7 @@ fn test_return_in_macro() { } mod issue6501 { + #[allow(clippy::unnecessary_lazy_evaluations)] fn foo(bar: Result<(), ()>) { bar.unwrap_or_else(|_| {}) } diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index c42567b51..341caf18b 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -108,6 +108,7 @@ fn test_return_in_macro() { } mod issue6501 { + #[allow(clippy::unnecessary_lazy_evaluations)] fn foo(bar: Result<(), ()>) { bar.unwrap_or_else(|_| return) } diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr index 74dda971f..c0abc2c63 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -85,109 +85,109 @@ LL | return String::new(); | ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::new()` error: unneeded `return` statement - --> $DIR/needless_return.rs:112:32 + --> $DIR/needless_return.rs:113:32 | LL | bar.unwrap_or_else(|_| return) | ^^^^^^ help: replace `return` with an empty block: `{}` error: unneeded `return` statement - --> $DIR/needless_return.rs:117:13 + --> $DIR/needless_return.rs:118:13 | LL | return; | ^^^^^^^ help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:119:20 + --> $DIR/needless_return.rs:120:20 | LL | let _ = || return; | ^^^^^^ help: replace `return` with an empty block: `{}` error: unneeded `return` statement - --> $DIR/needless_return.rs:125:32 + --> $DIR/needless_return.rs:126:32 | LL | res.unwrap_or_else(|_| return Foo) | ^^^^^^^^^^ help: remove `return`: `Foo` error: unneeded `return` statement - --> $DIR/needless_return.rs:134:5 + --> $DIR/needless_return.rs:135:5 | LL | return true; | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:138:5 + --> $DIR/needless_return.rs:139:5 | LL | return true; | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:143:9 + --> $DIR/needless_return.rs:144:9 | LL | return true; | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:145:9 + --> $DIR/needless_return.rs:146:9 | LL | return false; | ^^^^^^^^^^^^^ help: remove `return`: `false` error: unneeded `return` statement - --> $DIR/needless_return.rs:151:17 + --> $DIR/needless_return.rs:152:17 | LL | true => return false, | ^^^^^^^^^^^^ help: remove `return`: `false` error: unneeded `return` statement - --> $DIR/needless_return.rs:153:13 + --> $DIR/needless_return.rs:154:13 | LL | return true; | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:160:9 + --> $DIR/needless_return.rs:161:9 | LL | return true; | ^^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:162:16 + --> $DIR/needless_return.rs:163:16 | LL | let _ = || return true; | ^^^^^^^^^^^ help: remove `return`: `true` error: unneeded `return` statement - --> $DIR/needless_return.rs:170:5 + --> $DIR/needless_return.rs:171:5 | LL | return; | ^^^^^^^ help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:175:9 + --> $DIR/needless_return.rs:176:9 | LL | return; | ^^^^^^^ help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:177:9 + --> $DIR/needless_return.rs:178:9 | LL | return; | ^^^^^^^ help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:184:14 + --> $DIR/needless_return.rs:185:14 | LL | _ => return, | ^^^^^^ help: replace `return` with an empty block: `{}` error: unneeded `return` statement - --> $DIR/needless_return.rs:199:9 + --> $DIR/needless_return.rs:200:9 | LL | return String::from("test"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::from("test")` error: unneeded `return` statement - --> $DIR/needless_return.rs:201:9 + --> $DIR/needless_return.rs:202:9 | LL | return String::new(); | ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::new()` diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed index 9fdea79fe..ce3093c54 100644 --- a/tests/ui/option_if_let_else.fixed +++ b/tests/ui/option_if_let_else.fixed @@ -121,7 +121,7 @@ fn main() { let s = String::new(); // Lint, both branches immutably borrow `s`. - let _ = Some(0).map_or_else(|| s.len(), |x| s.len() + x); + let _ = Some(0).map_or(s.len(), |x| s.len() + x); let s = String::new(); // Lint, `Some` branch consumes `s`, but else branch doesn't use `s`. diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index d7711bc1c..4e64cd7cd 100644 --- a/tests/ui/option_if_let_else.stderr +++ b/tests/ui/option_if_let_else.stderr @@ -180,11 +180,11 @@ LL + } LL ~ }); | -error: use Option::map_or_else instead of an if let/else +error: use Option::map_or instead of an if let/else --> $DIR/option_if_let_else.rs:151:13 | LL | let _ = if let Some(x) = Some(0) { s.len() + x } else { s.len() }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or_else(|| s.len(), |x| s.len() + x)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Some(0).map_or(s.len(), |x| s.len() + x)` error: use Option::map_or instead of an if let/else --> $DIR/option_if_let_else.rs:155:13 diff --git a/tests/ui/or_fun_call.fixed b/tests/ui/or_fun_call.fixed index c2f94d0e8..d6d6ab497 100644 --- a/tests/ui/or_fun_call.fixed +++ b/tests/ui/or_fun_call.fixed @@ -43,7 +43,7 @@ fn or_fun_call() { with_enum.unwrap_or(Enum::A(5)); let with_const_fn = Some(Duration::from_secs(1)); - with_const_fn.unwrap_or_else(|| Duration::from_secs(5)); + with_const_fn.unwrap_or(Duration::from_secs(5)); let with_constructor = Some(vec![1]); with_constructor.unwrap_or_else(make); @@ -79,16 +79,16 @@ fn or_fun_call() { without_default.unwrap_or_else(Foo::new); let mut map = HashMap::::new(); - map.entry(42).or_insert_with(String::new); + map.entry(42).or_insert(String::new()); let mut map_vec = HashMap::>::new(); - map_vec.entry(42).or_insert_with(Vec::new); + map_vec.entry(42).or_insert(vec![]); let mut btree = BTreeMap::::new(); - btree.entry(42).or_insert_with(String::new); + btree.entry(42).or_insert(String::new()); let mut btree_vec = BTreeMap::>::new(); - btree_vec.entry(42).or_insert_with(Vec::new); + btree_vec.entry(42).or_insert(vec![]); let stringy = Some(String::from("")); let _ = stringy.unwrap_or_else(|| "".to_owned()); @@ -129,7 +129,7 @@ fn test_or_with_ctors() { let b = "b".to_string(); let _ = Some(Bar("a".to_string(), Duration::from_secs(1))) - .or_else(|| Some(Bar(b, Duration::from_secs(2)))); + .or(Some(Bar(b, Duration::from_secs(2)))); let vec = vec!["foo"]; let _ = opt.ok_or(vec.len()); @@ -155,16 +155,24 @@ fn f() -> Option<()> { } mod issue6675 { + unsafe fn ptr_to_ref<'a, T>(p: *const T) -> &'a T { + #[allow(unused)] + let x = vec![0; 1000]; // future-proofing, make this function expensive. + &*p + } + unsafe fn foo() { - let mut s = "test".to_owned(); - None.unwrap_or_else(|| s.as_mut_vec()); + let s = "test".to_owned(); + let s = &s as *const _; + None.unwrap_or_else(|| ptr_to_ref(s)); } fn bar() { - let mut s = "test".to_owned(); - None.unwrap_or_else(|| unsafe { s.as_mut_vec() }); + let s = "test".to_owned(); + let s = &s as *const _; + None.unwrap_or_else(|| unsafe { ptr_to_ref(s) }); #[rustfmt::skip] - None.unwrap_or_else(|| unsafe { s.as_mut_vec() }); + None.unwrap_or_else(|| unsafe { ptr_to_ref(s) }); } } diff --git a/tests/ui/or_fun_call.rs b/tests/ui/or_fun_call.rs index afaf92961..8eadc6ce3 100644 --- a/tests/ui/or_fun_call.rs +++ b/tests/ui/or_fun_call.rs @@ -155,16 +155,24 @@ fn f() -> Option<()> { } mod issue6675 { + unsafe fn ptr_to_ref<'a, T>(p: *const T) -> &'a T { + #[allow(unused)] + let x = vec![0; 1000]; // future-proofing, make this function expensive. + &*p + } + unsafe fn foo() { - let mut s = "test".to_owned(); - None.unwrap_or(s.as_mut_vec()); + let s = "test".to_owned(); + let s = &s as *const _; + None.unwrap_or(ptr_to_ref(s)); } fn bar() { - let mut s = "test".to_owned(); - None.unwrap_or(unsafe { s.as_mut_vec() }); + let s = "test".to_owned(); + let s = &s as *const _; + None.unwrap_or(unsafe { ptr_to_ref(s) }); #[rustfmt::skip] - None.unwrap_or( unsafe { s.as_mut_vec() } ); + None.unwrap_or( unsafe { ptr_to_ref(s) } ); } } diff --git a/tests/ui/or_fun_call.stderr b/tests/ui/or_fun_call.stderr index b2bcbd38c..9d0c42b10 100644 --- a/tests/ui/or_fun_call.stderr +++ b/tests/ui/or_fun_call.stderr @@ -1,16 +1,10 @@ -error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:46:19 - | -LL | with_const_fn.unwrap_or(Duration::from_secs(5)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| Duration::from_secs(5))` - | - = note: `-D clippy::or-fun-call` implied by `-D warnings` - error: use of `unwrap_or` followed by a function call --> $DIR/or_fun_call.rs:49:22 | LL | with_constructor.unwrap_or(make()); | ^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(make)` + | + = note: `-D clippy::or-fun-call` implied by `-D warnings` error: use of `unwrap_or` followed by a call to `new` --> $DIR/or_fun_call.rs:52:5 @@ -72,30 +66,6 @@ error: use of `unwrap_or` followed by a function call LL | without_default.unwrap_or(Foo::new()); | ^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(Foo::new)` -error: use of `or_insert` followed by a function call - --> $DIR/or_fun_call.rs:82:19 - | -LL | map.entry(42).or_insert(String::new()); - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(String::new)` - -error: use of `or_insert` followed by a function call - --> $DIR/or_fun_call.rs:85:23 - | -LL | map_vec.entry(42).or_insert(vec![]); - | ^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(Vec::new)` - -error: use of `or_insert` followed by a function call - --> $DIR/or_fun_call.rs:88:21 - | -LL | btree.entry(42).or_insert(String::new()); - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(String::new)` - -error: use of `or_insert` followed by a function call - --> $DIR/or_fun_call.rs:91:25 - | -LL | btree_vec.entry(42).or_insert(vec![]); - | ^^^^^^^^^^^^^^^^^ help: try this: `or_insert_with(Vec::new)` - error: use of `unwrap_or` followed by a function call --> $DIR/or_fun_call.rs:94:21 | @@ -120,29 +90,23 @@ error: use of `or` followed by a function call LL | let _ = Some("a".to_string()).or(Some("b".to_string())); | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some("b".to_string()))` -error: use of `or` followed by a function call - --> $DIR/or_fun_call.rs:132:10 - | -LL | .or(Some(Bar(b, Duration::from_secs(2)))); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `or_else(|| Some(Bar(b, Duration::from_secs(2))))` - -error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:160:14 - | -LL | None.unwrap_or(s.as_mut_vec()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| s.as_mut_vec())` - -error: use of `unwrap_or` followed by a function call - --> $DIR/or_fun_call.rs:165:14 - | -LL | None.unwrap_or(unsafe { s.as_mut_vec() }); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| unsafe { s.as_mut_vec() })` - error: use of `unwrap_or` followed by a function call --> $DIR/or_fun_call.rs:167:14 | -LL | None.unwrap_or( unsafe { s.as_mut_vec() } ); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| unsafe { s.as_mut_vec() })` +LL | None.unwrap_or(ptr_to_ref(s)); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| ptr_to_ref(s))` -error: aborting due to 24 previous errors +error: use of `unwrap_or` followed by a function call + --> $DIR/or_fun_call.rs:173:14 + | +LL | None.unwrap_or(unsafe { ptr_to_ref(s) }); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| unsafe { ptr_to_ref(s) })` + +error: use of `unwrap_or` followed by a function call + --> $DIR/or_fun_call.rs:175:14 + | +LL | None.unwrap_or( unsafe { ptr_to_ref(s) } ); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| unsafe { ptr_to_ref(s) })` + +error: aborting due to 18 previous errors