diff options
Diffstat (limited to 'clippy_lints/src')
46 files changed, 1285 insertions, 296 deletions
diff --git a/clippy_lints/src/atomic_ordering.rs b/clippy_lints/src/atomic_ordering.rs index 2d964ac2b9f..703d8a6f62b 100644 --- a/clippy_lints/src/atomic_ordering.rs +++ b/clippy_lints/src/atomic_ordering.rs @@ -8,7 +8,8 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; declare_clippy_lint! { /// **What it does:** Checks for usage of invalid atomic - /// ordering in atomic loads/stores and memory fences. + /// ordering in atomic loads/stores/exchanges/updates and + /// memory fences. /// /// **Why is this bad?** Using an invalid atomic ordering /// will cause a panic at run-time. @@ -17,22 +18,35 @@ declare_clippy_lint! { /// /// **Example:** /// ```rust,no_run - /// # use std::sync::atomic::{self, AtomicBool, Ordering}; + /// # use std::sync::atomic::{self, AtomicU8, Ordering}; /// - /// let x = AtomicBool::new(true); + /// let x = AtomicU8::new(0); /// + /// // Bad: `Release` and `AcqRel` cannot be used for `load`. /// let _ = x.load(Ordering::Release); /// let _ = x.load(Ordering::AcqRel); /// - /// x.store(false, Ordering::Acquire); - /// x.store(false, Ordering::AcqRel); + /// // Bad: `Acquire` and `AcqRel` cannot be used for `store`. + /// x.store(1, Ordering::Acquire); + /// x.store(2, Ordering::AcqRel); /// + /// // Bad: `Relaxed` cannot be used as a fence's ordering. /// atomic::fence(Ordering::Relaxed); /// atomic::compiler_fence(Ordering::Relaxed); + /// + /// // Bad: `Release` and `AcqRel` are both always invalid + /// // for the failure ordering (the last arg). + /// let _ = x.compare_exchange(1, 2, Ordering::SeqCst, Ordering::Release); + /// let _ = x.compare_exchange_weak(2, 3, Ordering::AcqRel, Ordering::AcqRel); + /// + /// // Bad: The failure ordering is not allowed to be + /// // stronger than the success order, and `SeqCst` is + /// // stronger than `Relaxed`. + /// let _ = x.fetch_update(Ordering::Relaxed, Ordering::SeqCst, |val| Some(val + val)); /// ``` pub INVALID_ATOMIC_ORDERING, correctness, - "usage of invalid atomic ordering in atomic loads/stores and memory fences" + "usage of invalid atomic ordering in atomic operations and memory fences" } declare_lint_pass!(AtomicOrdering => [INVALID_ATOMIC_ORDERING]); @@ -127,9 +141,89 @@ fn check_memory_fence(cx: &LateContext<'_>, expr: &Expr<'_>) { } } +fn opt_ordering_defid(cx: &LateContext<'_>, ord_arg: &Expr<'_>) -> Option<DefId> { + if let ExprKind::Path(ref ord_qpath) = ord_arg.kind { + cx.qpath_res(ord_qpath, ord_arg.hir_id).opt_def_id() + } else { + None + } +} + +fn check_atomic_compare_exchange(cx: &LateContext<'_>, expr: &Expr<'_>) { + if_chain! { + if let ExprKind::MethodCall(ref method_path, _, args, _) = &expr.kind; + let method = method_path.ident.name.as_str(); + if type_is_atomic(cx, &args[0]); + if method == "compare_exchange" || method == "compare_exchange_weak" || method == "fetch_update"; + let (success_order_arg, failure_order_arg) = if method == "fetch_update" { + (&args[1], &args[2]) + } else { + (&args[3], &args[4]) + }; + if let Some(fail_ordering_def_id) = opt_ordering_defid(cx, failure_order_arg); + then { + // Helper type holding on to some checking and error reporting data. Has + // - (success ordering name, + // - list of failure orderings forbidden by the success order, + // - suggestion message) + type OrdLintInfo = (&'static str, &'static [&'static str], &'static str); + let relaxed: OrdLintInfo = ("Relaxed", &["SeqCst", "Acquire"], "ordering mode `Relaxed`"); + let acquire: OrdLintInfo = ("Acquire", &["SeqCst"], "ordering modes `Acquire` or `Relaxed`"); + let seq_cst: OrdLintInfo = ("SeqCst", &[], "ordering modes `Acquire`, `SeqCst` or `Relaxed`"); + let release = ("Release", relaxed.1, relaxed.2); + let acqrel = ("AcqRel", acquire.1, acquire.2); + let search = [relaxed, acquire, seq_cst, release, acqrel]; + + let success_lint_info = opt_ordering_defid(cx, success_order_arg) + .and_then(|success_ord_def_id| -> Option<OrdLintInfo> { + search + .iter() + .find(|(ordering, ..)| { + match_def_path(cx, success_ord_def_id, + &["core", "sync", "atomic", "Ordering", ordering]) + }) + .copied() + }); + + if match_ordering_def_path(cx, fail_ordering_def_id, &["Release", "AcqRel"]) { + // If we don't know the success order is, use what we'd suggest + // if it were maximally permissive. + let suggested = success_lint_info.unwrap_or(seq_cst).2; + span_lint_and_help( + cx, + INVALID_ATOMIC_ORDERING, + failure_order_arg.span, + &format!( + "{}'s failure ordering may not be `Release` or `AcqRel`", + method, + ), + None, + &format!("consider using {} instead", suggested), + ); + } else if let Some((success_ord_name, bad_ords_given_success, suggested)) = success_lint_info { + if match_ordering_def_path(cx, fail_ordering_def_id, bad_ords_given_success) { + span_lint_and_help( + cx, + INVALID_ATOMIC_ORDERING, + failure_order_arg.span, + &format!( + "{}'s failure ordering may not be stronger than the success ordering of `{}`", + method, + success_ord_name, + ), + None, + &format!("consider using {} instead", suggested), + ); + } + } + } + } +} + impl<'tcx> LateLintPass<'tcx> for AtomicOrdering { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { check_atomic_load_store(cx, expr); check_memory_fence(cx, expr); + check_atomic_compare_exchange(cx, expr); } } diff --git a/clippy_lints/src/await_holding_lock.rs b/clippy_lints/src/await_holding_lock.rs index f18e7e5d997..367534499fd 100644 --- a/clippy_lints/src/await_holding_lock.rs +++ b/clippy_lints/src/await_holding_lock.rs @@ -10,7 +10,7 @@ declare_clippy_lint! { /// **What it does:** Checks for calls to await while holding a /// non-async-aware MutexGuard. /// - /// **Why is this bad?** The Mutex types found in syd::sync and parking_lot + /// **Why is this bad?** The Mutex types found in std::sync and parking_lot /// are not designed to operate in an async context across await points. /// /// There are two potential solutions. One is to use an asynx-aware Mutex diff --git a/clippy_lints/src/bit_mask.rs b/clippy_lints/src/bit_mask.rs index 81a34021e8a..a4ee54076ee 100644 --- a/clippy_lints/src/bit_mask.rs +++ b/clippy_lints/src/bit_mask.rs @@ -90,7 +90,7 @@ declare_clippy_lint! { /// if x & 0b1111 == 0 { } /// ``` pub VERBOSE_BIT_MASK, - style, + pedantic, "expressions where a bit mask is less readable than the corresponding method call" } diff --git a/clippy_lints/src/bytecount.rs b/clippy_lints/src/bytecount.rs index 189c07427ae..d7d02ebf985 100644 --- a/clippy_lints/src/bytecount.rs +++ b/clippy_lints/src/bytecount.rs @@ -1,6 +1,5 @@ use crate::utils::{ - contains_name, get_pat_name, match_type, paths, single_segment_path, snippet_with_applicability, - span_lint_and_sugg, walk_ptrs_ty, + contains_name, get_pat_name, match_type, paths, single_segment_path, snippet_with_applicability, span_lint_and_sugg, }; use if_chain::if_chain; use rustc_ast::ast::UintTy; @@ -53,7 +52,7 @@ impl<'tcx> LateLintPass<'tcx> for ByteCount { if let ExprKind::Binary(ref op, ref l, ref r) = body.value.kind; if op.node == BinOpKind::Eq; if match_type(cx, - walk_ptrs_ty(cx.typeck_results().expr_ty(&filter_args[0])), + cx.typeck_results().expr_ty(&filter_args[0]).peel_refs(), &paths::SLICE_ITER); then { let needle = match get_path_name(l) { @@ -63,7 +62,7 @@ impl<'tcx> LateLintPass<'tcx> for ByteCount { _ => { return; } } }; - if ty::Uint(UintTy::U8) != *walk_ptrs_ty(cx.typeck_results().expr_ty(needle)).kind() { + if ty::Uint(UintTy::U8) != *cx.typeck_results().expr_ty(needle).peel_refs().kind() { return; } let haystack = if let ExprKind::MethodCall(ref path, _, ref args, _) = diff --git a/clippy_lints/src/consts.rs b/clippy_lints/src/consts.rs index 3ee022e4e68..0000d39263e 100644 --- a/clippy_lints/src/consts.rs +++ b/clippy_lints/src/consts.rs @@ -21,7 +21,7 @@ pub enum Constant { /// A `String` (e.g., "abc"). Str(String), /// A binary string (e.g., `b"abc"`). - Binary(Lrc<Vec<u8>>), + Binary(Lrc<[u8]>), /// A single `char` (e.g., `'a'`). Char(char), /// An integer's bit representation. @@ -155,7 +155,7 @@ pub fn lit_to_constant(lit: &LitKind, ty: Option<Ty<'_>>) -> Constant { match *lit { LitKind::Str(ref is, _) => Constant::Str(is.to_string()), LitKind::Byte(b) => Constant::Int(u128::from(b)), - LitKind::ByteStr(ref s) => Constant::Binary(Lrc::clone(s)), + LitKind::ByteStr(ref s) => Constant::Binary(Lrc::from(s.as_slice())), LitKind::Char(c) => Constant::Char(c), LitKind::Int(n, _) => Constant::Int(n), LitKind::Float(ref is, LitFloatType::Suffixed(fty)) => match fty { diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index 50121a054c7..62bb70af06e 100644 --- a/clippy_lints/src/doc.rs +++ b/clippy_lints/src/doc.rs @@ -534,7 +534,7 @@ fn check_word(cx: &LateContext<'_>, word: &str, span: Span) { return false; } - let s = if s.ends_with('s') { &s[..s.len() - 1] } else { s }; + let s = s.strip_suffix('s').unwrap_or(s); s.chars().all(char::is_alphanumeric) && s.chars().filter(|&c| c.is_uppercase()).take(2).count() > 1 diff --git a/clippy_lints/src/duration_subsec.rs b/clippy_lints/src/duration_subsec.rs index 8ece44878fe..c0529a34cc4 100644 --- a/clippy_lints/src/duration_subsec.rs +++ b/clippy_lints/src/duration_subsec.rs @@ -7,7 +7,7 @@ use rustc_span::source_map::Spanned; use crate::consts::{constant, Constant}; use crate::utils::paths; -use crate::utils::{match_type, snippet_with_applicability, span_lint_and_sugg, walk_ptrs_ty}; +use crate::utils::{match_type, snippet_with_applicability, span_lint_and_sugg}; declare_clippy_lint! { /// **What it does:** Checks for calculation of subsecond microseconds or milliseconds @@ -43,7 +43,7 @@ impl<'tcx> LateLintPass<'tcx> for DurationSubsec { if_chain! { if let ExprKind::Binary(Spanned { node: BinOpKind::Div, .. }, ref left, ref right) = expr.kind; if let ExprKind::MethodCall(ref method_path, _ , ref args, _) = left.kind; - if match_type(cx, walk_ptrs_ty(cx.typeck_results().expr_ty(&args[0])), &paths::DURATION); + if match_type(cx, cx.typeck_results().expr_ty(&args[0]).peel_refs(), &paths::DURATION); if let Some((Constant::Int(divisor), _)) = constant(cx, cx.typeck_results(), right); then { let suggested_fn = match (method_path.ident.as_str().as_ref(), divisor) { diff --git a/clippy_lints/src/entry.rs b/clippy_lints/src/entry.rs index d616502a82a..35a5d00f4aa 100644 --- a/clippy_lints/src/entry.rs +++ b/clippy_lints/src/entry.rs @@ -1,6 +1,6 @@ use crate::utils::SpanlessEq; use crate::utils::{get_item_name, higher, is_type_diagnostic_item, match_type, paths, snippet, snippet_opt}; -use crate::utils::{snippet_with_applicability, span_lint_and_then, walk_ptrs_ty}; +use crate::utils::{snippet_with_applicability, span_lint_and_then}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; @@ -106,7 +106,7 @@ fn check_cond<'a>(cx: &LateContext<'_>, check: &'a Expr<'a>) -> Option<(&'static if let ExprKind::AddrOf(BorrowKind::Ref, _, ref key) = params[1].kind; then { let map = ¶ms[0]; - let obj_ty = walk_ptrs_ty(cx.typeck_results().expr_ty(map)); + let obj_ty = cx.typeck_results().expr_ty(map).peel_refs(); return if match_type(cx, obj_ty, &paths::BTREEMAP) { Some(("BTreeMap", map, key)) diff --git a/clippy_lints/src/fallible_impl_from.rs b/clippy_lints/src/fallible_impl_from.rs index 000762334f6..a9e05fddbe7 100644 --- a/clippy_lints/src/fallible_impl_from.rs +++ b/clippy_lints/src/fallible_impl_from.rs @@ -1,7 +1,5 @@ use crate::utils::paths::{BEGIN_PANIC, BEGIN_PANIC_FMT, FROM_TRAIT}; -use crate::utils::{ - is_expn_of, is_type_diagnostic_item, match_def_path, method_chain_args, span_lint_and_then, walk_ptrs_ty, -}; +use crate::utils::{is_expn_of, is_type_diagnostic_item, match_def_path, method_chain_args, span_lint_and_then}; use if_chain::if_chain; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; @@ -96,7 +94,7 @@ fn lint_impl_body<'tcx>(cx: &LateContext<'tcx>, impl_span: Span, impl_items: &[h // check for `unwrap` if let Some(arglists) = method_chain_args(expr, &["unwrap"]) { - let reciever_ty = walk_ptrs_ty(self.typeck_results.expr_ty(&arglists[0][0])); + let reciever_ty = self.typeck_results.expr_ty(&arglists[0][0]).peel_refs(); if is_type_diagnostic_item(self.lcx, reciever_ty, sym!(option_type)) || is_type_diagnostic_item(self.lcx, reciever_ty, sym!(result_type)) { diff --git a/clippy_lints/src/format.rs b/clippy_lints/src/format.rs index 8bd85af8768..d6541010bca 100644 --- a/clippy_lints/src/format.rs +++ b/clippy_lints/src/format.rs @@ -1,7 +1,7 @@ use crate::utils::paths; use crate::utils::{ is_expn_of, is_type_diagnostic_item, last_path_segment, match_def_path, match_function_call, snippet, - span_lint_and_then, walk_ptrs_ty, + span_lint_and_then, }; use if_chain::if_chain; use rustc_ast::ast::LitKind; @@ -90,7 +90,7 @@ fn on_argumentv1_new<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, arms: & if let PatKind::Tuple(ref pats, None) = arms[0].pat.kind; if pats.len() == 1; then { - let ty = walk_ptrs_ty(cx.typeck_results().pat_ty(&pats[0])); + let ty = cx.typeck_results().pat_ty(&pats[0]).peel_refs(); if *ty.kind() != rustc_middle::ty::Str && !is_type_diagnostic_item(cx, ty, sym!(string_type)) { return None; } diff --git a/clippy_lints/src/indexing_slicing.rs b/clippy_lints/src/indexing_slicing.rs index a28eda8be15..741195f3b10 100644 --- a/clippy_lints/src/indexing_slicing.rs +++ b/clippy_lints/src/indexing_slicing.rs @@ -88,7 +88,7 @@ declare_lint_pass!(IndexingSlicing => [INDEXING_SLICING, OUT_OF_BOUNDS_INDEXING] impl<'tcx> LateLintPass<'tcx> for IndexingSlicing { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { if let ExprKind::Index(ref array, ref index) = &expr.kind { - let ty = cx.typeck_results().expr_ty(array); + let ty = cx.typeck_results().expr_ty(array).peel_refs(); if let Some(range) = higher::range(index) { // Ranged indexes, i.e., &x[n..m], &x[n..], &x[..n] and &x[..] if let ty::Array(_, s) = ty.kind() { diff --git a/clippy_lints/src/inherent_to_string.rs b/clippy_lints/src/inherent_to_string.rs index f330fa8fab8..0877b44d901 100644 --- a/clippy_lints/src/inherent_to_string.rs +++ b/clippy_lints/src/inherent_to_string.rs @@ -5,7 +5,7 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; use crate::utils::{ get_trait_def_id, implements_trait, is_type_diagnostic_item, paths, return_ty, span_lint_and_help, - trait_ref_of_method, walk_ptrs_ty, + trait_ref_of_method, }; declare_clippy_lint! { @@ -125,7 +125,7 @@ fn show_lint(cx: &LateContext<'_>, item: &ImplItem<'_>) { // Get the real type of 'self' let fn_def_id = cx.tcx.hir().local_def_id(item.hir_id); let self_type = cx.tcx.fn_sig(fn_def_id).input(0); - let self_type = walk_ptrs_ty(self_type.skip_binder()); + let self_type = self_type.skip_binder().peel_refs(); // Emit either a warning or an error if implements_trait(cx, self_type, display_trait_id, &[]) { diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index 42a98dc963d..c9c4891bb08 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -1,4 +1,4 @@ -use crate::utils::{get_item_name, snippet_with_applicability, span_lint, span_lint_and_sugg, walk_ptrs_ty}; +use crate::utils::{get_item_name, snippet_with_applicability, span_lint, span_lint_and_sugg}; use rustc_ast::ast::LitKind; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; @@ -285,7 +285,7 @@ fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { }) } - let ty = &walk_ptrs_ty(cx.typeck_results().expr_ty(expr)); + let ty = &cx.typeck_results().expr_ty(expr).peel_refs(); match ty.kind() { ty::Dynamic(ref tt, ..) => tt.principal().map_or(false, |principal| { cx.tcx diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2020ef78509..58112ac8da5 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -230,7 +230,9 @@ mod macro_use; mod main_recursion; mod manual_async_fn; mod manual_non_exhaustive; +mod manual_strip; mod map_clone; +mod map_err_ignore; mod map_identity; mod map_unit_fn; mod match_on_vec_items; @@ -269,6 +271,7 @@ mod open_options; mod option_env_unwrap; mod option_if_let_else; mod overflow_check_conditional; +mod panic_in_result_fn; mod panic_unimplemented; mod partialeq_ne_impl; mod path_buf_push_overwrite; @@ -625,7 +628,9 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &main_recursion::MAIN_RECURSION, &manual_async_fn::MANUAL_ASYNC_FN, &manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE, + &manual_strip::MANUAL_STRIP, &map_clone::MAP_CLONE, + &map_err_ignore::MAP_ERR_IGNORE, &map_identity::MAP_IDENTITY, &map_unit_fn::OPTION_MAP_UNIT_FN, &map_unit_fn::RESULT_MAP_UNIT_FN, @@ -751,6 +756,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &option_env_unwrap::OPTION_ENV_UNWRAP, &option_if_let_else::OPTION_IF_LET_ELSE, &overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL, + &panic_in_result_fn::PANIC_IN_RESULT_FN, &panic_unimplemented::PANIC, &panic_unimplemented::PANIC_PARAMS, &panic_unimplemented::TODO, @@ -835,6 +841,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &types::LET_UNIT_VALUE, &types::LINKEDLIST, &types::OPTION_OPTION, + &types::RC_BUFFER, &types::REDUNDANT_ALLOCATION, &types::TYPE_COMPLEXITY, &types::UNIT_ARG, @@ -863,6 +870,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &utils::internal_lints::COMPILER_LINT_FUNCTIONS, &utils::internal_lints::DEFAULT_LINT, &utils::internal_lints::LINT_WITHOUT_LINT_PASS, + &utils::internal_lints::MATCH_TYPE_ON_DIAGNOSTIC_ITEM, &utils::internal_lints::OUTER_EXPN_EXPN_DATA, &utils::internal_lints::PRODUCE_ICE, &vec::USELESS_VEC, @@ -918,6 +926,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box implicit_saturating_sub::ImplicitSaturatingSub); store.register_late_pass(|| box methods::Methods); store.register_late_pass(|| box map_clone::MapClone); + store.register_late_pass(|| box map_err_ignore::MapErrIgnore); store.register_late_pass(|| box shadow::Shadow); store.register_late_pass(|| box types::LetUnitValue); store.register_late_pass(|| box types::UnitCmp); @@ -1091,6 +1100,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box manual_async_fn::ManualAsyncFn); store.register_early_pass(|| box redundant_field_names::RedundantFieldNames); store.register_late_pass(|| box vec_resize_to_zero::VecResizeToZero); + store.register_late_pass(|| box panic_in_result_fn::PanicInResultFn); + let single_char_binding_names_threshold = conf.single_char_binding_names_threshold; store.register_early_pass(move || box non_expressive_names::NonExpressiveNames { single_char_binding_names_threshold, @@ -1105,6 +1116,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box self_assignment::SelfAssignment); store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs); store.register_late_pass(|| box async_yields_async::AsyncYieldsAsync); + store.register_late_pass(|| box manual_strip::ManualStrip); + store.register_late_pass(|| box utils::internal_lints::MatchTypeOnDiagItem); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1135,6 +1148,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS), LintId::of(&missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS), LintId::of(&modulo_arithmetic::MODULO_ARITHMETIC), + LintId::of(&panic_in_result_fn::PANIC_IN_RESULT_FN), LintId::of(&panic_unimplemented::PANIC), LintId::of(&panic_unimplemented::TODO), LintId::of(&panic_unimplemented::UNIMPLEMENTED), @@ -1152,6 +1166,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![ LintId::of(&attrs::INLINE_ALWAYS), LintId::of(&await_holding_lock::AWAIT_HOLDING_LOCK), + LintId::of(&bit_mask::VERBOSE_BIT_MASK), LintId::of(&checked_conversions::CHECKED_CONVERSIONS), LintId::of(&copies::MATCH_SAME_ARMS), LintId::of(&copies::SAME_FUNCTIONS_IN_IF_CONDITION), @@ -1180,6 +1195,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::EXPLICIT_INTO_ITER_LOOP), LintId::of(&loops::EXPLICIT_ITER_LOOP), LintId::of(¯o_use::MACRO_USE_IMPORTS), + LintId::of(&map_err_ignore::MAP_ERR_IGNORE), LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS), LintId::of(&matches::MATCH_BOOL), LintId::of(&matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS), @@ -1230,6 +1246,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&utils::internal_lints::COMPILER_LINT_FUNCTIONS), LintId::of(&utils::internal_lints::DEFAULT_LINT), LintId::of(&utils::internal_lints::LINT_WITHOUT_LINT_PASS), + LintId::of(&utils::internal_lints::MATCH_TYPE_ON_DIAGNOSTIC_ITEM), LintId::of(&utils::internal_lints::OUTER_EXPN_EXPN_DATA), LintId::of(&utils::internal_lints::PRODUCE_ICE), ]); @@ -1249,7 +1266,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&attrs::USELESS_ATTRIBUTE), LintId::of(&bit_mask::BAD_BIT_MASK), LintId::of(&bit_mask::INEFFECTIVE_BIT_MASK), - LintId::of(&bit_mask::VERBOSE_BIT_MASK), LintId::of(&blacklisted_name::BLACKLISTED_NAME), LintId::of(&blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS), LintId::of(&booleans::LOGIC_BUG), @@ -1330,6 +1346,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&main_recursion::MAIN_RECURSION), LintId::of(&manual_async_fn::MANUAL_ASYNC_FN), LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), + LintId::of(&manual_strip::MANUAL_STRIP), LintId::of(&map_clone::MAP_CLONE), LintId::of(&map_identity::MAP_IDENTITY), LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN), @@ -1474,6 +1491,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::CHAR_LIT_AS_U8), LintId::of(&types::FN_TO_NUMERIC_CAST), LintId::of(&types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION), + LintId::of(&types::RC_BUFFER), LintId::of(&types::REDUNDANT_ALLOCATION), LintId::of(&types::TYPE_COMPLEXITY), LintId::of(&types::UNIT_ARG), @@ -1507,7 +1525,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&assign_ops::ASSIGN_OP_PATTERN), LintId::of(&attrs::BLANKET_CLIPPY_RESTRICTION_LINTS), LintId::of(&attrs::UNKNOWN_CLIPPY_LINTS), - LintId::of(&bit_mask::VERBOSE_BIT_MASK), LintId::of(&blacklisted_name::BLACKLISTED_NAME), LintId::of(&blocks_in_if_conditions::BLOCKS_IN_IF_CONDITIONS), LintId::of(&collapsible_if::COLLAPSIBLE_IF), @@ -1622,6 +1639,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::EXPLICIT_COUNTER_LOOP), LintId::of(&loops::MUT_RANGE_BOUND), LintId::of(&loops::WHILE_LET_LOOP), + LintId::of(&manual_strip::MANUAL_STRIP), LintId::of(&map_identity::MAP_IDENTITY), LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN), @@ -1775,6 +1793,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), LintId::of(&stable_sort_primitive::STABLE_SORT_PRIMITIVE), LintId::of(&types::BOX_VEC), + LintId::of(&types::RC_BUFFER), LintId::of(&types::REDUNDANT_ALLOCATION), LintId::of(&vec::USELESS_VEC), ]); diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 6c54c07869a..3410341a1e3 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1114,7 +1114,7 @@ fn get_vec_push<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) -> Option<(& if let Some(self_expr) = args.get(0); if let Some(pushed_item) = args.get(1); // Check that the method being called is push() on a Vec - if match_type(cx, cx.typeck_results().expr_ty(self_expr), &paths::VEC); + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(self_expr), sym!(vec_type)); if path.ident.name.as_str() == "push"; then { return Some((self_expr, pushed_item)) @@ -2601,11 +2601,9 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont span, NEEDLESS_COLLECT_MSG, |diag| { - let (arg, pred) = if contains_arg.starts_with('&') { - ("x", &contains_arg[1..]) - } else { - ("&x", &*contains_arg) - }; + let (arg, pred) = contains_arg + .strip_prefix('&') + .map_or(("&x", &*contains_arg), |s| ("x", s)); diag.span_suggestion( span, "replace with", diff --git a/clippy_lints/src/manual_strip.rs b/clippy_lints/src/manual_strip.rs new file mode 100644 index 00000000000..4afb0ab3bad --- /dev/null +++ b/clippy_lints/src/manual_strip.rs @@ -0,0 +1,245 @@ +use crate::consts::{constant, Constant}; +use crate::utils::usage::mutated_variables; +use crate::utils::{ + eq_expr_value, higher, match_def_path, multispan_sugg, paths, qpath_res, snippet, span_lint_and_then, +}; + +use if_chain::if_chain; +use rustc_ast::ast::LitKind; +use rustc_hir::def::Res; +use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use rustc_hir::BinOpKind; +use rustc_hir::{BorrowKind, Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::hir::map::Map; +use rustc_middle::ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Spanned; +use rustc_span::Span; + +declare_clippy_lint! { + /// **What it does:** + /// Suggests using `strip_{prefix,suffix}` over `str::{starts,ends}_with` and slicing using + /// the pattern's length. + /// + /// **Why is this bad?** + /// Using `str:strip_{prefix,suffix}` is safer and may have better performance as there is no + /// slicing which may panic and the compiler does not need to insert this panic code. It is + /// also sometimes more readable as it removes the need for duplicating or storing the pattern + /// used by `str::{starts,ends}_with` and in the slicing. + /// + /// **Known problems:** + /// None. + /// + /// **Example:** + /// + /// ```rust + /// let s = "hello, world!"; + /// if s.starts_with("hello, ") { + /// assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!"); + /// } + /// ``` + /// Use instead: + /// ```rust + /// let s = "hello, world!"; + /// if let Some(end) = s.strip_prefix("hello, ") { + /// assert_eq!(end.to_uppercase(), "WORLD!"); + /// } + /// ``` + pub MANUAL_STRIP, + complexity, + "suggests using `strip_{prefix,suffix}` over `str::{starts,ends}_with` and slicing" +} + +declare_lint_pass!(ManualStrip => [MANUAL_STRIP]); + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum StripKind { + Prefix, + Suffix, +} + +impl<'tcx> LateLintPass<'tcx> for ManualStrip { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if_chain! { + if let Some((cond, then, _)) = higher::if_block(&expr); + if let ExprKind::MethodCall(_, _, [target_arg, pattern], _) = cond.kind; + if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(cond.hir_id); + if let ExprKind::Path(target_path) = &target_arg.kind; + then { + let strip_kind = if match_def_path(cx, method_def_id, &paths::STR_STARTS_WITH) { + StripKind::Prefix + } else if match_def_path(cx, method_def_id, &paths::STR_ENDS_WITH) { + StripKind::Suffix + } else { + return; + }; + let target_res = qpath_res(cx, &target_path, target_arg.hir_id); + if target_res == Res::Err { + return; + }; + + if_chain! { + if let Res::Local(hir_id) = target_res; + if let Some(used_mutably) = mutated_variables(then, cx); + if used_mutably.contains(&hir_id); + then { + return; + } + } + + let strippings = find_stripping(cx, strip_kind, target_res, pattern, then); + if !strippings.is_empty() { + + let kind_word = match strip_kind { + StripKind::Prefix => "prefix", + StripKind::Suffix => "suffix", + }; + + let test_span = expr.span.until(then.span); + span_lint_and_then(cx, MANUAL_STRIP, strippings[0], &format!("stripping a {} manually", kind_word), |diag| { + diag.span_note(test_span, &format!("the {} was tested here", kind_word)); + multispan_sugg( + diag, + &format!("try using the `strip_{}` method", kind_word), + vec![(test_span, + format!("if let Some(<stripped>) = {}.strip_{}({}) ", + snippet(cx, target_arg.span, ".."), + kind_word, + snippet(cx, pattern.span, "..")))] + .into_iter().chain(strippings.into_iter().map(|span| (span, "<stripped>".into()))), + ) + }); + } + } + } + } +} + +// Returns `Some(arg)` if `expr` matches `arg.len()` and `None` otherwise. +fn len_arg<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { + if_chain! { + if let ExprKind::MethodCall(_, _, [arg], _) = expr.kind; + if let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); + if match_def_path(cx, method_def_id, &paths::STR_LEN); + then { + Some(arg) + } else { + None + } + } +} + +// Returns the length of the `expr` if it's a constant string or char. +fn constant_length(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<u128> { + let (value, _) = constant(cx, cx.typeck_results(), expr)?; + match value { + Constant::Str(value) => Some(value.len() as u128), + Constant::Char(value) => Some(value.len_utf8() as u128), + _ => None, + } +} + +// Tests if `expr` equals the length of the pattern. +fn eq_pattern_length<'tcx>(cx: &LateContext<'tcx>, pattern: &Expr<'_>, expr: &'tcx Expr<'_>) -> bool { + if let ExprKind::Lit(Spanned { + node: LitKind::Int(n, _), + .. + }) = expr.kind + { + constant_length(cx, pattern).map_or(false, |length| length == n) + } else { + len_arg(cx, expr).map_or(false, |arg| eq_expr_value(cx, pattern, arg)) + } +} + +// Tests if `expr` is a `&str`. +fn is_ref_str(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + match cx.typeck_results().expr_ty_adjusted(&expr).kind() { + ty::Ref(_, ty, _) => ty.is_str(), + _ => false, + } +} + +// Removes the outer `AddrOf` expression if needed. +fn peel_ref<'a>(expr: &'a Expr<'_>) -> &'a Expr<'a> { + if let ExprKind::AddrOf(BorrowKind::Ref, _, unref) = &expr.kind { + unref + } else { + expr + } +} + +// Find expressions where `target` is stripped using the length of `pattern`. +// We'll suggest replacing these expressions with the result of the `strip_{prefix,suffix}` +// method. +fn find_stripping<'tcx>( + cx: &LateContext<'tcx>, + strip_kind: StripKind, + target: Res, + pattern: &'tcx Expr<'_>, + expr: &'tcx Expr<'_>, +) -> Vec<Span> { + struct StrippingFinder<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + strip_kind: StripKind, + target: Res, + pattern: &'tcx Expr<'tcx>, + results: Vec<Span>, + } + + impl<'a, 'tcx> Visitor<'tcx> for StrippingFinder<'a, 'tcx> { + type Map = Map<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { + NestedVisitorMap::None + } + + fn visit_expr(&mut self, ex: &'tcx Expr<'_>) { + if_chain! { + if is_ref_str(self.cx, ex); + let unref = peel_ref(ex); + if let ExprKind::Index(indexed, index) = &unref.kind; + if let Some(range) = higher::range(index); + if let higher::Range { start, end, .. } = range; + if let ExprKind::Path(path) = &indexed.kind; + if qpath_res(self.cx, path, ex.hir_id) == self.target; + then { + match (self.strip_kind, start, end) { + (StripKind::Prefix, Some(start), None) => { + if eq_pattern_length(self.cx, self.pattern, start) { + self.results.push(ex.span); + return; + } + }, + (StripKind::Suffix, None, Some(end)) => { + if_chain! { + if let ExprKind::Binary(Spanned { node: BinOpKind::Sub, .. }, left, right) = end.kind; + if let Some(left_arg) = len_arg(self.cx, left); + if let ExprKind::Path(left_path) = &left_arg.kind; + if qpath_res(self.cx, left_path, left_arg.hir_id) == self.target; + if eq_pattern_length(self.cx, self.pattern, right); + then { + self.results.push(ex.span); + return; + } + } + }, + _ => {} + } + } + } + + walk_expr(self, ex); + } + } + + let mut finder = StrippingFinder { + cx, + strip_kind, + target, + pattern, + results: vec![], + }; + walk_expr(&mut finder, expr); + finder.results +} diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs new file mode 100644 index 00000000000..5298e16a04d --- /dev/null +++ b/clippy_lints/src/map_err_ignore.rs @@ -0,0 +1,147 @@ +use crate::utils::span_lint_and_help; + +use rustc_hir::{CaptureBy, Expr, ExprKind, PatKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Checks for instances of `map_err(|_| Some::Enum)` + /// + /// **Why is this bad?** This map_err throws away the original error rather than allowing the enum to contain and report the cause of the error + /// + /// **Known problems:** None. + /// + /// **Example:** + /// Before: + /// ```rust + /// use std::fmt; + /// + /// #[derive(Debug)] + /// enum Error { + /// Indivisible, + /// Remainder(u8), + /// } + /// + /// impl fmt::Display for Error { + /// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + /// match self { + /// Error::Indivisible => write!(f, "could not divide input by three"), + /// Error::Remainder(remainder) => write!( + /// f, + /// "input is not divisible by three, remainder = {}", + /// remainder + /// ), + /// } + /// } + /// } + /// + /// impl std::error::Error for Error {} + /// + /// fn divisible_by_3(input: &str) -> Result<(), Error> { + /// input + /// .parse::<i32>() + /// .map_err(|_| Error::Indivisible) + /// .map(|v| v % 3) + /// .and_then(|remainder| { + /// if remainder == 0 { + /// Ok(()) + /// } else { + /// Err(Error::Remainder(remainder as u8)) + /// } + /// }) + /// } + /// ``` + /// + /// After: + /// ```rust + /// use std::{fmt, num::ParseIntError}; + /// + /// #[derive(Debug)] + /// enum Error { + /// Indivisible(ParseIntError), + /// Remainder(u8), + /// } + /// + /// impl fmt::Display for Error { + /// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + /// match self { + /// Error::Indivisible(_) => write!(f, "could not divide input by three"), + /// Error::Remainder(remainder) => write!( + /// f, + /// "input is not divisible by three, remainder = {}", + /// remainder + /// ), + /// } + /// } + /// } + /// + /// impl std::error::Error for Error { + /// fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + /// match self { + /// Error::Indivisible(source) => Some(source), + /// _ => None, + /// } + /// } + /// } + /// + /// fn divisible_by_3(input: &str) -> Result<(), Error> { + /// input + /// .parse::<i32>() + /// .map_err(Error::Indivisible) + /// .map(|v| v % 3) + /// .and_then(|remainder| { + /// if remainder == 0 { + /// Ok(()) + /// } else { + /// Err(Error::Remainder(remainder as u8)) + /// } + /// }) + /// } + /// ``` + pub MAP_ERR_IGNORE, + pedantic, + "`map_err` should not ignore the original error" +} + +declare_lint_pass!(MapErrIgnore => [MAP_ERR_IGNORE]); + +impl<'tcx> LateLintPass<'tcx> for MapErrIgnore { + // do not try to lint if this is from a macro or desugaring + fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) { + if e.span.from_expansion() { + return; + } + + // check if this is a method call (e.g. x.foo()) + if let ExprKind::MethodCall(ref method, _t_span, ref args, _) = e.kind { + // only work if the method name is `map_err` and there are only 2 arguments (e.g. x.map_err(|_|[1] + // Enum::Variant[2])) + if method.ident.as_str() == "map_err" && args.len() == 2 { + // make sure the first argument is a closure, and grab the CaptureRef, body_id, and body_span fields + if let ExprKind::Closure(capture, _, body_id, body_span, _) = args[1].kind { + // check if this is by Reference (meaning there's no move statement) + if capture == CaptureBy::Ref { + // Get the closure body to check the parameters and values + let closure_body = cx.tcx.hir().body(body_id); + // make sure there's only one parameter (`|_|`) + if closure_body.params.len() == 1 { + // make sure that parameter is the wild token (`_`) + if let PatKind::Wild = closure_body.params[0].pat.kind { + // span the area of the closure capture and warn that the + // original error will be thrown away + span_lint_and_help( + cx, + MAP_ERR_IGNORE, + body_span, + "`map_err(|_|...` ignores the original error", + None, + "Consider wrapping the error in an enum variant", + ); + } + } + } + } + } + } + } +} diff --git a/clippy_lints/src/match_on_vec_items.rs b/clippy_lints/src/match_on_vec_items.rs index 57966452253..331b6c6c34a 100644 --- a/clippy_lints/src/match_on_vec_items.rs +++ b/clippy_lints/src/match_on_vec_items.rs @@ -1,4 +1,3 @@ -use crate::utils::walk_ptrs_ty; use crate::utils::{is_type_diagnostic_item, is_type_lang_item, snippet, span_lint_and_sugg}; use if_chain::if_chain; use rustc_errors::Applicability; @@ -90,12 +89,12 @@ fn is_vec_indexing<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Opti fn is_vector(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { let ty = cx.typeck_results().expr_ty(expr); - let ty = walk_ptrs_ty(ty); + let ty = ty.peel_refs(); is_type_diagnostic_item(cx, ty, sym!(vec_type)) } fn is_full_range(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { let ty = cx.typeck_results().expr_ty(expr); - let ty = walk_ptrs_ty(ty); + let ty = ty.peel_refs(); is_type_lang_item(cx, ty, LangItem::RangeFull) } diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 819846ebc79..b1a4e06d4c3 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -6,7 +6,7 @@ use crate::utils::{ expr_block, get_arg_name, get_parent_expr, in_macro, indent_of, is_allowed, is_expn_of, is_refutable, is_type_diagnostic_item, is_wild, match_qpath, match_type, match_var, multispan_sugg, remove_blocks, snippet, snippet_block, snippet_with_applicability, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, - span_lint_and_then, walk_ptrs_ty, + span_lint_and_then, }; use if_chain::if_chain; use rustc_ast::ast::LitKind; @@ -502,7 +502,7 @@ impl_lint_pass!(Matches => [ impl<'tcx> LateLintPass<'tcx> for Matches { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if in_external_macro(cx.sess(), expr.span) { + if in_external_macro(cx.sess(), expr.span) || in_macro(expr.span) { return; } @@ -794,7 +794,7 @@ fn check_overlapping_arms<'tcx>(cx: &LateContext<'tcx>, ex: &'tcx Expr<'_>, arms } fn check_wild_err_arm(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) { - let ex_ty = walk_ptrs_ty(cx.typeck_results().expr_ty(ex)); + let ex_ty = cx.typeck_results().expr_ty(ex).peel_refs(); if is_type_diagnostic_item(cx, ex_ty, sym!(result_type)) { for arm in arms { if let PatKind::TupleStruct(ref path, ref inner, _) = arm.pat.kind { diff --git a/clippy_lints/src/methods/bind_instead_of_map.rs b/clippy_lints/src/methods/bind_instead_of_map.rs index 498f12518f8..ae37942e55a 100644 --- a/clippy_lints/src/methods/bind_instead_of_map.rs +++ b/clippy_lints/src/methods/bind_instead_of_map.rs @@ -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, } } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index ba69c8266b1..dadd0f8ebb7 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -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_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<'_>, @@ -1801,14 +1774,14 @@ fn lint_or_fun_call<'tcx>( ) { if let hir::ExprKind::MethodCall(ref path, _, ref args, _) = &arg.kind { if path.ident.as_str() == "len" { - let ty = walk_ptrs_ty(cx.typeck_results().expr_ty(&args[0])); + let ty = cx.typeck_results().expr_ty(&args[0]).peel_refs(); match ty.kind() { ty::Slice(_) | ty::Array(_, _) => return, _ => (), } - if match_type(cx, ty, &paths::VEC) { + if is_type_diagnostic_item(cx, ty, sym!(vec_type)) { return; } } @@ -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); @@ -1909,7 +1881,7 @@ fn lint_expect_fun_call( && (method_name.ident.name == sym!(as_str) || method_name.ident.name == sym!(as_ref)) && { let arg_type = cx.typeck_results().expr_ty(&call_args[0]); - let base_type = walk_ptrs_ty(arg_type); + let base_type = arg_type.peel_refs(); *base_type.kind() == ty::Str || is_type_diagnostic_item(cx, base_type, sym!(string_type)) } { @@ -2170,7 +2142,7 @@ fn lint_clone_on_copy(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Exp } fn lint_clone_on_ref_ptr(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr<'_>) { - let obj_ty = walk_ptrs_ty(cx.typeck_results().expr_ty(arg)); + let obj_ty = cx.typeck_results().expr_ty(arg).peel_refs(); if let ty::Adt(_, subst) = obj_ty.kind() { let caller_type = if is_type_diagnostic_item(cx, obj_ty, sym::Rc) { @@ -2201,7 +2173,7 @@ fn lint_string_extend(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::E let arg = &args[1]; if let Some(arglists) = method_chain_args(arg, &["chars"]) { let target = &arglists[0][0]; - let self_ty = walk_ptrs_ty(cx.typeck_results().expr_ty(target)); + let self_ty = cx.typeck_results().expr_ty(target).peel_refs(); let ref_str = if *self_ty.kind() == ty::Str { "" } else if is_type_diagnostic_item(cx, self_ty, sym!(string_type)) { @@ -2229,7 +2201,7 @@ fn lint_string_extend(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::E } fn lint_extend(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) { - let obj_ty = walk_ptrs_ty(cx.typeck_results().expr_ty(&args[0])); + let obj_ty = cx.typeck_results().expr_ty(&args[0]).peel_refs(); if is_type_diagnostic_item(cx, obj_ty, sym!(string_type)) { lint_string_extend(cx, expr, args); } @@ -2412,7 +2384,7 @@ fn lint_iter_next<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, iter_ } } else if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(caller_expr), sym!(vec_type)) || matches!( - &walk_ptrs_ty(cx.typeck_results().expr_ty(caller_expr)).kind(), + &cx.typeck_results().expr_ty(caller_expr).peel_refs().kind(), ty::Array(_, _) ) { @@ -2615,7 +2587,7 @@ fn derefs_to_slice<'tcx>( /// lint use of `unwrap()` for `Option`s and `Result`s fn lint_unwrap(cx: &LateContext<'_>, expr: &hir::Expr<'_>, unwrap_args: &[hir::Expr<'_>]) { - let obj_ty = walk_ptrs_ty(cx.typeck_results().expr_ty(&unwrap_args[0])); + let obj_ty = cx.typeck_results().expr_ty(&unwrap_args[0]).peel_refs(); let mess = if is_type_diagnostic_item(cx, obj_ty, sym!(option_type)) { Some((UNWRAP_USED, "an Option", "None")) @@ -2643,7 +2615,7 @@ fn lint_unwrap(cx: &LateContext<'_>, expr: &hir::Expr<'_>, unwrap_args: &[hir::E /// lint use of `expect()` for `Option`s and `Result`s fn lint_expect(cx: &LateContext<'_>, expr: &hir::Expr<'_>, expect_args: &[hir::Expr<'_>]) { - let obj_ty = walk_ptrs_ty(cx.typeck_results().expr_ty(&expect_args[0])); + let obj_ty = cx.typeck_results().expr_ty(&expect_args[0]).peel_refs(); let mess = if is_type_diagnostic_item(cx, obj_ty, sym!(option_type)) { Some((EXPECT_USED, "an Option", "None")) @@ -3162,7 +3134,7 @@ fn lint_chars_cmp( if segment.ident.name == sym!(Some); then { let mut applicability = Applicability::MachineApplicable; - let self_ty = walk_ptrs_ty(cx.typeck_results().expr_ty_adjusted(&args[0][0])); + let self_ty = cx.typeck_results().expr_ty_adjusted(&args[0][0]).peel_refs(); if *self_ty.kind() != ty::Str { return false; @@ -3374,7 +3346,7 @@ fn lint_into_iter(cx: &LateContext<'_>, expr: &hir::Expr<'_>, self_ref_ty: Ty<'_ INTO_ITER_ON_REF, method_span, &format!( - "this `.into_iter()` call is equivalent to `.{}()` and will not move the `{}`", + "this `.into_iter()` call is equivalent to `.{}()` and will not consume the `{}`", method_name, kind, ), "call directly", diff --git a/clippy_lints/src/methods/unnecessary_lazy_eval.rs b/clippy_lints/src/methods/unnecessary_lazy_eval.rs index 31517659c34..08b3eab9b7c 100644 --- a/clippy_lints/src/methods/unnecessary_lazy_eval.rs +++ b/clippy_lints/src/methods/unnecessary_lazy_eval.rs @@ -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 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 { @@ -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, ); diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index d4a50dd9013..909e79f661a 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -17,7 +17,7 @@ use crate::utils::sugg::Sugg; use crate::utils::{ get_item_name, get_parent_expr, higher, implements_trait, in_constant, is_integer_const, iter_input_pats, last_path_segment, match_qpath, match_trait_method, paths, snippet, snippet_opt, span_lint, span_lint_and_sugg, - span_lint_and_then, span_lint_hir_and_then, walk_ptrs_ty, SpanlessEq, + span_lint_and_then, span_lint_hir_and_then, SpanlessEq, }; declare_clippy_lint! { @@ -99,11 +99,11 @@ declare_clippy_lint! { /// if y != x {} // where both are floats /// /// // Good - /// let error = f64::EPSILON; // Use an epsilon for comparison + /// let error_margin = f64::EPSILON; // Use an epsilon for comparison /// // Or, if Rust <= 1.42, use `std::f64::EPSILON` constant instead. - /// // let error = std::f64::EPSILON; - /// if (y - 1.23f64).abs() < error { } - /// if (y - x).abs() > error { } + /// // let error_margin = std::f64::EPSILON; + /// if (y - 1.23f64).abs() < error_margin { } + /// if (y - x).abs() > error_margin { } /// ``` pub FLOAT_CMP, correctness, @@ -242,10 +242,10 @@ declare_clippy_lint! { /// if x == ONE { } // where both are floats /// /// // Good - /// let error = f64::EPSILON; // Use an epsilon for comparison + /// let error_margin = f64::EPSILON; // Use an epsilon for comparison /// // Or, if Rust <= 1.42, use `std::f64::EPSILON` constant instead. - /// // let error = std::f64::EPSILON; - /// if (x - ONE).abs() < error { } + /// // let error_margin = std::f64::EPSILON; + /// if (x - ONE).abs() < error_margin { } /// ``` pub FLOAT_CMP_CONST, restriction, @@ -411,16 +411,16 @@ impl<'tcx> LateLintPass<'tcx> for MiscLints { if !is_comparing_arrays { diag.span_suggestion( expr.span, - "consider comparing them within some error", + "consider comparing them within some margin of error", format!( - "({}).abs() {} error", + "({}).abs() {} error_margin", lhs - rhs, if op == BinOpKind::Eq { '<' } else { '>' } ), Applicability::HasPlaceholders, // snippet ); } - diag.note("`f32::EPSILON` and `f64::EPSILON` are available for the `error`"); + diag.note("`f32::EPSILON` and `f64::EPSILON` are available for the `error_margin`"); }); } else if op == BinOpKind::Rem && is_integer_const(cx, right, 1) { span_lint(cx, MODULO_ONE, expr.span, "any number modulo 1 will be 0"); @@ -561,7 +561,7 @@ fn is_signum(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { } fn is_float(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - let value = &walk_ptrs_ty(cx.typeck_results().expr_ty(expr)).kind(); + let value = &cx.typeck_results().expr_ty(expr).peel_refs().kind(); if let ty::Array(arr_ty, _) = value { return matches!(arr_ty.kind(), ty::Float(_)); @@ -571,7 +571,7 @@ fn is_float(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { } fn is_array(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - matches!(&walk_ptrs_ty(cx.typeck_results().expr_ty(expr)).kind(), ty::Array(_, _)) + matches!(&cx.typeck_results().expr_ty(expr).peel_refs().kind(), ty::Array(_, _)) } fn check_to_owned(cx: &LateContext<'_>, expr: &Expr<'_>, other: &Expr<'_>, left: bool) { diff --git a/clippy_lints/src/misc_early.rs b/clippy_lints/src/misc_early.rs index 02789735c17..9cb1cfb915d 100644 --- a/clippy_lints/src/misc_early.rs +++ b/clippy_lints/src/misc_early.rs @@ -377,8 +377,8 @@ impl EarlyLintPass for MiscEarlyLints { if let PatKind::Ident(_, ident, None) = arg.pat.kind { let arg_name = ident.to_string(); - if arg_name.starts_with('_') { - if let Some(correspondence) = registered_names.get(&arg_name[1..]) { + if let Some(arg_name) = arg_name.strip_prefix('_') { + if let Some(correspondence) = registered_names.get(arg_name) { span_lint( cx, DUPLICATE_UNDERSCORE_ARGUMENT, @@ -386,7 +386,7 @@ impl EarlyLintPass for MiscEarlyLints { &format!( "`{}` already exists, having another argument having almost the same \ name makes code comprehension and documentation more difficult", - arg_name[1..].to_owned() + arg_name ), ); } diff --git a/clippy_lints/src/mut_key.rs b/clippy_lints/src/mut_key.rs index 7423107e8f9..8a2dbdc50ea 100644 --- a/clippy_lints/src/mut_key.rs +++ b/clippy_lints/src/mut_key.rs @@ -1,4 +1,4 @@ -use crate::utils::{match_def_path, paths, span_lint, trait_ref_of_method, walk_ptrs_ty}; +use crate::utils::{match_def_path, paths, span_lint, trait_ref_of_method}; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{Adt, Array, RawPtr, Ref, Slice, Tuple, Ty, TypeAndMut}; @@ -12,8 +12,10 @@ declare_clippy_lint! { /// `BtreeSet` rely on either the hash or the order of keys be unchanging, /// so having types with interior mutability is a bad idea. /// - /// **Known problems:** We don't currently account for `Rc` or `Arc`, so - /// this may yield false positives. + /// **Known problems:** It's correct to use a struct, that contains interior mutability + /// as a key, when its `Hash` implementation doesn't access any of the interior mutable types. + /// However, this lint is unable to recognize this, so it causes a false positive in theses cases. + /// The `bytes` crate is a great example of this. /// /// **Example:** /// ```rust @@ -96,7 +98,7 @@ fn check_sig<'tcx>(cx: &LateContext<'tcx>, item_hir_id: hir::HirId, decl: &hir:: // We want to lint 1. sets or maps with 2. not immutable key types and 3. no unerased // generics (because the compiler cannot ensure immutability for unknown types). fn check_ty<'tcx>(cx: &LateContext<'tcx>, span: Span, ty: Ty<'tcx>) { - let ty = walk_ptrs_ty(ty); + let ty = ty.peel_refs(); if let Adt(def, substs) = ty.kind() { if [&paths::HASHMAP, &paths::BTREEMAP, &paths::HASHSET, &paths::BTREESET] .iter() diff --git a/clippy_lints/src/non_copy_const.rs b/clippy_lints/src/non_copy_const.rs index 73eabd4207e..bb44eeb6adc 100644 --- a/clippy_lints/src/non_copy_const.rs +++ b/clippy_lints/src/non_copy_const.rs @@ -6,14 +6,16 @@ use std::ptr; use rustc_hir::def::{DefKind, Res}; use rustc_hir::{Expr, ExprKind, ImplItem, ImplItemKind, Item, ItemKind, Node, TraitItem, TraitItemKind, UnOp}; +use rustc_infer::traits::specialization_graph; use rustc_lint::{LateContext, LateLintPass, Lint}; use rustc_middle::ty::adjustment::Adjust; -use rustc_middle::ty::{Ty, TypeFlags}; +use rustc_middle::ty::{AssocKind, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::{InnerSpan, Span, DUMMY_SP}; use rustc_typeck::hir_ty_to_ty; -use crate::utils::{in_constant, is_copy, qpath_res, span_lint_and_then}; +use crate::utils::{in_constant, qpath_res, span_lint_and_then}; +use if_chain::if_chain; declare_clippy_lint! { /// **What it does:** Checks for declaration of `const` items which is interior @@ -83,11 +85,10 @@ declare_clippy_lint! { "referencing `const` with interior mutability" } -#[allow(dead_code)] #[derive(Copy, Clone)] enum Source { Item { item: Span }, - Assoc { item: Span, ty: Span }, + Assoc { item: Span }, Expr { expr: Span }, } @@ -110,10 +111,15 @@ impl Source { } fn verify_ty_bound<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, source: Source) { - if ty.is_freeze(cx.tcx.at(DUMMY_SP), cx.param_env) || is_copy(cx, ty) { - // An `UnsafeCell` is `!Copy`, and an `UnsafeCell` is also the only type which - // is `!Freeze`, thus if our type is `Copy` we can be sure it must be `Freeze` - // as well. + // Ignore types whose layout is unknown since `is_freeze` reports every generic types as `!Freeze`, + // making it indistinguishable from `UnsafeCell`. i.e. it isn't a tool to prove a type is + // 'unfrozen'. However, this code causes a false negative in which + // a type contains a layout-unknown type, but also a unsafe cell like `const CELL: Cell<T>`. + // Yet, it's better than `ty.has_type_flags(TypeFlags::HAS_TY_PARAM | TypeFlags::HAS_PROJECTION)` + // since it works when a pointer indirection involves (`Cell<*const T>`). + // Making up a `ParamEnv` where every generic params and assoc types are `Freeze`is another option; + // but I'm not sure whether it's a decent way, if possible. + if cx.tcx.layout_of(cx.param_env.and(ty)).is_err() || ty.is_freeze(cx.tcx.at(DUMMY_SP), cx.param_env) { return; } @@ -127,11 +133,7 @@ fn verify_ty_bound<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, source: Source) { let const_kw_span = span.from_inner(InnerSpan::new(0, 5)); diag.span_label(const_kw_span, "make this a static item (maybe with lazy_static)"); }, - Source::Assoc { ty: ty_span, .. } => { - if ty.flags().intersects(TypeFlags::HAS_FREE_LOCAL_NAMES) { - diag.span_label(ty_span, &format!("consider requiring `{}` to be `Copy`", ty)); - } - }, + Source::Assoc { .. } => (), Source::Expr { .. } => { diag.help("assign this const to a local or static variable, and use the variable here"); }, @@ -152,14 +154,10 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst { fn check_trait_item(&mut self, cx: &LateContext<'tcx>, trait_item: &'tcx TraitItem<'_>) { if let TraitItemKind::Const(hir_ty, ..) = &trait_item.kind { let ty = hir_ty_to_ty(cx.tcx, hir_ty); - verify_ty_bound( - cx, - ty, - Source::Assoc { - ty: hir_ty.span, - item: trait_item.span, - }, - ); + // Normalize assoc types because ones originated from generic params + // bounded other traits could have their bound. + let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty); + verify_ty_bound(cx, normalized, Source::Assoc { item: trait_item.span }); } } @@ -167,17 +165,50 @@ impl<'tcx> LateLintPass<'tcx> for NonCopyConst { if let ImplItemKind::Const(hir_ty, ..) = &impl_item.kind { let item_hir_id = cx.tcx.hir().get_parent_node(impl_item.hir_id); let item = cx.tcx.hir().expect_item(item_hir_id); - // Ensure the impl is an inherent impl. - if let ItemKind::Impl { of_trait: None, .. } = item.kind { - let ty = hir_ty_to_ty(cx.tcx, hir_ty); - verify_ty_bound( - cx, - ty, - Source::Assoc { - ty: hir_ty.span, - item: impl_item.span, - }, - ); + + match &item.kind { + ItemKind::Impl { + of_trait: Some(of_trait_ref), + .. + } => { + if_chain! { + // Lint a trait impl item only when the definition is a generic type, + // assuming a assoc const is not meant to be a interior mutable type. + if let Some(of_trait_def_id) = of_trait_ref.trait_def_id(); + if let Some(of_assoc_item) = specialization_graph::Node::Trait(of_trait_def_id) + .item(cx.tcx, impl_item.ident, AssocKind::Const, of_trait_def_id); + if cx + .tcx + .layout_of(cx.tcx.param_env(of_trait_def_id).and( + // Normalize assoc types because ones originated from generic params + // bounded other traits could have their bound at the trait defs; + // and, in that case, the definition is *not* generic. + cx.tcx.normalize_erasing_regions( + cx.tcx.param_env(of_trait_def_id), + cx.tcx.type_of(of_assoc_item.def_id), + ), + )) + .is_err(); + then { + let ty = hir_ty_to_ty(cx.tcx, hir_ty); + let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty); + verify_ty_bound( + cx, + normalized, + Source::Assoc { + item: impl_item.span, + }, + ); + } + } + }, + ItemKind::Impl { of_trait: None, .. } => { + let ty = hir_ty_to_ty(cx.tcx, hir_ty); + // Normalize assoc types originated from generic params. + let normalized = cx.tcx.normalize_erasing_regions(cx.param_env, ty); + verify_ty_bound(cx, normalized, Source::Assoc { item: impl_item.span }); + }, + _ => (), } } } diff --git a/clippy_lints/src/open_options.rs b/clippy_lints/src/open_options.rs index e99d0317ba2..73a99a3a2f8 100644 --- a/clippy_lints/src/open_options.rs +++ b/clippy_lints/src/open_options.rs @@ -1,4 +1,4 @@ -use crate::utils::{match_type, paths, span_lint, walk_ptrs_ty}; +use crate::utils::{match_type, paths, span_lint}; use rustc_ast::ast::LitKind; use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; @@ -30,7 +30,7 @@ declare_lint_pass!(OpenOptions => [NONSENSICAL_OPEN_OPTIONS]); impl<'tcx> LateLintPass<'tcx> for OpenOptions { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { if let ExprKind::MethodCall(ref path, _, ref arguments, _) = e.kind { - let obj_ty = walk_ptrs_ty(cx.typeck_results().expr_ty(&arguments[0])); + let obj_ty = cx.typeck_results().expr_ty(&arguments[0]).peel_refs(); if path.ident.name == sym!(open) && match_type(cx, obj_ty, &paths::OPEN_OPTIONS) { let mut options = Vec::new(); get_open_options(cx, &arguments[0], &mut options); @@ -58,7 +58,7 @@ enum OpenOption { fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec<(OpenOption, Argument)>) { if let ExprKind::MethodCall(ref path, _, ref arguments, _) = argument.kind { - let obj_ty = walk_ptrs_ty(cx.typeck_results().expr_ty(&arguments[0])); + let obj_ty = cx.typeck_results().expr_ty(&arguments[0]).peel_refs(); // Only proceed if this is a call on some object of type std::fs::OpenOptions if match_type(cx, obj_ty, &paths::OPEN_OPTIONS) && arguments.len() >= 2 { diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index 9494efe736c..4a3eb9c983a 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -1,6 +1,7 @@ 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 crate::utils::{is_type_diagnostic_item, paths, span_lint_and_sugg}; use if_chain::if_chain; use rustc_errors::Applicability; @@ -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. @@ -73,7 +68,7 @@ declare_lint_pass!(OptionIfLetElse => [OPTION_IF_LET_ELSE]); 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.typeck_results().expr_ty(&receiver), &paths::RESULT) + && is_type_diagnostic_item(cx, &cx.typeck_results().expr_ty(&receiver), sym!(result_type)) } else { false } @@ -92,6 +87,7 @@ struct OptionIfLetElseOccurence { struct ReturnBreakContinueMacroVisitor { seen_return_break_continue: bool, } + impl ReturnBreakContinueMacroVisitor { fn new() -> ReturnBreakContinueMacroVisitor { ReturnBreakContinueMacroVisitor { @@ -99,6 +95,7 @@ impl ReturnBreakContinueMacroVisitor { } } } + impl<'tcx> Visitor<'tcx> for ReturnBreakContinueMacroVisitor { type Map = Map<'tcx>; fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { @@ -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 { @@ -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; @@ -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 { @@ -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, diff --git a/clippy_lints/src/panic_in_result_fn.rs b/clippy_lints/src/panic_in_result_fn.rs new file mode 100644 index 00000000000..4077aba6ef1 --- /dev/null +++ b/clippy_lints/src/panic_in_result_fn.rs @@ -0,0 +1,90 @@ +use crate::utils::{is_expn_of, is_type_diagnostic_item, return_ty, span_lint_and_then}; +use rustc_hir as hir; +use rustc_hir::intravisit::{self, FnKind, NestedVisitorMap, Visitor}; +use rustc_hir::Expr; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::hir::map::Map; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::Span; + +declare_clippy_lint! { + /// **What it does:** Checks for usage of `panic!`, `unimplemented!`, `todo!` or `unreachable!` in a function of type result. + /// + /// **Why is this bad?** For some codebases, it is desirable for functions of type result to return an error instead of crashing. Hence unimplemented, panic and unreachable should be avoided. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// fn result_with_panic() -> Result<bool, String> + /// { + /// panic!("error"); + /// } + /// ``` + pub PANIC_IN_RESULT_FN, + restriction, + "functions of type `Result<..>` that contain `panic!()`, `todo!()` or `unreachable()` or `unimplemented()` " +} + +declare_lint_pass!(PanicInResultFn => [PANIC_IN_RESULT_FN]); + +impl<'tcx> LateLintPass<'tcx> for PanicInResultFn { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + fn_kind: FnKind<'tcx>, + _: &'tcx hir::FnDecl<'tcx>, + body: &'tcx hir::Body<'tcx>, + span: Span, + hir_id: hir::HirId, + ) { + if !matches!(fn_kind, FnKind::Closure(_)) + && is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym!(result_type)) + { + lint_impl_body(cx, span, body); + } + } +} + +struct FindPanicUnimplementedUnreachable { + result: Vec<Span>, +} + +impl<'tcx> Visitor<'tcx> for FindPanicUnimplementedUnreachable { + type Map = Map<'tcx>; + + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if ["unimplemented", "unreachable", "panic", "todo"] + .iter() + .any(|fun| is_expn_of(expr.span, fun).is_some()) + { + self.result.push(expr.span); + } + // and check sub-expressions + intravisit::walk_expr(self, expr); + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { + NestedVisitorMap::None + } +} + +fn lint_impl_body<'tcx>(cx: &LateContext<'tcx>, impl_span: Span, body: &'tcx hir::Body<'tcx>) { + let mut panics = FindPanicUnimplementedUnreachable { result: Vec::new() }; + panics.visit_expr(&body.value); + if !panics.result.is_empty() { + span_lint_and_then( + cx, + PANIC_IN_RESULT_FN, + impl_span, + "used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`", + move |diag| { + diag.help( + "`unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing", + ); + diag.span_note(panics.result, "return Err() instead of panicking"); + }, + ); + } +} diff --git a/clippy_lints/src/path_buf_push_overwrite.rs b/clippy_lints/src/path_buf_push_overwrite.rs index b8583402928..6eeb031d383 100644 --- a/clippy_lints/src/path_buf_push_overwrite.rs +++ b/clippy_lints/src/path_buf_push_overwrite.rs @@ -1,4 +1,4 @@ -use crate::utils::{match_type, paths, span_lint_and_sugg, walk_ptrs_ty}; +use crate::utils::{match_type, paths, span_lint_and_sugg}; use if_chain::if_chain; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; @@ -46,7 +46,7 @@ impl<'tcx> LateLintPass<'tcx> for PathBufPushOverwrite { if let ExprKind::MethodCall(ref path, _, ref args, _) = expr.kind; if path.ident.name == sym!(push); if args.len() == 2; - if match_type(cx, walk_ptrs_ty(cx.typeck_results().expr_ty(&args[0])), &paths::PATH_BUF); + if match_type(cx, cx.typeck_results().expr_ty(&args[0]).peel_refs(), &paths::PATH_BUF); if let Some(get_index_arg) = args.get(1); if let ExprKind::Lit(ref lit) = get_index_arg.kind; if let LitKind::Str(ref path_lit, _) = lit.node; diff --git a/clippy_lints/src/redundant_clone.rs b/clippy_lints/src/redundant_clone.rs index 1615f192370..4d20a819804 100644 --- a/clippy_lints/src/redundant_clone.rs +++ b/clippy_lints/src/redundant_clone.rs @@ -240,10 +240,9 @@ impl<'tcx> LateLintPass<'tcx> for RedundantClone { ); let mut app = Applicability::MaybeIncorrect; - let mut call_snip = &snip[dot + 1..]; + let call_snip = &snip[dot + 1..]; // Machine applicable when `call_snip` looks like `foobar()` - if call_snip.ends_with("()") { - call_snip = call_snip[..call_snip.len()-2].trim(); + if let Some(call_snip) = call_snip.strip_suffix("()").map(str::trim) { if call_snip.as_bytes().iter().all(|b| b.is_ascii_alphabetic() || *b == b'_') { app = Applicability::MachineApplicable; } diff --git a/clippy_lints/src/repeat_once.rs b/clippy_lints/src/repeat_once.rs index c0890018d46..ae601353009 100644 --- a/clippy_lints/src/repeat_once.rs +++ b/clippy_lints/src/repeat_once.rs @@ -1,5 +1,5 @@ use crate::consts::{constant_context, Constant}; -use crate::utils::{in_macro, is_type_diagnostic_item, snippet, span_lint_and_sugg, walk_ptrs_ty}; +use crate::utils::{in_macro, is_type_diagnostic_item, snippet, span_lint_and_sugg}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::{Expr, ExprKind}; @@ -44,7 +44,7 @@ impl<'tcx> LateLintPass<'tcx> for RepeatOnce { if let Some(Constant::Int(1)) = constant_context(cx, cx.typeck_results()).expr(&count); if !in_macro(receiver.span); then { - let ty = walk_ptrs_ty(cx.typeck_results().expr_ty(&receiver)); + let ty = cx.typeck_results().expr_ty(&receiver).peel_refs(); if ty.is_str() { span_lint_and_sugg( cx, diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs index 087d50c90e6..225fe58906f 100644 --- a/clippy_lints/src/shadow.rs +++ b/clippy_lints/src/shadow.rs @@ -25,7 +25,6 @@ declare_clippy_lint! { /// **Example:** /// ```rust /// # let x = 1; - /// /// // Bad /// let x = &x; /// @@ -75,7 +74,9 @@ declare_clippy_lint! { /// names to bindings or introducing more scopes to contain the bindings. /// /// **Known problems:** This lint, as the other shadowing related lints, - /// currently only catches very simple patterns. + /// currently only catches very simple patterns. Note that + /// `allow`/`warn`/`deny`/`forbid` attributes only work on the function level + /// for this lint. /// /// **Example:** /// ```rust diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index 7a659bf779c..15b66684eab 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -8,7 +8,7 @@ use rustc_span::source_map::Spanned; use if_chain::if_chain; use crate::utils::SpanlessEq; -use crate::utils::{get_parent_expr, is_allowed, is_type_diagnostic_item, span_lint, span_lint_and_sugg, walk_ptrs_ty}; +use crate::utils::{get_parent_expr, is_allowed, is_type_diagnostic_item, span_lint, span_lint_and_sugg}; declare_clippy_lint! { /// **What it does:** Checks for string appends of the form `x = x + y` (without @@ -134,7 +134,7 @@ impl<'tcx> LateLintPass<'tcx> for StringAdd { } fn is_string(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { - is_type_diagnostic_item(cx, walk_ptrs_ty(cx.typeck_results().expr_ty(e)), sym!(string_type)) + is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(e).peel_refs(), sym!(string_type)) } fn is_add(cx: &LateContext<'_>, src: &Expr<'_>, target: &Expr<'_>) -> bool { diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index 47a73ca9a24..54b38d9f4ce 100644 --- a/clippy_lints/src/swap.rs +++ b/clippy_lints/src/swap.rs @@ -1,7 +1,6 @@ use crate::utils::sugg::Sugg; use crate::utils::{ differing_macro_contexts, eq_expr_value, is_type_diagnostic_item, snippet_with_applicability, span_lint_and_then, - walk_ptrs_ty, }; use if_chain::if_chain; use rustc_errors::Applicability; @@ -194,7 +193,7 @@ fn check_for_slice<'a>(cx: &LateContext<'_>, lhs1: &'a Expr<'_>, lhs2: &'a Expr< if let ExprKind::Index(ref lhs1, ref idx1) = lhs1.kind { if let ExprKind::Index(ref lhs2, ref idx2) = lhs2.kind { if eq_expr_value(cx, lhs1, lhs2) { - let ty = walk_ptrs_ty(cx.typeck_results().expr_ty(lhs1)); + let ty = cx.typeck_results().expr_ty(lhs1).peel_refs(); if matches!(ty.kind(), ty::Slice(_)) || matches!(ty.kind(), ty::Array(_, _)) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 6c6188d61ad..a29a199b8c3 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -215,11 +215,41 @@ declare_clippy_lint! { "redundant allocation" } +declare_clippy_lint! { + /// **What it does:** Checks for Rc<T> and Arc<T> when T is a mutable buffer type such as String or Vec + /// + /// **Why is this bad?** Expressions such as Rc<String> have no advantage over Rc<str>, since + /// it is larger and involves an extra level of indirection, and doesn't implement Borrow<str>. + /// + /// While mutating a buffer type would still be possible with Rc::get_mut(), it only + /// works if there are no additional references yet, which defeats the purpose of + /// enclosing it in a shared ownership type. Instead, additionally wrapping the inner + /// type with an interior mutable container (such as RefCell or Mutex) would normally + /// be used. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust,ignore + /// # use std::rc::Rc; + /// fn foo(interned: Rc<String>) { ... } + /// ``` + /// + /// Better: + /// + /// ```rust,ignore + /// fn foo(interned: Rc<str>) { ... } + /// ``` + pub RC_BUFFER, + perf, + "shared ownership of a buffer type" +} + pub struct Types { vec_box_size_threshold: u64, } -impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION]); +impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER]); impl<'tcx> LateLintPass<'tcx> for Types { fn check_fn(&mut self, cx: &LateContext<'_>, _: FnKind<'_>, decl: &FnDecl<'_>, _: &Body<'_>, _: Span, id: HirId) { @@ -272,6 +302,19 @@ fn match_type_parameter(cx: &LateContext<'_>, qpath: &QPath<'_>, path: &[&str]) None } +fn match_buffer_type(cx: &LateContext<'_>, qpath: &QPath<'_>) -> Option<&'static str> { + if match_type_parameter(cx, qpath, &paths::STRING).is_some() { + return Some("str"); + } + if match_type_parameter(cx, qpath, &paths::OS_STRING).is_some() { + return Some("std::ffi::OsStr"); + } + if match_type_parameter(cx, qpath, &paths::PATH_BUF).is_some() { + return Some("std::path::Path"); + } + None +} + fn match_borrows_parameter(_cx: &LateContext<'_>, qpath: &QPath<'_>) -> Option<Span> { let last = last_path_segment(qpath); if_chain! { @@ -321,14 +364,15 @@ impl Types { if let Some(def_id) = res.opt_def_id() { if Some(def_id) == cx.tcx.lang_items().owned_box() { if let Some(span) = match_borrows_parameter(cx, qpath) { + let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, REDUNDANT_ALLOCATION, hir_ty.span, "usage of `Box<&T>`", "try", - snippet(cx, span, "..").to_string(), - Applicability::MachineApplicable, + snippet_with_applicability(cx, span, "..", &mut applicability).to_string(), + applicability, ); return; // don't recurse into the type } @@ -345,14 +389,15 @@ impl Types { } } else if cx.tcx.is_diagnostic_item(sym::Rc, def_id) { if let Some(span) = match_type_parameter(cx, qpath, &paths::RC) { + let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, REDUNDANT_ALLOCATION, hir_ty.span, "usage of `Rc<Rc<T>>`", "try", - snippet(cx, span, "..").to_string(), - Applicability::MachineApplicable, + snippet_with_applicability(cx, span, "..", &mut applicability).to_string(), + applicability, ); return; // don't recurse into the type } @@ -368,25 +413,109 @@ impl Types { GenericArg::Type(ty) => ty.span, _ => return, }; + let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, REDUNDANT_ALLOCATION, hir_ty.span, "usage of `Rc<Box<T>>`", "try", - format!("Rc<{}>", snippet(cx, inner_span, "..")), + format!( + "Rc<{}>", + snippet_with_applicability(cx, inner_span, "..", &mut applicability) + ), + applicability, + ); + return; // don't recurse into the type + } + if let Some(alternate) = match_buffer_type(cx, qpath) { + span_lint_and_sugg( + cx, + RC_BUFFER, + hir_ty.span, + "usage of `Rc<T>` when T is a buffer type", + "try", + format!("Rc<{}>", alternate), + Applicability::MachineApplicable, + ); + return; // don't recurse into the type + } + if match_type_parameter(cx, qpath, &paths::VEC).is_some() { + let vec_ty = match &last_path_segment(qpath).args.unwrap().args[0] { + GenericArg::Type(ty) => match &ty.kind { + TyKind::Path(qpath) => qpath, + _ => return, + }, + _ => return, + }; + let inner_span = match &last_path_segment(&vec_ty).args.unwrap().args[0] { + GenericArg::Type(ty) => ty.span, + _ => return, + }; + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + RC_BUFFER, + hir_ty.span, + "usage of `Rc<T>` when T is a buffer type", + "try", + format!( + "Rc<[{}]>", + snippet_with_applicability(cx, inner_span, "..", &mut applicability) + ), Applicability::MachineApplicable, ); return; // don't recurse into the type } if let Some(span) = match_borrows_parameter(cx, qpath) { + let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, REDUNDANT_ALLOCATION, hir_ty.span, "usage of `Rc<&T>`", "try", - snippet(cx, span, "..").to_string(), + snippet_with_applicability(cx, span, "..", &mut applicability).to_string(), + applicability, + ); + return; // don't recurse into the type + } + } else if cx.tcx.is_diagnostic_item(sym::Arc, def_id) { + if let Some(alternate) = match_buffer_type(cx, qpath) { + span_lint_and_sugg( + cx, + RC_BUFFER, + hir_ty.span, + "usage of `Arc<T>` when T is a buffer type", + "try", + format!("Arc<{}>", alternate), + Applicability::MachineApplicable, + ); + return; // don't recurse into the type + } + if match_type_parameter(cx, qpath, &paths::VEC).is_some() { + let vec_ty = match &last_path_segment(qpath).args.unwrap().args[0] { + GenericArg::Type(ty) => match &ty.kind { + TyKind::Path(qpath) => qpath, + _ => return, + }, + _ => return, + }; + let inner_span = match &last_path_segment(&vec_ty).args.unwrap().args[0] { + GenericArg::Type(ty) => ty.span, + _ => return, + }; + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + RC_BUFFER, + hir_ty.span, + "usage of `Arc<T>` when T is a buffer type", + "try", + format!( + "Arc<[{}]>", + snippet_with_applicability(cx, inner_span, "..", &mut applicability) + ), Applicability::MachineApplicable, ); return; // don't recurse into the type @@ -546,7 +675,6 @@ impl Types { // details. return; } - let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, BORROWED_BOX, @@ -556,8 +684,12 @@ impl Types { format!( "&{}{}", ltopt, - &snippet_with_applicability(cx, inner.span, "..", &mut applicability) + &snippet(cx, inner.span, "..") ), + // To make this `MachineApplicable`, at least one needs to check if it isn't a trait item + // because the trait impls of it will break otherwise; + // and there may be other cases that result in invalid code. + // For example, type coercion doesn't work nicely. Applicability::Unspecified, ); return; // don't recurse into the type diff --git a/clippy_lints/src/unwrap_in_result.rs b/clippy_lints/src/unwrap_in_result.rs index 1c7e62ecd3d..0f8797243ec 100644 --- a/clippy_lints/src/unwrap_in_result.rs +++ b/clippy_lints/src/unwrap_in_result.rs @@ -1,4 +1,4 @@ -use crate::utils::{is_type_diagnostic_item, method_chain_args, return_ty, span_lint_and_then, walk_ptrs_ty}; +use crate::utils::{is_type_diagnostic_item, method_chain_args, return_ty, span_lint_and_then}; use if_chain::if_chain; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; @@ -81,7 +81,7 @@ impl<'a, 'tcx> Visitor<'tcx> for FindExpectUnwrap<'a, 'tcx> { fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { // check for `expect` if let Some(arglists) = method_chain_args(expr, &["expect"]) { - let reciever_ty = walk_ptrs_ty(self.typeck_results.expr_ty(&arglists[0][0])); + let reciever_ty = self.typeck_results.expr_ty(&arglists[0][0]).peel_refs(); if is_type_diagnostic_item(self.lcx, reciever_ty, sym!(option_type)) || is_type_diagnostic_item(self.lcx, reciever_ty, sym!(result_type)) { @@ -91,7 +91,7 @@ impl<'a, 'tcx> Visitor<'tcx> for FindExpectUnwrap<'a, 'tcx> { // check for `unwrap` if let Some(arglists) = method_chain_args(expr, &["unwrap"]) { - let reciever_ty = walk_ptrs_ty(self.typeck_results.expr_ty(&arglists[0][0])); + let reciever_ty = self.typeck_results.expr_ty(&arglists[0][0]).peel_refs(); if is_type_diagnostic_item(self.lcx, reciever_ty, sym!(option_type)) || is_type_diagnostic_item(self.lcx, reciever_ty, sym!(result_type)) { diff --git a/clippy_lints/src/useless_conversion.rs b/clippy_lints/src/useless_conversion.rs index 615440e15f3..4e4a206a583 100644 --- a/clippy_lints/src/useless_conversion.rs +++ b/clippy_lints/src/useless_conversion.rs @@ -72,7 +72,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { cx, USELESS_CONVERSION, e.span, - "useless conversion to the same type", + &format!("useless conversion to the same type: `{}`", b), "consider removing `.into()`", sugg, Applicability::MachineApplicable, // snippet @@ -95,7 +95,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { cx, USELESS_CONVERSION, e.span, - "useless conversion to the same type", + &format!("useless conversion to the same type: `{}`", b), "consider removing `.into_iter()`", sugg, Applicability::MachineApplicable, // snippet @@ -116,7 +116,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { cx, USELESS_CONVERSION, e.span, - "useless conversion to the same type", + &format!("useless conversion to the same type: `{}`", b), None, "consider removing `.try_into()`", ); @@ -147,7 +147,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { cx, USELESS_CONVERSION, e.span, - "useless conversion to the same type", + &format!("useless conversion to the same type: `{}`", b), None, &hint, ); @@ -166,7 +166,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { cx, USELESS_CONVERSION, e.span, - "useless conversion to the same type", + &format!("useless conversion to the same type: `{}`", b), &sugg_msg, sugg.to_string(), Applicability::MachineApplicable, // snippet diff --git a/clippy_lints/src/utils/diagnostics.rs b/clippy_lints/src/utils/diagnostics.rs index e4e65b5f4d4..0a58231558e 100644 --- a/clippy_lints/src/utils/diagnostics.rs +++ b/clippy_lints/src/utils/diagnostics.rs @@ -51,6 +51,8 @@ pub fn span_lint<T: LintContext>(cx: &T, lint: &'static Lint, sp: impl Into<Mult /// /// The `help` message can be optionally attached to a `Span`. /// +/// If you change the signature, remember to update the internal lint `CollapsibleCalls` +/// /// # Example /// /// ```ignore @@ -87,6 +89,8 @@ pub fn span_lint_and_help<'a, T: LintContext>( /// The `note` message is presented separately from the main lint message /// and is attached to a specific span: /// +/// If you change the signature, remember to update the internal lint `CollapsibleCalls` +/// /// # Example /// /// ```ignore @@ -126,6 +130,7 @@ pub fn span_lint_and_note<'a, T: LintContext>( /// Like `span_lint` but allows to add notes, help and suggestions using a closure. /// /// If you need to customize your lint output a lot, use this function. +/// If you change the signature, remember to update the internal lint `CollapsibleCalls` pub fn span_lint_and_then<'a, T: LintContext, F>(cx: &'a T, lint: &'static Lint, sp: Span, msg: &str, f: F) where F: for<'b> FnOnce(&mut DiagnosticBuilder<'b>), @@ -168,6 +173,10 @@ pub fn span_lint_hir_and_then( /// In the example below, `help` is `"try"` and `sugg` is the suggested replacement `".any(|x| x > /// 2)"`. /// +/// If you change the signature, remember to update the internal lint `CollapsibleCalls` +/// +/// # Example +/// /// ```ignore /// error: This `.fold` can be more succinctly expressed as `.any` /// --> $DIR/methods.rs:390:13 diff --git a/clippy_lints/src/utils/eager_or_lazy.rs b/clippy_lints/src/utils/eager_or_lazy.rs new file mode 100644 index 00000000000..6938d9971d9 --- /dev/null +++ b/clippy_lints/src/utils/eager_or_lazy.rs @@ -0,0 +1,128 @@ +//! Utilities for evaluating whether eagerly evaluated expressions can be made lazy and vice versa. +//! +//! Things to consider: +//! - has the expression side-effects? +//! - is the expression computationally expensive? +//! +//! See lints: +//! - unnecessary-lazy-evaluations +//! - or-fun-call +//! - option-if-let-else + +use crate::utils::is_ctor_or_promotable_const_function; +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_lint::LateContext; +use rustc_middle::hir::map::Map; + +/// 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::Path(..) | ExprKind::Field(..) => true, + ExprKind::AddrOf(_, _, addr_of_expr) => identify_some_pure_patterns(addr_of_expr), + ExprKind::Tup(tup_exprs) => tup_exprs.iter().all(|expr| identify_some_pure_patterns(expr)), + ExprKind::Struct(_, fields, expr) => { + fields.iter().all(|f| identify_some_pure_patterns(f.expr)) + && expr.map_or(true, |e| identify_some_pure_patterns(e)) + }, + ExprKind::Call( + &Expr { + kind: + ExprKind::Path(QPath::Resolved( + _, + Path { + res: Res::Def(DefKind::Ctor(..) | DefKind::Variant, ..), + .. + }, + )), + .. + }, + args, + ) => args.iter().all(|expr| identify_some_pure_patterns(expr)), + 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::Cast(..) + | ExprKind::Type(..) + | ExprKind::DropTemps(..) + | ExprKind::Loop(..) + | 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, + } +} + +/// 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<'a, 'tcx>(cx: &'a 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, + } + + 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::MethodCall(..) => true, + _ => false, + }; + + if call_found { + self.found |= true; + } + + if !self.found { + intravisit::walk_expr(self, expr); + } + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { + NestedVisitorMap::None + } + } + + let mut finder = FunCallFinder { cx, found: false }; + finder.visit_expr(expr); + finder.found +} + +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) +} + +pub fn is_lazyness_candidate<'a, 'tcx>(cx: &'a LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + identify_some_potentially_expensive_patterns(cx, expr) +} diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs index 8fa5d22210a..bfe426a25eb 100644 --- a/clippy_lints/src/utils/internal_lints.rs +++ b/clippy_lints/src/utils/internal_lints.rs @@ -1,6 +1,6 @@ use crate::utils::{ - is_expn_of, match_def_path, match_qpath, match_type, method_calls, paths, run_lints, snippet, span_lint, - span_lint_and_help, span_lint_and_sugg, walk_ptrs_ty, SpanlessEq, + is_expn_of, match_def_path, match_qpath, match_type, method_calls, path_to_res, paths, qpath_res, run_lints, + snippet, span_lint, span_lint_and_help, span_lint_and_sugg, SpanlessEq, }; use if_chain::if_chain; use rustc_ast::ast::{Crate as AstCrate, ItemKind, LitKind, NodeId}; @@ -11,7 +11,7 @@ use rustc_hir as hir; use rustc_hir::def::{DefKind, Res}; use rustc_hir::hir_id::CRATE_HIR_ID; use rustc_hir::intravisit::{NestedVisitorMap, Visitor}; -use rustc_hir::{Crate, Expr, ExprKind, HirId, Item, MutTy, Mutability, Path, StmtKind, Ty, TyKind}; +use rustc_hir::{Crate, Expr, ExprKind, HirId, Item, MutTy, Mutability, Node, Path, StmtKind, Ty, TyKind}; use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass}; use rustc_middle::hir::map::Map; use rustc_session::{declare_lint_pass, declare_tool_lint, impl_lint_pass}; @@ -206,6 +206,29 @@ declare_clippy_lint! { "found collapsible `span_lint_and_then` calls" } +declare_clippy_lint! { + /// **What it does:** Checks for calls to `utils::match_type()` on a type diagnostic item + /// and suggests to use `utils::is_type_diagnostic_item()` instead. + /// + /// **Why is this bad?** `utils::is_type_diagnostic_item()` does not require hardcoded paths. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// Bad: + /// ```rust,ignore + /// utils::match_type(cx, ty, &paths::VEC) + /// ``` + /// + /// Good: + /// ```rust,ignore + /// utils::is_type_diagnostic_item(cx, ty, sym!(vec_type)) + /// ``` + pub MATCH_TYPE_ON_DIAGNOSTIC_ITEM, + internal, + "using `utils::match_type()` instead of `utils::is_type_diagnostic_item()`" +} + declare_lint_pass!(ClippyLintsInternal => [CLIPPY_LINTS_INTERNAL]); impl EarlyLintPass for ClippyLintsInternal { @@ -404,7 +427,7 @@ impl<'tcx> LateLintPass<'tcx> for CompilerLintFunctions { if let ExprKind::MethodCall(ref path, _, ref args, _) = expr.kind; let fn_name = path.ident; if let Some(sugg) = self.map.get(&*fn_name.as_str()); - let ty = walk_ptrs_ty(cx.typeck_results().expr_ty(&args[0])); + let ty = cx.typeck_results().expr_ty(&args[0]).peel_refs(); if match_type(cx, ty, &paths::EARLY_CONTEXT) || match_type(cx, ty, &paths::LATE_CONTEXT); then { @@ -437,7 +460,7 @@ impl<'tcx> LateLintPass<'tcx> for OuterExpnDataPass { let args = arg_lists[1]; if args.len() == 1; let self_arg = &args[0]; - let self_ty = walk_ptrs_ty(cx.typeck_results().expr_ty(self_arg)); + let self_ty = cx.typeck_results().expr_ty(self_arg).peel_refs(); if match_type(cx, self_ty, &paths::SYNTAX_CONTEXT); then { span_lint_and_sugg( @@ -652,3 +675,89 @@ fn suggest_note( Applicability::MachineApplicable, ); } + +declare_lint_pass!(MatchTypeOnDiagItem => [MATCH_TYPE_ON_DIAGNOSTIC_ITEM]); + +impl<'tcx> LateLintPass<'tcx> for MatchTypeOnDiagItem { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { + if !run_lints(cx, &[MATCH_TYPE_ON_DIAGNOSTIC_ITEM], expr.hir_id) { + return; + } + + if_chain! { + // Check if this is a call to utils::match_type() + if let ExprKind::Call(fn_path, [context, ty, ty_path]) = expr.kind; + if let ExprKind::Path(fn_qpath) = &fn_path.kind; + if match_qpath(&fn_qpath, &["utils", "match_type"]); + // Extract the path to the matched type + if let Some(segments) = path_to_matched_type(cx, ty_path); + let segments: Vec<&str> = segments.iter().map(|sym| &**sym).collect(); + if let Some(ty_did) = path_to_res(cx, &segments[..]).and_then(|res| res.opt_def_id()); + // Check if the matched type is a diagnostic item + let diag_items = cx.tcx.diagnostic_items(ty_did.krate); + if let Some(item_name) = diag_items.iter().find_map(|(k, v)| if *v == ty_did { Some(k) } else { None }); + then { + let cx_snippet = snippet(cx, context.span, "_"); + let ty_snippet = snippet(cx, ty.span, "_"); + + span_lint_and_sugg( + cx, + MATCH_TYPE_ON_DIAGNOSTIC_ITEM, + expr.span, + "usage of `utils::match_type()` on a type diagnostic item", + "try", + format!("utils::is_type_diagnostic_item({}, {}, sym!({}))", cx_snippet, ty_snippet, item_name), + Applicability::MaybeIncorrect, + ); + } + } + } +} + +fn path_to_matched_type(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<Vec<SymbolStr>> { + use rustc_hir::ItemKind; + + match &expr.kind { + ExprKind::AddrOf(.., expr) => return path_to_matched_type(cx, expr), + ExprKind::Path(qpath) => match qpath_res(cx, qpath, expr.hir_id) { + Res::Local(hir_id) => { + let parent_id = cx.tcx.hir().get_parent_node(hir_id); + if let Some(Node::Local(local)) = cx.tcx.hir().find(parent_id) { + if let Some(init) = local.init { + return path_to_matched_type(cx, init); + } + } + }, + Res::Def(DefKind::Const | DefKind::Static, def_id) => { + if let Some(Node::Item(item)) = cx.tcx.hir().get_if_local(def_id) { + if let ItemKind::Const(.., body_id) | ItemKind::Static(.., body_id) = item.kind { + let body = cx.tcx.hir().body(body_id); + return path_to_matched_type(cx, &body.value); + } + } + }, + _ => {}, + }, + ExprKind::Array(exprs) => { + let segments: Vec<SymbolStr> = exprs + .iter() + .filter_map(|expr| { + if let ExprKind::Lit(lit) = &expr.kind { + if let LitKind::Str(sym, _) = lit.node { + return Some(sym.as_str()); + } + } + + None + }) + .collect(); + + if segments.len() == exprs.len() { + return Some(segments); + } + }, + _ => {}, + } + + None +} diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 3ebbfed6456..ea52741b7cc 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -10,6 +10,7 @@ pub mod comparisons; pub mod conf; pub mod constants; mod diagnostics; +pub mod eager_or_lazy; pub mod higher; mod hir_utils; pub mod inspector; @@ -130,6 +131,9 @@ pub fn is_wild<'tcx>(pat: &impl std::ops::Deref<Target = Pat<'tcx>>) -> bool { } /// Checks if type is struct, enum or union type with the given def path. +/// +/// If the type is a diagnostic item, use `is_type_diagnostic_item` instead. +/// If you change the signature, remember to update the internal lint `MatchTypeOnDiagItem` pub fn match_type(cx: &LateContext<'_>, ty: Ty<'_>, path: &[&str]) -> bool { match ty.kind() { ty::Adt(adt, _) => match_def_path(cx, adt.did, path), @@ -138,6 +142,8 @@ pub fn match_type(cx: &LateContext<'_>, ty: Ty<'_>, path: &[&str]) -> bool { } /// Checks if the type is equal to a diagnostic item +/// +/// If you change the signature, remember to update the internal lint `MatchTypeOnDiagItem` pub fn is_type_diagnostic_item(cx: &LateContext<'_>, ty: Ty<'_>, diag_item: Symbol) -> bool { match ty.kind() { ty::Adt(adt, _) => cx.tcx.is_diagnostic_item(diag_item, adt.did), @@ -748,14 +754,6 @@ pub fn walk_ptrs_hir_ty<'tcx>(ty: &'tcx hir::Ty<'tcx>) -> &'tcx hir::Ty<'tcx> { } } -/// Returns the base type for references and raw pointers. -pub fn walk_ptrs_ty(ty: Ty<'_>) -> Ty<'_> { - match ty.kind() { - ty::Ref(_, ty, _) => walk_ptrs_ty(ty), - _ => ty, - } -} - /// Returns the base type for references and raw pointers, and count reference /// depth. pub fn walk_ptrs_ty_depth(ty: Ty<'_>) -> (Ty<'_>, usize) { diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 3b031a552e5..1583afad208 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -113,8 +113,12 @@ pub const STD_CONVERT_IDENTITY: [&str; 3] = ["std", "convert", "identity"]; pub const STD_FS_CREATE_DIR: [&str; 3] = ["std", "fs", "create_dir"]; pub const STD_MEM_TRANSMUTE: [&str; 3] = ["std", "mem", "transmute"]; pub const STD_PTR_NULL: [&str; 3] = ["std", "ptr", "null"]; +pub const STRING: [&str; 3] = ["alloc", "string", "String"]; pub const STRING_AS_MUT_STR: [&str; 4] = ["alloc", "string", "String", "as_mut_str"]; pub const STRING_AS_STR: [&str; 4] = ["alloc", "string", "String", "as_str"]; +pub const STR_ENDS_WITH: [&str; 4] = ["core", "str", "<impl str>", "ends_with"]; +pub const STR_LEN: [&str; 4] = ["core", "str", "<impl str>", "len"]; +pub const STR_STARTS_WITH: [&str; 4] = ["core", "str", "<impl str>", "starts_with"]; pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"]; pub const TO_OWNED: [&str; 3] = ["alloc", "borrow", "ToOwned"]; pub const TO_OWNED_METHOD: [&str; 4] = ["alloc", "borrow", "ToOwned", "to_owned"]; diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index 811fde388d1..ec8b7e59b59 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -49,7 +49,7 @@ impl<'a> Sugg<'a> { /// Convenience function around `hir_opt` for suggestions with a default /// text. pub fn hir(cx: &LateContext<'_>, expr: &hir::Expr<'_>, default: &'a str) -> Self { - Self::hir_opt(cx, expr).unwrap_or_else(|| Sugg::NonParen(Cow::Borrowed(default))) + Self::hir_opt(cx, expr).unwrap_or(Sugg::NonParen(Cow::Borrowed(default))) } /// Same as `hir`, but it adapts the applicability level by following rules: diff --git a/clippy_lints/src/utils/usage.rs b/clippy_lints/src/utils/usage.rs index 4a64b935ac9..ea1dc3be29b 100644 --- a/clippy_lints/src/utils/usage.rs +++ b/clippy_lints/src/utils/usage.rs @@ -1,6 +1,8 @@ use crate::utils::match_var; use rustc_data_structures::fx::FxHashSet; +use rustc_hir as hir; use rustc_hir::def::Res; +use rustc_hir::intravisit; use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; use rustc_hir::{Expr, HirId, Path}; use rustc_infer::infer::TyCtxtInferExt; @@ -108,3 +110,67 @@ pub fn is_unused<'tcx>(ident: &'tcx Ident, body: &'tcx Expr<'_>) -> bool { walk_expr(&mut visitor, body); !visitor.used } + +pub struct ParamBindingIdCollector { + binding_hir_ids: Vec<hir::HirId>, +} +impl<'tcx> ParamBindingIdCollector { + fn collect_binding_hir_ids(body: &'tcx hir::Body<'tcx>) -> Vec<hir::HirId> { + let mut finder = ParamBindingIdCollector { + binding_hir_ids: Vec::new(), + }; + finder.visit_body(body); + finder.binding_hir_ids + } +} +impl<'tcx> intravisit::Visitor<'tcx> for ParamBindingIdCollector { + type Map = Map<'tcx>; + + fn visit_param(&mut self, param: &'tcx hir::Param<'tcx>) { + if let hir::PatKind::Binding(_, hir_id, ..) = param.pat.kind { + self.binding_hir_ids.push(hir_id); + } + } + + fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> { + intravisit::NestedVisitorMap::None + } +} + +pub struct BindingUsageFinder<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + binding_ids: Vec<hir::HirId>, + usage_found: bool, +} +impl<'a, 'tcx> BindingUsageFinder<'a, 'tcx> { + pub fn are_params_used(cx: &'a LateContext<'tcx>, body: &'tcx hir::Body<'tcx>) -> bool { + let mut finder = BindingUsageFinder { + cx, + binding_ids: ParamBindingIdCollector::collect_binding_hir_ids(body), + usage_found: false, + }; + finder.visit_body(body); + finder.usage_found + } +} +impl<'a, 'tcx> intravisit::Visitor<'tcx> for BindingUsageFinder<'a, 'tcx> { + type Map = Map<'tcx>; + + fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) { + if !self.usage_found { + intravisit::walk_expr(self, expr); + } + } + + fn visit_path(&mut self, path: &'tcx hir::Path<'tcx>, _: hir::HirId) { + if let hir::def::Res::Local(id) = path.res { + if self.binding_ids.contains(&id) { + self.usage_found = true; + } + } + } + + fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> { + intravisit::NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) + } +} diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index e653240d049..fac63bcb993 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -322,11 +322,15 @@ impl EarlyLintPass for Write { } /// Given a format string that ends in a newline and its span, calculates the span of the -/// newline. +/// newline, or the format string itself if the format string consists solely of a newline. fn newline_span(fmtstr: &StrLit) -> Span { let sp = fmtstr.span; let contents = &fmtstr.symbol.as_str(); + if *contents == r"\n" { + return sp; + } + let newline_sp_hi = sp.hi() - match fmtstr.style { StrStyle::Cooked => BytePos(1), |