From d1b087fdee1024e11498e2d6c3e36d31e9371d3b Mon Sep 17 00:00:00 2001 From: flip1995 Date: Mon, 14 Mar 2022 12:02:53 +0100 Subject: [PATCH] Merge commit 'dc5423ad448877e33cca28db2f1445c9c4473c75' into clippyup --- .github/workflows/clippy.yml | 7 - .github/workflows/clippy_bors.yml | 7 - .github/workflows/clippy_dev.yml | 7 + CHANGELOG.md | 7 + Cargo.toml | 2 +- clippy_lints/src/attrs.rs | 60 +- .../src/casts/cast_slice_different_sizes.rs | 117 +++ clippy_lints/src/casts/mod.rs | 48 ++ clippy_lints/src/dbg_macro.rs | 5 + clippy_lints/src/lib.register_all.rs | 6 + clippy_lints/src/lib.register_complexity.rs | 3 + clippy_lints/src/lib.register_correctness.rs | 1 + clippy_lints/src/lib.register_internal.rs | 1 + clippy_lints/src/lib.register_lints.rs | 9 + clippy_lints/src/lib.register_perf.rs | 2 + clippy_lints/src/lib.register_restriction.rs | 1 + clippy_lints/src/lib.rs | 3 + clippy_lints/src/loops/missing_spin_loop.rs | 56 ++ clippy_lints/src/loops/mod.rs | 39 + clippy_lints/src/matches/mod.rs | 47 ++ clippy_lints/src/matches/needless_match.rs | 197 ++++++ clippy_lints/src/methods/iter_with_drain.rs | 72 ++ clippy_lints/src/methods/mod.rs | 68 +- .../src/methods/option_map_or_none.rs | 9 +- .../src/methods/unnecessary_filter_map.rs | 26 +- .../src/methods/unnecessary_iter_cloned.rs | 91 +-- clippy_lints/src/methods/utils.rs | 87 +++ clippy_lints/src/needless_pass_by_value.rs | 7 +- clippy_lints/src/only_used_in_recursion.rs | 668 ++++++++++++++++++ clippy_lints/src/question_mark.rs | 6 +- clippy_lints/src/redundant_clone.rs | 59 +- .../src/suspicious_operation_groupings.rs | 8 +- clippy_lints/src/transmute/mod.rs | 4 + clippy_lints/src/unnecessary_sort_by.rs | 47 +- clippy_lints/src/use_self.rs | 19 +- clippy_lints/src/utils/internal_lints.rs | 54 +- .../internal_lints/metadata_collector.rs | 6 +- clippy_lints/src/write.rs | 6 +- clippy_utils/src/ast_utils.rs | 54 +- clippy_utils/src/consts.rs | 9 +- clippy_utils/src/hir_utils.rs | 6 +- clippy_utils/src/msrvs.rs | 2 +- clippy_utils/src/paths.rs | 6 + clippy_utils/src/ty.rs | 6 +- rust-toolchain | 2 +- rustfmt.toml | 2 +- tests/compile-test.rs | 9 +- .../ui-internal/invalid_msrv_attr_impl.fixed | 40 ++ tests/ui-internal/invalid_msrv_attr_impl.rs | 38 + .../ui-internal/invalid_msrv_attr_impl.stderr | 32 + tests/ui/allow_attributes_without_reason.rs | 14 + .../ui/allow_attributes_without_reason.stderr | 23 + tests/ui/cast_slice_different_sizes.rs | 39 + tests/ui/cast_slice_different_sizes.stderr | 52 ++ .../no_std_main_recursion.rs | 4 +- tests/ui/dbg_macro.rs | 6 + tests/ui/dbg_macro.stderr | 20 +- tests/ui/extend_with_drain.fixed | 4 +- tests/ui/extend_with_drain.rs | 4 +- tests/ui/iter_with_drain.fixed | 56 ++ tests/ui/iter_with_drain.rs | 56 ++ tests/ui/iter_with_drain.stderr | 40 ++ tests/ui/manual_map_option.fixed | 1 + tests/ui/manual_map_option.rs | 1 + tests/ui/missing_spin_loop.fixed | 28 + tests/ui/missing_spin_loop.rs | 28 + tests/ui/missing_spin_loop.stderr | 40 ++ tests/ui/missing_spin_loop_no_std.fixed | 23 + tests/ui/missing_spin_loop_no_std.rs | 23 + tests/ui/missing_spin_loop_no_std.stderr | 10 + tests/ui/needless_match.fixed | 83 +++ tests/ui/needless_match.rs | 122 ++++ tests/ui/needless_match.stderr | 122 ++++ tests/ui/only_used_in_recursion.rs | 122 ++++ tests/ui/only_used_in_recursion.stderr | 82 +++ tests/ui/range_plus_minus_one.fixed | 2 +- tests/ui/range_plus_minus_one.rs | 2 +- .../transmutes_expressible_as_ptr_casts.fixed | 4 +- .../ui/transmutes_expressible_as_ptr_casts.rs | 4 +- ...transmutes_expressible_as_ptr_casts.stderr | 4 +- tests/ui/unnecessary_filter_map.rs | 129 ++++ tests/ui/unnecessary_filter_map.stderr | 8 +- tests/ui/unnecessary_find_map.rs | 23 + tests/ui/unnecessary_find_map.stderr | 38 + tests/ui/unnecessary_sort_by.fixed | 10 +- tests/ui/unnecessary_sort_by.rs | 2 - tests/ui/unnecessary_sort_by.stderr | 32 +- tests/ui/use_self.fixed | 25 +- tests/ui/use_self.rs | 25 +- tests/ui/use_self.stderr | 20 +- util/gh-pages/index.html | 284 ++++++-- 91 files changed, 3351 insertions(+), 332 deletions(-) create mode 100644 clippy_lints/src/casts/cast_slice_different_sizes.rs create mode 100644 clippy_lints/src/loops/missing_spin_loop.rs create mode 100644 clippy_lints/src/matches/needless_match.rs create mode 100644 clippy_lints/src/methods/iter_with_drain.rs create mode 100644 clippy_lints/src/only_used_in_recursion.rs create mode 100644 tests/ui-internal/invalid_msrv_attr_impl.fixed create mode 100644 tests/ui-internal/invalid_msrv_attr_impl.rs create mode 100644 tests/ui-internal/invalid_msrv_attr_impl.stderr create mode 100644 tests/ui/allow_attributes_without_reason.rs create mode 100644 tests/ui/allow_attributes_without_reason.stderr create mode 100644 tests/ui/cast_slice_different_sizes.rs create mode 100644 tests/ui/cast_slice_different_sizes.stderr create mode 100644 tests/ui/iter_with_drain.fixed create mode 100644 tests/ui/iter_with_drain.rs create mode 100644 tests/ui/iter_with_drain.stderr create mode 100644 tests/ui/missing_spin_loop.fixed create mode 100644 tests/ui/missing_spin_loop.rs create mode 100644 tests/ui/missing_spin_loop.stderr create mode 100644 tests/ui/missing_spin_loop_no_std.fixed create mode 100644 tests/ui/missing_spin_loop_no_std.rs create mode 100644 tests/ui/missing_spin_loop_no_std.stderr create mode 100644 tests/ui/needless_match.fixed create mode 100644 tests/ui/needless_match.rs create mode 100644 tests/ui/needless_match.stderr create mode 100644 tests/ui/only_used_in_recursion.rs create mode 100644 tests/ui/only_used_in_recursion.stderr create mode 100644 tests/ui/unnecessary_find_map.rs create mode 100644 tests/ui/unnecessary_find_map.stderr diff --git a/.github/workflows/clippy.yml b/.github/workflows/clippy.yml index 116ae031bb7..cd83bc9642b 100644 --- a/.github/workflows/clippy.yml +++ b/.github/workflows/clippy.yml @@ -74,10 +74,3 @@ jobs: run: bash .github/driver.sh env: OS: ${{ runner.os }} - - - name: Test cargo dev new lint - run: | - cargo dev new_lint --name new_early_pass --pass early - cargo dev new_lint --name new_late_pass --pass late - cargo check - git reset --hard HEAD diff --git a/.github/workflows/clippy_bors.yml b/.github/workflows/clippy_bors.yml index 989667037c1..f571485e6d3 100644 --- a/.github/workflows/clippy_bors.yml +++ b/.github/workflows/clippy_bors.yml @@ -143,13 +143,6 @@ jobs: env: OS: ${{ runner.os }} - - name: Test cargo dev new lint - run: | - cargo dev new_lint --name new_early_pass --pass early - cargo dev new_lint --name new_late_pass --pass late - cargo check - git reset --hard HEAD - integration_build: needs: changelog runs-on: ubuntu-latest diff --git a/.github/workflows/clippy_dev.yml b/.github/workflows/clippy_dev.yml index fe8bce00fa8..5dfab1d2ebc 100644 --- a/.github/workflows/clippy_dev.yml +++ b/.github/workflows/clippy_dev.yml @@ -36,6 +36,13 @@ jobs: - name: Test fmt run: cargo dev fmt --check + - name: Test cargo dev new lint + run: | + cargo dev new_lint --name new_early_pass --pass early + cargo dev new_lint --name new_late_pass --pass late + cargo check + git reset --hard HEAD + # These jobs doesn't actually test anything, but they're only used to tell # bors the build completed, as there is no practical way to detect when a # workflow is successful listening to webhooks only. diff --git a/CHANGELOG.md b/CHANGELOG.md index 35983b7fb50..2bc393d6042 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3042,6 +3042,7 @@ Released 2018-09-13 [`absurd_extreme_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#absurd_extreme_comparisons +[`allow_attributes_without_reason`]: https://rust-lang.github.io/rust-clippy/master/index.html#allow_attributes_without_reason [`almost_swapped`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_swapped [`approx_constant`]: https://rust-lang.github.io/rust-clippy/master/index.html#approx_constant [`as_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#as_conversions @@ -3076,6 +3077,7 @@ Released 2018-09-13 [`cast_ptr_alignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_ptr_alignment [`cast_ref_to_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_ref_to_mut [`cast_sign_loss`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_sign_loss +[`cast_slice_different_sizes`]: https://rust-lang.github.io/rust-clippy/master/index.html#cast_slice_different_sizes [`char_lit_as_u8`]: https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8 [`chars_last_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_last_cmp [`chars_next_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_next_cmp @@ -3225,6 +3227,7 @@ Released 2018-09-13 [`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero [`iter_overeager_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_overeager_cloned [`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next +[`iter_with_drain`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_with_drain [`iterator_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iterator_step_by_zero [`just_underscores_and_digits`]: https://rust-lang.github.io/rust-clippy/master/index.html#just_underscores_and_digits [`large_const_arrays`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_const_arrays @@ -3297,6 +3300,7 @@ Released 2018-09-13 [`missing_inline_in_public_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_inline_in_public_items [`missing_panics_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_panics_doc [`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc +[`missing_spin_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_spin_loop [`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes [`mixed_case_hex_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_case_hex_literals [`mod_module_files`]: https://rust-lang.github.io/rust-clippy/master/index.html#mod_module_files @@ -3327,6 +3331,7 @@ Released 2018-09-13 [`needless_for_each`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_for_each [`needless_late_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_late_init [`needless_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes +[`needless_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_match [`needless_option_as_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_as_deref [`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value [`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark @@ -3351,6 +3356,7 @@ Released 2018-09-13 [`not_unsafe_ptr_arg_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref [`octal_escapes`]: https://rust-lang.github.io/rust-clippy/master/index.html#octal_escapes [`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect +[`only_used_in_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref [`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref [`option_env_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_env_unwrap @@ -3498,6 +3504,7 @@ Released 2018-09-13 [`unit_return_expecting_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_return_expecting_ord [`unnecessary_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast [`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map +[`unnecessary_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_find_map [`unnecessary_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fold [`unnecessary_lazy_evaluations`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations [`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed diff --git a/Cargo.toml b/Cargo.toml index d4ca9480bec..123af23881b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,7 +50,7 @@ syn = { version = "1.0", features = ["full"] } futures = "0.3" parking_lot = "0.11.2" tokio = { version = "1", features = ["io-util"] } -num_cpus = "1.13" +rustc-semver = "1.1" [build-dependencies] rustc_tools_util = { version = "0.2", path = "rustc_tools_util" } diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index a58d12ddd6b..d0b03c0a9c2 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -255,7 +255,38 @@ declare_clippy_lint! { "usage of `cfg(operating_system)` instead of `cfg(target_os = \"operating_system\")`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for attributes that allow lints without a reason. + /// + /// (This requires the `lint_reasons` feature) + /// + /// ### Why is this bad? + /// Allowing a lint should always have a reason. This reason should be documented to + /// ensure that others understand the reasoning + /// + /// ### Example + /// Bad: + /// ```rust + /// #![feature(lint_reasons)] + /// + /// #![allow(clippy::some_lint)] + /// ``` + /// + /// Good: + /// ```rust + /// #![feature(lint_reasons)] + /// + /// #![allow(clippy::some_lint, reason = "False positive rust-lang/rust-clippy#1002020")] + /// ``` + #[clippy::version = "1.61.0"] + pub ALLOW_ATTRIBUTES_WITHOUT_REASON, + restriction, + "ensures that all `allow` and `expect` attributes have a reason" +} + declare_lint_pass!(Attributes => [ + ALLOW_ATTRIBUTES_WITHOUT_REASON, INLINE_ALWAYS, DEPRECATED_SEMVER, USELESS_ATTRIBUTE, @@ -269,6 +300,9 @@ impl<'tcx> LateLintPass<'tcx> for Attributes { if is_lint_level(ident.name) { check_clippy_lint_names(cx, ident.name, items); } + if matches!(ident.name, sym::allow | sym::expect) { + check_lint_reason(cx, ident.name, items, attr); + } if items.is_empty() || !attr.has_name(sym::deprecated) { return; } @@ -404,6 +438,30 @@ fn check_clippy_lint_names(cx: &LateContext<'_>, name: Symbol, items: &[NestedMe } } +fn check_lint_reason(cx: &LateContext<'_>, name: Symbol, items: &[NestedMetaItem], attr: &'_ Attribute) { + // Check for the feature + if !cx.tcx.sess.features_untracked().lint_reasons { + return; + } + + // Check if the reason is present + if let Some(item) = items.last().and_then(NestedMetaItem::meta_item) + && let MetaItemKind::NameValue(_) = &item.kind + && item.path == sym::reason + { + return; + } + + span_lint_and_help( + cx, + ALLOW_ATTRIBUTES_WITHOUT_REASON, + attr.span, + &format!("`{}` attribute without specifying a reason", name.as_str()), + None, + "try adding a reason at the end with `, reason = \"..\"`", + ); +} + fn is_relevant_item(cx: &LateContext<'_>, item: &Item<'_>) -> bool { if let ItemKind::Fn(_, _, eid) = item.kind { is_relevant_expr(cx, cx.tcx.typeck_body(eid), &cx.tcx.hir().body(eid).value) @@ -659,5 +717,5 @@ fn check_mismatched_target_os(cx: &EarlyContext<'_>, attr: &Attribute) { } fn is_lint_level(symbol: Symbol) -> bool { - matches!(symbol, sym::allow | sym::warn | sym::deny | sym::forbid) + matches!(symbol, sym::allow | sym::expect | sym::warn | sym::deny | sym::forbid) } diff --git a/clippy_lints/src/casts/cast_slice_different_sizes.rs b/clippy_lints/src/casts/cast_slice_different_sizes.rs new file mode 100644 index 00000000000..3608c1654d5 --- /dev/null +++ b/clippy_lints/src/casts/cast_slice_different_sizes.rs @@ -0,0 +1,117 @@ +use clippy_utils::{diagnostics::span_lint_and_then, meets_msrv, msrvs, source::snippet_opt}; +use if_chain::if_chain; +use rustc_ast::Mutability; +use rustc_hir::{Expr, ExprKind, Node}; +use rustc_lint::LateContext; +use rustc_middle::ty::{self, layout::LayoutOf, Ty, TypeAndMut}; +use rustc_semver::RustcVersion; + +use super::CAST_SLICE_DIFFERENT_SIZES; + +fn is_child_of_cast(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + let map = cx.tcx.hir(); + if_chain! { + if let Some(parent_id) = map.find_parent_node(expr.hir_id); + if let Some(parent) = map.find(parent_id); + then { + let expr = match parent { + Node::Block(block) => { + if let Some(parent_expr) = block.expr { + parent_expr + } else { + return false; + } + }, + Node::Expr(expr) => expr, + _ => return false, + }; + + matches!(expr.kind, ExprKind::Cast(..)) + } else { + false + } + } +} + +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, msrv: &Option) { + // suggestion is invalid if `ptr::slice_from_raw_parts` does not exist + if !meets_msrv(msrv.as_ref(), &msrvs::PTR_SLICE_RAW_PARTS) { + return; + } + + // if this cast is the child of another cast expression then don't emit something for it, the full + // chain will be analyzed + if is_child_of_cast(cx, expr) { + return; + } + + if let Some((from_slice_ty, to_slice_ty)) = expr_cast_chain_tys(cx, expr) { + if let (Ok(from_layout), Ok(to_layout)) = (cx.layout_of(from_slice_ty.ty), cx.layout_of(to_slice_ty.ty)) { + let from_size = from_layout.size.bytes(); + let to_size = to_layout.size.bytes(); + if from_size != to_size && from_size != 0 && to_size != 0 { + span_lint_and_then( + cx, + CAST_SLICE_DIFFERENT_SIZES, + expr.span, + &format!( + "casting between raw pointers to `[{}]` (element size {}) and `[{}]` (element size {}) does not adjust the count", + from_slice_ty, from_size, to_slice_ty, to_size, + ), + |diag| { + let cast_expr = match expr.kind { + ExprKind::Cast(cast_expr, ..) => cast_expr, + _ => unreachable!("expr should be a cast as checked by expr_cast_chain_tys"), + }; + let ptr_snippet = snippet_opt(cx, cast_expr.span).unwrap(); + + let (mutbl_fn_str, mutbl_ptr_str) = match to_slice_ty.mutbl { + Mutability::Mut => ("_mut", "mut"), + Mutability::Not => ("", "const"), + }; + let sugg = format!( + "core::ptr::slice_from_raw_parts{mutbl_fn_str}({ptr_snippet} as *{mutbl_ptr_str} {to_slice_ty}, ..)" + ); + + diag.span_suggestion( + expr.span, + &format!("replace with `ptr::slice_from_raw_parts{mutbl_fn_str}`"), + sugg, + rustc_errors::Applicability::HasPlaceholders, + ); + }, + ); + } + } + } +} + +/// Returns the type T of the pointed to *const [T] or *mut [T] and the mutability of the slice if +/// the type is one of those slices +fn get_raw_slice_ty_mut(ty: Ty<'_>) -> Option> { + match ty.kind() { + ty::RawPtr(TypeAndMut { ty: slice_ty, mutbl }) => match slice_ty.kind() { + ty::Slice(ty) => Some(TypeAndMut { ty: *ty, mutbl: *mutbl }), + _ => None, + }, + _ => None, + } +} + +/// Returns the pair (original ptr T, final ptr U) if the expression is composed of casts +/// Returns None if the expr is not a Cast +fn expr_cast_chain_tys<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option<(TypeAndMut<'tcx>, TypeAndMut<'tcx>)> { + if let ExprKind::Cast(cast_expr, _cast_to_hir_ty) = expr.peel_blocks().kind { + let cast_to = cx.typeck_results().expr_ty(expr); + let to_slice_ty = get_raw_slice_ty_mut(cast_to)?; + if let Some((inner_from_ty, _inner_to_ty)) = expr_cast_chain_tys(cx, cast_expr) { + Some((inner_from_ty, to_slice_ty)) + } else { + let cast_from = cx.typeck_results().expr_ty(cast_expr); + let from_slice_ty = get_raw_slice_ty_mut(cast_from)?; + Some((from_slice_ty, to_slice_ty)) + } + } else { + None + } +} diff --git a/clippy_lints/src/casts/mod.rs b/clippy_lints/src/casts/mod.rs index f2077c569c0..6a0eabd089d 100644 --- a/clippy_lints/src/casts/mod.rs +++ b/clippy_lints/src/casts/mod.rs @@ -5,6 +5,7 @@ mod cast_precision_loss; mod cast_ptr_alignment; mod cast_ref_to_mut; mod cast_sign_loss; +mod cast_slice_different_sizes; mod char_lit_as_u8; mod fn_to_numeric_cast; mod fn_to_numeric_cast_any; @@ -409,6 +410,50 @@ declare_clippy_lint! { "casts from an enum type to an integral type which will truncate the value" } +declare_clippy_lint! { + /// Checks for `as` casts between raw pointers to slices with differently sized elements. + /// + /// ### Why is this bad? + /// The produced raw pointer to a slice does not update its length metadata. The produced + /// pointer will point to a different number of bytes than the original pointer because the + /// length metadata of a raw slice pointer is in elements rather than bytes. + /// Producing a slice reference from the raw pointer will either create a slice with + /// less data (which can be surprising) or create a slice with more data and cause Undefined Behavior. + /// + /// ### Example + /// // Missing data + /// ```rust + /// let a = [1_i32, 2, 3, 4]; + /// let p = &a as *const [i32] as *const [u8]; + /// unsafe { + /// println!("{:?}", &*p); + /// } + /// ``` + /// // Undefined Behavior (note: also potential alignment issues) + /// ```rust + /// let a = [1_u8, 2, 3, 4]; + /// let p = &a as *const [u8] as *const [u32]; + /// unsafe { + /// println!("{:?}", &*p); + /// } + /// ``` + /// Instead use `ptr::slice_from_raw_parts` to construct a slice from a data pointer and the correct length + /// ```rust + /// let a = [1_i32, 2, 3, 4]; + /// let old_ptr = &a as *const [i32]; + /// // The data pointer is cast to a pointer to the target `u8` not `[u8]` + /// // The length comes from the known length of 4 i32s times the 4 bytes per i32 + /// let new_ptr = core::ptr::slice_from_raw_parts(old_ptr as *const u8, 16); + /// unsafe { + /// println!("{:?}", &*new_ptr); + /// } + /// ``` + #[clippy::version = "1.60.0"] + pub CAST_SLICE_DIFFERENT_SIZES, + correctness, + "casting using `as` between raw pointers to slices of types with different sizes" +} + pub struct Casts { msrv: Option, } @@ -428,6 +473,7 @@ impl_lint_pass!(Casts => [ CAST_LOSSLESS, CAST_REF_TO_MUT, CAST_PTR_ALIGNMENT, + CAST_SLICE_DIFFERENT_SIZES, UNNECESSARY_CAST, FN_TO_NUMERIC_CAST_ANY, FN_TO_NUMERIC_CAST, @@ -478,6 +524,8 @@ impl<'tcx> LateLintPass<'tcx> for Casts { cast_ref_to_mut::check(cx, expr); cast_ptr_alignment::check(cx, expr); char_lit_as_u8::check(cx, expr); + ptr_as_ptr::check(cx, expr, &self.msrv); + cast_slice_different_sizes::check(cx, expr, &self.msrv); } extract_msrv_attr!(LateContext); diff --git a/clippy_lints/src/dbg_macro.rs b/clippy_lints/src/dbg_macro.rs index df1a4128af3..a0e5d302633 100644 --- a/clippy_lints/src/dbg_macro.rs +++ b/clippy_lints/src/dbg_macro.rs @@ -1,4 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_in_test_function; use clippy_utils::macros::root_macro_call_first_node; use clippy_utils::source::snippet_with_applicability; use rustc_errors::Applicability; @@ -35,6 +36,10 @@ impl LateLintPass<'_> for DbgMacro { fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { let Some(macro_call) = root_macro_call_first_node(cx, expr) else { return }; if cx.tcx.is_diagnostic_item(sym::dbg_macro, macro_call.def_id) { + // we make an exception for test code + if is_in_test_function(cx.tcx, expr.hir_id) { + return; + } let mut applicability = Applicability::MachineApplicable; let suggestion = match expr.peel_drop_temps().kind { // dbg!() diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index abfb4603537..23bca5a0eab 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -25,6 +25,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(booleans::NONMINIMAL_BOOL), LintId::of(casts::CAST_ENUM_TRUNCATION), LintId::of(casts::CAST_REF_TO_MUT), + LintId::of(casts::CAST_SLICE_DIFFERENT_SIZES), LintId::of(casts::CHAR_LIT_AS_U8), LintId::of(casts::FN_TO_NUMERIC_CAST), LintId::of(casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION), @@ -109,6 +110,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(loops::ITER_NEXT_LOOP), LintId::of(loops::MANUAL_FLATTEN), LintId::of(loops::MANUAL_MEMCPY), + LintId::of(loops::MISSING_SPIN_LOOP), LintId::of(loops::MUT_RANGE_BOUND), LintId::of(loops::NEEDLESS_COLLECT), LintId::of(loops::NEEDLESS_RANGE_LOOP), @@ -136,6 +138,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(matches::MATCH_OVERLAPPING_ARM), LintId::of(matches::MATCH_REF_PATS), LintId::of(matches::MATCH_SINGLE_BINDING), + LintId::of(matches::NEEDLESS_MATCH), LintId::of(matches::REDUNDANT_PATTERN_MATCHING), LintId::of(matches::SINGLE_MATCH), LintId::of(matches::WILDCARD_IN_OR_PATTERNS), @@ -163,6 +166,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(methods::ITER_NTH_ZERO), LintId::of(methods::ITER_OVEREAGER_CLONED), LintId::of(methods::ITER_SKIP_NEXT), + LintId::of(methods::ITER_WITH_DRAIN), LintId::of(methods::MANUAL_FILTER_MAP), LintId::of(methods::MANUAL_FIND_MAP), LintId::of(methods::MANUAL_SATURATING_ARITHMETIC), @@ -189,6 +193,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(methods::SUSPICIOUS_SPLITN), LintId::of(methods::UNINIT_ASSUMED_INIT), LintId::of(methods::UNNECESSARY_FILTER_MAP), + LintId::of(methods::UNNECESSARY_FIND_MAP), LintId::of(methods::UNNECESSARY_FOLD), LintId::of(methods::UNNECESSARY_LAZY_EVALUATIONS), LintId::of(methods::UNNECESSARY_TO_OWNED), @@ -231,6 +236,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(non_expressive_names::JUST_UNDERSCORES_AND_DIGITS), LintId::of(non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS), LintId::of(octal_escapes::OCTAL_ESCAPES), + LintId::of(only_used_in_recursion::ONLY_USED_IN_RECURSION), LintId::of(open_options::NONSENSICAL_OPEN_OPTIONS), LintId::of(option_env_unwrap::OPTION_ENV_UNWRAP), LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index bd5ff613447..68d6c6ce5f7 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -30,6 +30,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN), LintId::of(matches::MATCH_AS_REF), LintId::of(matches::MATCH_SINGLE_BINDING), + LintId::of(matches::NEEDLESS_MATCH), LintId::of(matches::WILDCARD_IN_OR_PATTERNS), LintId::of(methods::BIND_INSTEAD_OF_MAP), LintId::of(methods::CLONE_ON_COPY), @@ -49,6 +50,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(methods::SEARCH_IS_SOME), LintId::of(methods::SKIP_WHILE_NEXT), LintId::of(methods::UNNECESSARY_FILTER_MAP), + LintId::of(methods::UNNECESSARY_FIND_MAP), LintId::of(methods::USELESS_ASREF), LintId::of(misc::SHORT_CIRCUIT_STATEMENT), LintId::of(misc_early::UNNEEDED_WILDCARD_PATTERN), @@ -63,6 +65,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD), LintId::of(no_effect::NO_EFFECT), LintId::of(no_effect::UNNECESSARY_OPERATION), + LintId::of(only_used_in_recursion::ONLY_USED_IN_RECURSION), LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), LintId::of(partialeq_ne_impl::PARTIALEQ_NE_IMPL), LintId::of(precedence::PRECEDENCE), diff --git a/clippy_lints/src/lib.register_correctness.rs b/clippy_lints/src/lib.register_correctness.rs index d7bf91eb692..df63f84463d 100644 --- a/clippy_lints/src/lib.register_correctness.rs +++ b/clippy_lints/src/lib.register_correctness.rs @@ -13,6 +13,7 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve LintId::of(bit_mask::INEFFECTIVE_BIT_MASK), LintId::of(booleans::LOGIC_BUG), LintId::of(casts::CAST_REF_TO_MUT), + LintId::of(casts::CAST_SLICE_DIFFERENT_SIZES), LintId::of(copies::IFS_SAME_COND), LintId::of(copies::IF_SAME_THEN_ELSE), LintId::of(derive::DERIVE_HASH_XOR_EQ), diff --git a/clippy_lints/src/lib.register_internal.rs b/clippy_lints/src/lib.register_internal.rs index 7d4c7d2adb5..4778f4fdfa7 100644 --- a/clippy_lints/src/lib.register_internal.rs +++ b/clippy_lints/src/lib.register_internal.rs @@ -14,6 +14,7 @@ store.register_group(true, "clippy::internal", Some("clippy_internal"), vec![ LintId::of(utils::internal_lints::LINT_WITHOUT_LINT_PASS), LintId::of(utils::internal_lints::MATCH_TYPE_ON_DIAGNOSTIC_ITEM), LintId::of(utils::internal_lints::MISSING_CLIPPY_VERSION_ATTRIBUTE), + LintId::of(utils::internal_lints::MISSING_MSRV_ATTR_IMPL), LintId::of(utils::internal_lints::OUTER_EXPN_EXPN_DATA), LintId::of(utils::internal_lints::PRODUCE_ICE), LintId::of(utils::internal_lints::UNNECESSARY_SYMBOL_STR), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 686482671b4..1a45763a869 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -26,6 +26,8 @@ store.register_lints(&[ #[cfg(feature = "internal")] utils::internal_lints::MISSING_CLIPPY_VERSION_ATTRIBUTE, #[cfg(feature = "internal")] + utils::internal_lints::MISSING_MSRV_ATTR_IMPL, + #[cfg(feature = "internal")] utils::internal_lints::OUTER_EXPN_EXPN_DATA, #[cfg(feature = "internal")] utils::internal_lints::PRODUCE_ICE, @@ -42,6 +44,7 @@ store.register_lints(&[ assign_ops::ASSIGN_OP_PATTERN, assign_ops::MISREFACTORED_ASSIGN_OP, async_yields_async::ASYNC_YIELDS_ASYNC, + attrs::ALLOW_ATTRIBUTES_WITHOUT_REASON, attrs::BLANKET_CLIPPY_RESTRICTION_LINTS, attrs::DEPRECATED_CFG_ATTR, attrs::DEPRECATED_SEMVER, @@ -75,6 +78,7 @@ store.register_lints(&[ casts::CAST_PTR_ALIGNMENT, casts::CAST_REF_TO_MUT, casts::CAST_SIGN_LOSS, + casts::CAST_SLICE_DIFFERENT_SIZES, casts::CHAR_LIT_AS_U8, casts::FN_TO_NUMERIC_CAST, casts::FN_TO_NUMERIC_CAST_ANY, @@ -219,6 +223,7 @@ store.register_lints(&[ loops::ITER_NEXT_LOOP, loops::MANUAL_FLATTEN, loops::MANUAL_MEMCPY, + loops::MISSING_SPIN_LOOP, loops::MUT_RANGE_BOUND, loops::NEEDLESS_COLLECT, loops::NEEDLESS_RANGE_LOOP, @@ -255,6 +260,7 @@ store.register_lints(&[ matches::MATCH_SINGLE_BINDING, matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS, matches::MATCH_WILD_ERR_ARM, + matches::NEEDLESS_MATCH, matches::REDUNDANT_PATTERN_MATCHING, matches::REST_PAT_IN_FULLY_BOUND_STRUCTS, matches::SINGLE_MATCH, @@ -296,6 +302,7 @@ store.register_lints(&[ methods::ITER_NTH_ZERO, methods::ITER_OVEREAGER_CLONED, methods::ITER_SKIP_NEXT, + methods::ITER_WITH_DRAIN, methods::MANUAL_FILTER_MAP, methods::MANUAL_FIND_MAP, methods::MANUAL_SATURATING_ARITHMETIC, @@ -323,6 +330,7 @@ store.register_lints(&[ methods::SUSPICIOUS_SPLITN, methods::UNINIT_ASSUMED_INIT, methods::UNNECESSARY_FILTER_MAP, + methods::UNNECESSARY_FIND_MAP, methods::UNNECESSARY_FOLD, methods::UNNECESSARY_LAZY_EVALUATIONS, methods::UNNECESSARY_TO_OWNED, @@ -392,6 +400,7 @@ store.register_lints(&[ non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY, nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES, octal_escapes::OCTAL_ESCAPES, + only_used_in_recursion::ONLY_USED_IN_RECURSION, open_options::NONSENSICAL_OPEN_OPTIONS, option_env_unwrap::OPTION_ENV_UNWRAP, option_if_let_else::OPTION_IF_LET_ELSE, diff --git a/clippy_lints/src/lib.register_perf.rs b/clippy_lints/src/lib.register_perf.rs index c44ef124bfa..6e9c0ee33a1 100644 --- a/clippy_lints/src/lib.register_perf.rs +++ b/clippy_lints/src/lib.register_perf.rs @@ -10,11 +10,13 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![ LintId::of(large_const_arrays::LARGE_CONST_ARRAYS), LintId::of(large_enum_variant::LARGE_ENUM_VARIANT), LintId::of(loops::MANUAL_MEMCPY), + LintId::of(loops::MISSING_SPIN_LOOP), LintId::of(loops::NEEDLESS_COLLECT), LintId::of(methods::EXPECT_FUN_CALL), LintId::of(methods::EXTEND_WITH_DRAIN), LintId::of(methods::ITER_NTH), LintId::of(methods::ITER_OVEREAGER_CLONED), + LintId::of(methods::ITER_WITH_DRAIN), LintId::of(methods::MANUAL_STR_REPEAT), LintId::of(methods::OR_FUN_CALL), LintId::of(methods::SINGLE_CHAR_PATTERN), diff --git a/clippy_lints/src/lib.register_restriction.rs b/clippy_lints/src/lib.register_restriction.rs index f89f35b885c..4e30fc38197 100644 --- a/clippy_lints/src/lib.register_restriction.rs +++ b/clippy_lints/src/lib.register_restriction.rs @@ -8,6 +8,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve LintId::of(as_conversions::AS_CONVERSIONS), LintId::of(asm_syntax::INLINE_ASM_X86_ATT_SYNTAX), LintId::of(asm_syntax::INLINE_ASM_X86_INTEL_SYNTAX), + LintId::of(attrs::ALLOW_ATTRIBUTES_WITHOUT_REASON), LintId::of(casts::FN_TO_NUMERIC_CAST_ANY), LintId::of(create_dir::CREATE_DIR), LintId::of(dbg_macro::DBG_MACRO), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 230e2b2ccdf..504235d0d1e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -318,6 +318,7 @@ mod non_octal_unix_permissions; mod non_send_fields_in_send_ty; mod nonstandard_macro_braces; mod octal_escapes; +mod only_used_in_recursion; mod open_options; mod option_env_unwrap; mod option_if_let_else; @@ -505,6 +506,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(utils::internal_lints::LintWithoutLintPass::default())); store.register_late_pass(|| Box::new(utils::internal_lints::MatchTypeOnDiagItem)); store.register_late_pass(|| Box::new(utils::internal_lints::OuterExpnDataPass)); + store.register_late_pass(|| Box::new(utils::internal_lints::MsrvAttrImpl)); } store.register_late_pass(|| Box::new(utils::author::Author)); @@ -856,6 +858,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || Box::new(borrow_as_ptr::BorrowAsPtr::new(msrv))); store.register_late_pass(move || Box::new(manual_bits::ManualBits::new(msrv))); store.register_late_pass(|| Box::new(default_union_representation::DefaultUnionRepresentation)); + store.register_late_pass(|| Box::new(only_used_in_recursion::OnlyUsedInRecursion)); store.register_late_pass(|| Box::new(dbg_macro::DbgMacro)); let cargo_ignore_publish = conf.cargo_ignore_publish; store.register_late_pass(move || { diff --git a/clippy_lints/src/loops/missing_spin_loop.rs b/clippy_lints/src/loops/missing_spin_loop.rs new file mode 100644 index 00000000000..0696afa3922 --- /dev/null +++ b/clippy_lints/src/loops/missing_spin_loop.rs @@ -0,0 +1,56 @@ +use super::MISSING_SPIN_LOOP; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_no_std_crate; +use rustc_errors::Applicability; +use rustc_hir::{Block, Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_middle::ty; +use rustc_span::sym; + +fn unpack_cond<'tcx>(cond: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> { + match &cond.kind { + ExprKind::Block( + Block { + stmts: [], + expr: Some(e), + .. + }, + _, + ) + | ExprKind::Unary(_, e) => unpack_cond(e), + ExprKind::Binary(_, l, r) => { + let l = unpack_cond(l); + if let ExprKind::MethodCall(..) = l.kind { + l + } else { + unpack_cond(r) + } + }, + _ => cond, + } +} + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, cond: &'tcx Expr<'_>, body: &'tcx Expr<'_>) { + if_chain! { + if let ExprKind::Block(Block { stmts: [], expr: None, ..}, _) = body.kind; + if let ExprKind::MethodCall(method, [callee, ..], _) = unpack_cond(cond).kind; + if [sym::load, sym::compare_exchange, sym::compare_exchange_weak].contains(&method.ident.name); + if let ty::Adt(def, _substs) = cx.typeck_results().expr_ty(callee).kind(); + if cx.tcx.is_diagnostic_item(sym::AtomicBool, def.did()); + then { + span_lint_and_sugg( + cx, + MISSING_SPIN_LOOP, + body.span, + "busy-waiting loop should at least have a spin loop hint", + "try this", + (if is_no_std_crate(cx) { + "{ core::hint::spin_loop() }" + } else { + "{ std::hint::spin_loop() }" + }).into(), + Applicability::MachineApplicable + ); + } + } +} diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index 5bc32acf56e..f029067d367 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -7,6 +7,7 @@ mod for_loops_over_fallibles; mod iter_next_loop; mod manual_flatten; mod manual_memcpy; +mod missing_spin_loop; mod mut_range_bound; mod needless_collect; mod needless_range_loop; @@ -560,6 +561,42 @@ declare_clippy_lint! { "for loops over `Option`s or `Result`s with a single expression can be simplified" } +declare_clippy_lint! { + /// ### What it does + /// Check for empty spin loops + /// + /// ### Why is this bad? + /// The loop body should have something like `thread::park()` or at least + /// `std::hint::spin_loop()` to avoid needlessly burning cycles and conserve + /// energy. Perhaps even better use an actual lock, if possible. + /// + /// ### Known problems + /// This lint doesn't currently trigger on `while let` or + /// `loop { match .. { .. } }` loops, which would be considered idiomatic in + /// combination with e.g. `AtomicBool::compare_exchange_weak`. + /// + /// ### Example + /// + /// ```ignore + /// use core::sync::atomic::{AtomicBool, Ordering}; + /// let b = AtomicBool::new(true); + /// // give a ref to `b` to another thread,wait for it to become false + /// while b.load(Ordering::Acquire) {}; + /// ``` + /// Use instead: + /// ```rust,no_run + ///# use core::sync::atomic::{AtomicBool, Ordering}; + ///# let b = AtomicBool::new(true); + /// while b.load(Ordering::Acquire) { + /// std::hint::spin_loop() + /// } + /// ``` + #[clippy::version = "1.59.0"] + pub MISSING_SPIN_LOOP, + perf, + "An empty busy waiting loop" +} + declare_lint_pass!(Loops => [ MANUAL_MEMCPY, MANUAL_FLATTEN, @@ -579,6 +616,7 @@ declare_lint_pass!(Loops => [ WHILE_IMMUTABLE_CONDITION, SAME_ITEM_PUSH, SINGLE_ELEMENT_LOOP, + MISSING_SPIN_LOOP, ]); impl<'tcx> LateLintPass<'tcx> for Loops { @@ -628,6 +666,7 @@ impl<'tcx> LateLintPass<'tcx> for Loops { if let Some(higher::While { condition, body }) = higher::While::hir(expr) { while_immutable_condition::check(cx, condition, body); + missing_spin_loop::check(cx, condition, body); } needless_collect::check(expr, cx); diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 92179eb6f0e..ff85623acf4 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -16,6 +16,7 @@ mod match_same_arms; mod match_single_binding; mod match_wild_enum; mod match_wild_err_arm; +mod needless_match; mod overlapping_arms; mod redundant_pattern_match; mod rest_pat_in_fully_bound_struct; @@ -566,6 +567,49 @@ declare_clippy_lint! { "`match` with identical arm bodies" } +declare_clippy_lint! { + /// ### What it does + /// Checks for unnecessary `match` or match-like `if let` returns for `Option` and `Result` + /// when function signatures are the same. + /// + /// ### Why is this bad? + /// This `match` block does nothing and might not be what the coder intended. + /// + /// ### Example + /// ```rust,ignore + /// fn foo() -> Result<(), i32> { + /// match result { + /// Ok(val) => Ok(val), + /// Err(err) => Err(err), + /// } + /// } + /// + /// fn bar() -> Option { + /// if let Some(val) = option { + /// Some(val) + /// } else { + /// None + /// } + /// } + /// ``` + /// + /// Could be replaced as + /// + /// ```rust,ignore + /// fn foo() -> Result<(), i32> { + /// result + /// } + /// + /// fn bar() -> Option { + /// option + /// } + /// ``` + #[clippy::version = "1.61.0"] + pub NEEDLESS_MATCH, + complexity, + "`match` or match-like `if let` that are unnecessary" +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -599,6 +643,7 @@ impl_lint_pass!(Matches => [ REDUNDANT_PATTERN_MATCHING, MATCH_LIKE_MATCHES_MACRO, MATCH_SAME_ARMS, + NEEDLESS_MATCH, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -622,6 +667,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches { overlapping_arms::check(cx, ex, arms); match_wild_enum::check(cx, ex, arms); match_as_ref::check(cx, ex, arms, expr); + needless_match::check_match(cx, ex, arms); if self.infallible_destructuring_match_linted { self.infallible_destructuring_match_linted = false; @@ -640,6 +686,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches { match_like_matches::check(cx, expr); } redundant_pattern_match::check(cx, expr); + needless_match::check(cx, expr); } } diff --git a/clippy_lints/src/matches/needless_match.rs b/clippy_lints/src/matches/needless_match.rs new file mode 100644 index 00000000000..76131d307d7 --- /dev/null +++ b/clippy_lints/src/matches/needless_match.rs @@ -0,0 +1,197 @@ +use super::NEEDLESS_MATCH; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_applicability; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{eq_expr_value, get_parent_expr, higher, is_else_clause, is_lang_ctor, peel_blocks_with_stmt}; +use rustc_errors::Applicability; +use rustc_hir::LangItem::OptionNone; +use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, Pat, PatKind, Path, PathSegment, QPath, UnOp}; +use rustc_lint::LateContext; +use rustc_span::sym; + +pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) { + // This is for avoiding collision with `match_single_binding`. + if arms.len() < 2 { + return; + } + + for arm in arms { + if let PatKind::Wild = arm.pat.kind { + let ret_expr = strip_return(arm.body); + if !eq_expr_value(cx, ex, ret_expr) { + return; + } + } else if !pat_same_as_expr(arm.pat, arm.body) { + return; + } + } + + if let Some(match_expr) = get_parent_expr(cx, ex) { + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + NEEDLESS_MATCH, + match_expr.span, + "this match expression is unnecessary", + "replace it with", + snippet_with_applicability(cx, ex.span, "..", &mut applicability).to_string(), + applicability, + ); + } +} + +/// Check for nop `if let` expression that assembled as unnecessary match +/// +/// ```rust,ignore +/// if let Some(a) = option { +/// Some(a) +/// } else { +/// None +/// } +/// ``` +/// OR +/// ```rust,ignore +/// if let SomeEnum::A = some_enum { +/// SomeEnum::A +/// } else if let SomeEnum::B = some_enum { +/// SomeEnum::B +/// } else { +/// some_enum +/// } +/// ``` +pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>) { + if_chain! { + if let Some(ref if_let) = higher::IfLet::hir(cx, ex); + if !is_else_clause(cx.tcx, ex); + if check_if_let(cx, if_let); + then { + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + NEEDLESS_MATCH, + ex.span, + "this if-let expression is unnecessary", + "replace it with", + snippet_with_applicability(cx, if_let.let_expr.span, "..", &mut applicability).to_string(), + applicability, + ); + } + } +} + +fn check_if_let(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool { + if let Some(if_else) = if_let.if_else { + if !pat_same_as_expr(if_let.let_pat, peel_blocks_with_stmt(if_let.if_then)) { + return false; + } + + // Recurrsively check for each `else if let` phrase, + if let Some(ref nested_if_let) = higher::IfLet::hir(cx, if_else) { + return check_if_let(cx, nested_if_let); + } + + if matches!(if_else.kind, ExprKind::Block(..)) { + let else_expr = peel_blocks_with_stmt(if_else); + let ret = strip_return(else_expr); + let let_expr_ty = cx.typeck_results().expr_ty(if_let.let_expr); + if is_type_diagnostic_item(cx, let_expr_ty, sym::Option) { + if let ExprKind::Path(ref qpath) = ret.kind { + return is_lang_ctor(cx, qpath, OptionNone) || eq_expr_value(cx, if_let.let_expr, ret); + } + } else { + return eq_expr_value(cx, if_let.let_expr, ret); + } + return true; + } + } + false +} + +/// Strip `return` keyword if the expression type is `ExprKind::Ret`. +fn strip_return<'hir>(expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> { + if let ExprKind::Ret(Some(ret)) = expr.kind { + ret + } else { + expr + } +} + +fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { + let expr = strip_return(expr); + match (&pat.kind, &expr.kind) { + // Example: `Some(val) => Some(val)` + ( + PatKind::TupleStruct(QPath::Resolved(_, path), [first_pat, ..], _), + ExprKind::Call(call_expr, [first_param, ..]), + ) => { + if let ExprKind::Path(QPath::Resolved(_, call_path)) = call_expr.kind { + if has_identical_segments(path.segments, call_path.segments) + && has_same_non_ref_symbol(first_pat, first_param) + { + return true; + } + } + }, + // Example: `val => val`, or `ref val => *val` + (PatKind::Binding(annot, _, pat_ident, _), _) => { + let new_expr = if let ( + BindingAnnotation::Ref | BindingAnnotation::RefMut, + ExprKind::Unary(UnOp::Deref, operand_expr), + ) = (annot, &expr.kind) + { + operand_expr + } else { + expr + }; + + if let ExprKind::Path(QPath::Resolved( + _, + Path { + segments: [first_seg, ..], + .. + }, + )) = new_expr.kind + { + return pat_ident.name == first_seg.ident.name; + } + }, + // Example: `Custom::TypeA => Custom::TypeB`, or `None => None` + (PatKind::Path(QPath::Resolved(_, p_path)), ExprKind::Path(QPath::Resolved(_, e_path))) => { + return has_identical_segments(p_path.segments, e_path.segments); + }, + // Example: `5 => 5` + (PatKind::Lit(pat_lit_expr), ExprKind::Lit(expr_spanned)) => { + if let ExprKind::Lit(pat_spanned) = &pat_lit_expr.kind { + return pat_spanned.node == expr_spanned.node; + } + }, + _ => {}, + } + + false +} + +fn has_identical_segments(left_segs: &[PathSegment<'_>], right_segs: &[PathSegment<'_>]) -> bool { + if left_segs.len() != right_segs.len() { + return false; + } + for i in 0..left_segs.len() { + if left_segs[i].ident.name != right_segs[i].ident.name { + return false; + } + } + true +} + +fn has_same_non_ref_symbol(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { + if_chain! { + if let PatKind::Binding(annot, _, pat_ident, _) = pat.kind; + if !matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut); + if let ExprKind::Path(QPath::Resolved(_, Path {segments: [first_seg, ..], .. })) = expr.kind; + then { + return pat_ident.name == first_seg.ident.name; + } + } + + false +} diff --git a/clippy_lints/src/methods/iter_with_drain.rs b/clippy_lints/src/methods/iter_with_drain.rs new file mode 100644 index 00000000000..958c3773087 --- /dev/null +++ b/clippy_lints/src/methods/iter_with_drain.rs @@ -0,0 +1,72 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_integer_const; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{ + higher::{self, Range}, + SpanlessEq, +}; +use rustc_ast::ast::RangeLimits; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, QPath}; +use rustc_lint::LateContext; +use rustc_span::symbol::{sym, Symbol}; +use rustc_span::Span; + +use super::ITER_WITH_DRAIN; + +const DRAIN_TYPES: &[Symbol] = &[sym::Vec, sym::VecDeque]; + +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: &Expr<'_>) { + let ty = cx.typeck_results().expr_ty(recv).peel_refs(); + if let Some(drained_type) = DRAIN_TYPES.iter().find(|&&sym| is_type_diagnostic_item(cx, ty, sym)) { + // Refuse to emit `into_iter` suggestion on draining struct fields due + // to the strong possibility of processing unmovable field. + if let ExprKind::Field(..) = recv.kind { + return; + } + + if let Some(range) = higher::Range::hir(arg) { + let left_full = match range { + Range { start: Some(start), .. } if is_integer_const(cx, start, 0) => true, + Range { start: None, .. } => true, + _ => false, + }; + let full = left_full + && match range { + Range { + end: Some(end), + limits: RangeLimits::HalfOpen, + .. + } => { + // `x.drain(..x.len())` call + if_chain! { + if let ExprKind::MethodCall(len_path, len_args, _) = end.kind; + if len_path.ident.name == sym::len && len_args.len() == 1; + if let ExprKind::Path(QPath::Resolved(_, drain_path)) = recv.kind; + if let ExprKind::Path(QPath::Resolved(_, len_path)) = len_args[0].kind; + if SpanlessEq::new(cx).eq_path(drain_path, len_path); + then { true } + else { false } + } + }, + Range { + end: None, + limits: RangeLimits::HalfOpen, + .. + } => true, + _ => false, + }; + if full { + span_lint_and_sugg( + cx, + ITER_WITH_DRAIN, + span.with_hi(expr.span.hi()), + &format!("`drain(..)` used on a `{}`", drained_type), + "try this", + "into_iter()".to_string(), + Applicability::MaybeIncorrect, + ); + } + } + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 3021a40fae1..5edd22cd14c 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -32,6 +32,7 @@ mod iter_nth; mod iter_nth_zero; mod iter_overeager_cloned; mod iter_skip_next; +mod iter_with_drain; mod iterator_step_by_zero; mod manual_saturating_arithmetic; mod manual_str_repeat; @@ -1118,6 +1119,31 @@ declare_clippy_lint! { "using `.skip(x).next()` on an iterator" } +declare_clippy_lint! { + /// ### What it does + /// Checks for use of `.drain(..)` on `Vec` and `VecDeque` for iteration. + /// + /// ### Why is this bad? + /// `.into_iter()` is simpler with better performance. + /// + /// ### Example + /// ```rust + /// # use std::collections::HashSet; + /// let mut foo = vec![0, 1, 2, 3]; + /// let bar: HashSet = foo.drain(..).collect(); + /// ``` + /// Use instead: + /// ```rust + /// # use std::collections::HashSet; + /// let foo = vec![0, 1, 2, 3]; + /// let bar: HashSet = foo.into_iter().collect(); + /// ``` + #[clippy::version = "1.61.0"] + pub ITER_WITH_DRAIN, + perf, + "replace `.drain(..)` with `.into_iter()`" +} + declare_clippy_lint! { /// ### What it does /// Checks for use of `.get().unwrap()` (or @@ -1309,7 +1335,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for `filter_map` calls which could be replaced by `filter` or `map`. + /// Checks for `filter_map` calls that could be replaced by `filter` or `map`. /// More specifically it checks if the closure provided is only performing one of the /// filter or map operations and suggests the appropriate option. /// @@ -1337,6 +1363,36 @@ declare_clippy_lint! { "using `filter_map` when a more succinct alternative exists" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `find_map` calls that could be replaced by `find` or `map`. More + /// specifically it checks if the closure provided is only performing one of the + /// find or map operations and suggests the appropriate option. + /// + /// ### Why is this bad? + /// Complexity. The intent is also clearer if only a single + /// operation is being performed. + /// + /// ### Example + /// ```rust + /// let _ = (0..3).find_map(|x| if x > 2 { Some(x) } else { None }); + /// + /// // As there is no transformation of the argument this could be written as: + /// let _ = (0..3).find(|&x| x > 2); + /// ``` + /// + /// ```rust + /// let _ = (0..4).find_map(|x| Some(x + 1)); + /// + /// // As there is no conditional check on the argument this could be written as: + /// let _ = (0..4).map(|x| x + 1).next(); + /// ``` + #[clippy::version = "1.61.0"] + pub UNNECESSARY_FIND_MAP, + complexity, + "using `find_map` when a more succinct alternative exists" +} + declare_clippy_lint! { /// ### What it does /// Checks for `into_iter` calls on references which should be replaced by `iter` @@ -2017,9 +2073,11 @@ impl_lint_pass!(Methods => [ GET_UNWRAP, STRING_EXTEND_CHARS, ITER_CLONED_COLLECT, + ITER_WITH_DRAIN, USELESS_ASREF, UNNECESSARY_FOLD, UNNECESSARY_FILTER_MAP, + UNNECESSARY_FIND_MAP, INTO_ITER_ON_REF, SUSPICIOUS_MAP, UNINIT_ASSUMED_INIT, @@ -2296,6 +2354,9 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio Some(("map", [_, arg], _)) => suspicious_map::check(cx, expr, recv, arg), _ => {}, }, + ("drain", [arg]) => { + iter_with_drain::check(cx, expr, recv, span, arg); + }, ("expect", [_]) => match method_call(recv) { Some(("ok", [recv], _)) => ok_expect::check(cx, expr, recv), _ => expect_used::check(cx, expr, recv), @@ -2305,9 +2366,12 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio extend_with_drain::check(cx, expr, recv, arg); }, ("filter_map", [arg]) => { - unnecessary_filter_map::check(cx, expr, arg); + unnecessary_filter_map::check(cx, expr, arg, name); filter_map_identity::check(cx, expr, arg, span); }, + ("find_map", [arg]) => { + unnecessary_filter_map::check(cx, expr, arg, name); + }, ("flat_map", [arg]) => { flat_map_identity::check(cx, expr, arg, span); flat_map_option::check(cx, expr, arg, span); diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index bdf8cea1207..2a5ab6e625c 100644 --- a/clippy_lints/src/methods/option_map_or_none.rs +++ b/clippy_lints/src/methods/option_map_or_none.rs @@ -14,10 +14,7 @@ use super::RESULT_MAP_OR_INTO_OPTION; // The expression inside a closure may or may not have surrounding braces // which causes problems when generating a suggestion. -fn reduce_unit_expression<'a>( - cx: &LateContext<'_>, - expr: &'a hir::Expr<'_>, -) -> Option<(&'a hir::Expr<'a>, &'a [hir::Expr<'a>])> { +fn reduce_unit_expression<'a>(expr: &'a hir::Expr<'_>) -> Option<(&'a hir::Expr<'a>, &'a [hir::Expr<'a>])> { match expr.kind { hir::ExprKind::Call(func, arg_char) => Some((func, arg_char)), hir::ExprKind::Block(block, _) => { @@ -25,7 +22,7 @@ fn reduce_unit_expression<'a>( (&[], Some(inner_expr)) => { // If block only contains an expression, // reduce `|x| { x + 1 }` to `|x| x + 1` - reduce_unit_expression(cx, inner_expr) + reduce_unit_expression(inner_expr) }, _ => None, } @@ -77,7 +74,7 @@ pub(super) fn check<'tcx>( if let hir::ExprKind::Closure(_, _, id, span, _) = map_arg.kind; let arg_snippet = snippet(cx, span, ".."); let body = cx.tcx.hir().body(id); - if let Some((func, [arg_char])) = reduce_unit_expression(cx, &body.value); + if let Some((func, [arg_char])) = reduce_unit_expression(&body.value); if let Some(id) = path_def_id(cx, func).and_then(|ctor_id| cx.tcx.parent(ctor_id)); if Some(id) == cx.tcx.lang_items().option_some_variant(); then { diff --git a/clippy_lints/src/methods/unnecessary_filter_map.rs b/clippy_lints/src/methods/unnecessary_filter_map.rs index a307e33875e..2fda254ca98 100644 --- a/clippy_lints/src/methods/unnecessary_filter_map.rs +++ b/clippy_lints/src/methods/unnecessary_filter_map.rs @@ -1,4 +1,6 @@ +use super::utils::clone_or_copy_needed; use clippy_utils::diagnostics::span_lint; +use clippy_utils::ty::is_copy; use clippy_utils::usage::mutated_variables; use clippy_utils::{is_lang_ctor, is_trait_method, path_to_local_id}; use rustc_hir as hir; @@ -9,8 +11,9 @@ use rustc_middle::ty; use rustc_span::sym; use super::UNNECESSARY_FILTER_MAP; +use super::UNNECESSARY_FIND_MAP; -pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr<'_>) { +pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr<'_>, name: &str) { if !is_trait_method(cx, expr, sym::Iterator) { return; } @@ -20,6 +23,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr< let arg_id = body.params[0].pat.hir_id; let mutates_arg = mutated_variables(&body.value, cx).map_or(true, |used_mutably| used_mutably.contains(&arg_id)); + let (clone_or_copy_needed, _) = clone_or_copy_needed(cx, body.params[0].pat, &body.value); let (mut found_mapping, mut found_filtering) = check_expression(cx, arg_id, &body.value); @@ -28,13 +32,15 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr< found_mapping |= return_visitor.found_mapping; found_filtering |= return_visitor.found_filtering; + let in_ty = cx.typeck_results().node_type(body.params[0].hir_id); let sugg = if !found_filtering { - "map" - } else if !found_mapping && !mutates_arg { - let in_ty = cx.typeck_results().node_type(body.params[0].hir_id); + if name == "filter_map" { "map" } else { "map(..).next()" } + } else if !found_mapping && !mutates_arg && (!clone_or_copy_needed || is_copy(cx, in_ty)) { match cx.typeck_results().expr_ty(&body.value).kind() { - ty::Adt(adt, subst) if cx.tcx.is_diagnostic_item(sym::Option, adt.did()) && in_ty == subst.type_at(0) => { - "filter" + ty::Adt(adt, subst) + if cx.tcx.is_diagnostic_item(sym::Option, adt.did()) && in_ty == subst.type_at(0) => + { + if name == "filter_map" { "filter" } else { "find" } }, _ => return, } @@ -43,9 +49,13 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, arg: &hir::Expr< }; span_lint( cx, - UNNECESSARY_FILTER_MAP, + if name == "filter_map" { + UNNECESSARY_FILTER_MAP + } else { + UNNECESSARY_FIND_MAP + }, expr.span, - &format!("this `.filter_map` can be written more simply using `.{}`", sugg), + &format!("this `.{}` can be written more simply using `.{}`", name, sugg), ); } } diff --git a/clippy_lints/src/methods/unnecessary_iter_cloned.rs b/clippy_lints/src/methods/unnecessary_iter_cloned.rs index 65e94c5f44a..7a39557ad57 100644 --- a/clippy_lints/src/methods/unnecessary_iter_cloned.rs +++ b/clippy_lints/src/methods/unnecessary_iter_cloned.rs @@ -1,14 +1,12 @@ +use super::utils::clone_or_copy_needed; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::ForLoop; use clippy_utils::source::snippet_opt; use clippy_utils::ty::{get_associated_type, get_iterator_item_ty, implements_trait}; -use clippy_utils::{fn_def_id, get_parent_expr, path_to_local_id, usage}; +use clippy_utils::{fn_def_id, get_parent_expr}; use rustc_errors::Applicability; -use rustc_hir::intravisit::{walk_expr, Visitor}; -use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind, HirId, LangItem, Mutability, Pat}; +use rustc_hir::{def_id::DefId, Expr, ExprKind, LangItem}; use rustc_lint::LateContext; -use rustc_middle::hir::nested_filter; -use rustc_middle::ty; use rustc_span::{sym, Symbol}; use super::UNNECESSARY_TO_OWNED; @@ -100,89 +98,6 @@ pub fn check_for_loop_iter( false } -/// The core logic of `check_for_loop_iter` above, this function wraps a use of -/// `CloneOrCopyVisitor`. -fn clone_or_copy_needed<'tcx>( - cx: &LateContext<'tcx>, - pat: &Pat<'tcx>, - body: &'tcx Expr<'tcx>, -) -> (bool, Vec<&'tcx Expr<'tcx>>) { - let mut visitor = CloneOrCopyVisitor { - cx, - binding_hir_ids: pat_bindings(pat), - clone_or_copy_needed: false, - addr_of_exprs: Vec::new(), - }; - visitor.visit_expr(body); - (visitor.clone_or_copy_needed, visitor.addr_of_exprs) -} - -/// Returns a vector of all `HirId`s bound by the pattern. -fn pat_bindings(pat: &Pat<'_>) -> Vec { - let mut collector = usage::ParamBindingIdCollector { - binding_hir_ids: Vec::new(), - }; - collector.visit_pat(pat); - collector.binding_hir_ids -} - -/// `clone_or_copy_needed` will be false when `CloneOrCopyVisitor` is done visiting if the only -/// operations performed on `binding_hir_ids` are: -/// * to take non-mutable references to them -/// * to use them as non-mutable `&self` in method calls -/// If any of `binding_hir_ids` is used in any other way, then `clone_or_copy_needed` will be true -/// when `CloneOrCopyVisitor` is done visiting. -struct CloneOrCopyVisitor<'cx, 'tcx> { - cx: &'cx LateContext<'tcx>, - binding_hir_ids: Vec, - clone_or_copy_needed: bool, - addr_of_exprs: Vec<&'tcx Expr<'tcx>>, -} - -impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> { - type NestedFilter = nested_filter::OnlyBodies; - - fn nested_visit_map(&mut self) -> Self::Map { - self.cx.tcx.hir() - } - - fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { - walk_expr(self, expr); - if self.is_binding(expr) { - if let Some(parent) = get_parent_expr(self.cx, expr) { - match parent.kind { - ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => { - self.addr_of_exprs.push(parent); - return; - }, - ExprKind::MethodCall(_, args, _) => { - if_chain! { - if args.iter().skip(1).all(|arg| !self.is_binding(arg)); - if let Some(method_def_id) = self.cx.typeck_results().type_dependent_def_id(parent.hir_id); - let method_ty = self.cx.tcx.type_of(method_def_id); - let self_ty = method_ty.fn_sig(self.cx.tcx).input(0).skip_binder(); - if matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Not)); - then { - return; - } - } - }, - _ => {}, - } - } - self.clone_or_copy_needed = true; - } - } -} - -impl<'cx, 'tcx> CloneOrCopyVisitor<'cx, 'tcx> { - fn is_binding(&self, expr: &Expr<'tcx>) -> bool { - self.binding_hir_ids - .iter() - .any(|hir_id| path_to_local_id(expr, *hir_id)) - } -} - /// Returns true if the named method is `IntoIterator::into_iter`. pub fn is_into_iter(cx: &LateContext<'_>, callee_def_id: DefId) -> bool { cx.tcx.lang_items().require(LangItem::IntoIterIntoIter) == Ok(callee_def_id) diff --git a/clippy_lints/src/methods/utils.rs b/clippy_lints/src/methods/utils.rs index 63c3273bd68..3015531e843 100644 --- a/clippy_lints/src/methods/utils.rs +++ b/clippy_lints/src/methods/utils.rs @@ -1,10 +1,14 @@ use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{get_parent_expr, path_to_local_id, usage}; use if_chain::if_chain; use rustc_ast::ast; use rustc_errors::Applicability; use rustc_hir as hir; +use rustc_hir::intravisit::{walk_expr, Visitor}; +use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, Mutability, Pat}; use rustc_lint::LateContext; +use rustc_middle::hir::nested_filter; use rustc_middle::ty::{self, Ty}; use rustc_span::symbol::sym; @@ -79,3 +83,86 @@ pub(super) fn get_hint_if_single_char_arg( } } } + +/// The core logic of `check_for_loop_iter` in `unnecessary_iter_cloned.rs`, this function wraps a +/// use of `CloneOrCopyVisitor`. +pub(super) fn clone_or_copy_needed<'tcx>( + cx: &LateContext<'tcx>, + pat: &Pat<'tcx>, + body: &'tcx Expr<'tcx>, +) -> (bool, Vec<&'tcx Expr<'tcx>>) { + let mut visitor = CloneOrCopyVisitor { + cx, + binding_hir_ids: pat_bindings(pat), + clone_or_copy_needed: false, + addr_of_exprs: Vec::new(), + }; + visitor.visit_expr(body); + (visitor.clone_or_copy_needed, visitor.addr_of_exprs) +} + +/// Returns a vector of all `HirId`s bound by the pattern. +fn pat_bindings(pat: &Pat<'_>) -> Vec { + let mut collector = usage::ParamBindingIdCollector { + binding_hir_ids: Vec::new(), + }; + collector.visit_pat(pat); + collector.binding_hir_ids +} + +/// `clone_or_copy_needed` will be false when `CloneOrCopyVisitor` is done visiting if the only +/// operations performed on `binding_hir_ids` are: +/// * to take non-mutable references to them +/// * to use them as non-mutable `&self` in method calls +/// If any of `binding_hir_ids` is used in any other way, then `clone_or_copy_needed` will be true +/// when `CloneOrCopyVisitor` is done visiting. +struct CloneOrCopyVisitor<'cx, 'tcx> { + cx: &'cx LateContext<'tcx>, + binding_hir_ids: Vec, + clone_or_copy_needed: bool, + addr_of_exprs: Vec<&'tcx Expr<'tcx>>, +} + +impl<'cx, 'tcx> Visitor<'tcx> for CloneOrCopyVisitor<'cx, 'tcx> { + type NestedFilter = nested_filter::OnlyBodies; + + fn nested_visit_map(&mut self) -> Self::Map { + self.cx.tcx.hir() + } + + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + walk_expr(self, expr); + if self.is_binding(expr) { + if let Some(parent) = get_parent_expr(self.cx, expr) { + match parent.kind { + ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) => { + self.addr_of_exprs.push(parent); + return; + }, + ExprKind::MethodCall(_, args, _) => { + if_chain! { + if args.iter().skip(1).all(|arg| !self.is_binding(arg)); + if let Some(method_def_id) = self.cx.typeck_results().type_dependent_def_id(parent.hir_id); + let method_ty = self.cx.tcx.type_of(method_def_id); + let self_ty = method_ty.fn_sig(self.cx.tcx).input(0).skip_binder(); + if matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Not)); + then { + return; + } + } + }, + _ => {}, + } + } + self.clone_or_copy_needed = true; + } + } +} + +impl<'cx, 'tcx> CloneOrCopyVisitor<'cx, 'tcx> { + fn is_binding(&self, expr: &Expr<'tcx>) -> bool { + self.binding_hir_ids + .iter() + .any(|hir_id| path_to_local_id(expr, *hir_id)) + } +} diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index a57dc2b2798..5eb7b0f0521 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -199,7 +199,12 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue { let sugg = |diag: &mut Diagnostic| { if let ty::Adt(def, ..) = ty.kind() { if let Some(span) = cx.tcx.hir().span_if_local(def.did()) { - if can_type_implement_copy(cx.tcx, cx.param_env, ty, traits::ObligationCause::dummy_with_span(span)).is_ok() { + if can_type_implement_copy( + cx.tcx, + cx.param_env, + ty, + traits::ObligationCause::dummy_with_span(span), + ).is_ok() { diag.span_help(span, "consider marking this type as `Copy`"); } } diff --git a/clippy_lints/src/only_used_in_recursion.rs b/clippy_lints/src/only_used_in_recursion.rs new file mode 100644 index 00000000000..b828d9334ee --- /dev/null +++ b/clippy_lints/src/only_used_in_recursion.rs @@ -0,0 +1,668 @@ +use std::collections::VecDeque; + +use clippy_utils::diagnostics::span_lint_and_sugg; +use itertools::{izip, Itertools}; +use rustc_ast::{walk_list, Label, Mutability}; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_errors::Applicability; +use rustc_hir::def::{DefKind, Res}; +use rustc_hir::def_id::DefId; +use rustc_hir::definitions::{DefPathData, DisambiguatedDefPathData}; +use rustc_hir::intravisit::{walk_expr, FnKind, Visitor}; +use rustc_hir::{ + Arm, Block, Body, Expr, ExprKind, Guard, HirId, ImplicitSelfKind, Let, Local, Pat, PatKind, Path, PathSegment, + QPath, Stmt, StmtKind, TyKind, UnOp, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_middle::ty::{Ty, TyCtxt, TypeckResults}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::kw; +use rustc_span::symbol::Ident; +use rustc_span::Span; + +declare_clippy_lint! { + /// ### What it does + /// Checks for arguments that are only used in recursion with no side-effects. + /// + /// ### Why is this bad? + /// It could contain a useless calculation and can make function simpler. + /// + /// The arguments can be involved in calculations and assignments but as long as + /// the calculations have no side-effects (function calls or mutating dereference) + /// and the assigned variables are also only in recursion, it is useless. + /// + /// ### Known problems + /// In some cases, this would not catch all useless arguments. + /// + /// ```rust + /// fn foo(a: usize, b: usize) -> usize { + /// let f = |x| x + 1; + /// + /// if a == 0 { + /// 1 + /// } else { + /// foo(a - 1, f(b)) + /// } + /// } + /// ``` + /// + /// For example, the argument `b` is only used in recursion, but the lint would not catch it. + /// + /// List of some examples that can not be caught: + /// - binary operation of non-primitive types + /// - closure usage + /// - some `break` relative operations + /// - struct pattern binding + /// + /// Also, when you recurse the function name with path segments, it is not possible to detect. + /// + /// ### Example + /// ```rust + /// fn f(a: usize, b: usize) -> usize { + /// if a == 0 { + /// 1 + /// } else { + /// f(a - 1, b + 1) + /// } + /// } + /// # fn main() { + /// # print!("{}", f(1, 1)); + /// # } + /// ``` + /// Use instead: + /// ```rust + /// fn f(a: usize) -> usize { + /// if a == 0 { + /// 1 + /// } else { + /// f(a - 1) + /// } + /// } + /// # fn main() { + /// # print!("{}", f(1)); + /// # } + /// ``` + #[clippy::version = "1.60.0"] + pub ONLY_USED_IN_RECURSION, + complexity, + "arguments that is only used in recursion can be removed" +} +declare_lint_pass!(OnlyUsedInRecursion => [ONLY_USED_IN_RECURSION]); + +impl<'tcx> LateLintPass<'tcx> for OnlyUsedInRecursion { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + kind: FnKind<'tcx>, + decl: &'tcx rustc_hir::FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + _: Span, + id: HirId, + ) { + if let FnKind::ItemFn(ident, ..) | FnKind::Method(ident, ..) = kind { + let def_id = id.owner.to_def_id(); + let data = cx.tcx.def_path(def_id).data; + + if data.len() > 1 { + match data.get(data.len() - 2) { + Some(DisambiguatedDefPathData { + data: DefPathData::Impl, + disambiguator, + }) if *disambiguator != 0 => return, + _ => {}, + } + } + + let has_self = !matches!(decl.implicit_self, ImplicitSelfKind::None); + + let ty_res = cx.typeck_results(); + let param_span = body + .params + .iter() + .flat_map(|param| { + let mut v = Vec::new(); + param.pat.each_binding(|_, hir_id, span, ident| { + v.push((hir_id, span, ident)); + }); + v + }) + .skip(if has_self { 1 } else { 0 }) + .filter(|(_, _, ident)| !ident.name.as_str().starts_with('_')) + .collect_vec(); + + let params = body.params.iter().map(|param| param.pat).collect(); + + let mut visitor = SideEffectVisit { + graph: FxHashMap::default(), + has_side_effect: FxHashSet::default(), + ret_vars: Vec::new(), + contains_side_effect: false, + break_vars: FxHashMap::default(), + params, + fn_ident: ident, + fn_def_id: def_id, + is_method: matches!(kind, FnKind::Method(..)), + has_self, + ty_res, + ty_ctx: cx.tcx, + }; + + visitor.visit_expr(&body.value); + let vars = std::mem::take(&mut visitor.ret_vars); + // this would set the return variables to side effect + visitor.add_side_effect(vars); + + let mut queue = visitor.has_side_effect.iter().copied().collect::>(); + + // a simple BFS to check all the variables that have side effect + while let Some(id) = queue.pop_front() { + if let Some(next) = visitor.graph.get(&id) { + for i in next { + if !visitor.has_side_effect.contains(i) { + visitor.has_side_effect.insert(*i); + queue.push_back(*i); + } + } + } + } + + for (id, span, ident) in param_span { + // if the variable is not used in recursion, it would be marked as unused + if !visitor.has_side_effect.contains(&id) { + let mut queue = VecDeque::new(); + let mut visited = FxHashSet::default(); + + queue.push_back(id); + + // a simple BFS to check the graph can reach to itself + // if it can't, it means the variable is never used in recursion + while let Some(id) = queue.pop_front() { + if let Some(next) = visitor.graph.get(&id) { + for i in next { + if !visited.contains(i) { + visited.insert(id); + queue.push_back(*i); + } + } + } + } + + if visited.contains(&id) { + span_lint_and_sugg( + cx, + ONLY_USED_IN_RECURSION, + span, + "parameter is only used in recursion", + "if this is intentional, prefix with an underscore", + format!("_{}", ident.name.as_str()), + Applicability::MaybeIncorrect, + ); + } + } + } + } + } +} + +pub fn is_primitive(ty: Ty<'_>) -> bool { + match ty.kind() { + ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str => true, + ty::Ref(_, t, _) => is_primitive(*t), + _ => false, + } +} + +pub fn is_array(ty: Ty<'_>) -> bool { + match ty.kind() { + ty::Array(..) | ty::Slice(..) => true, + ty::Ref(_, t, _) => is_array(*t), + _ => false, + } +} + +/// This builds the graph of side effect. +/// The edge `a -> b` means if `a` has side effect, `b` will have side effect. +/// +/// There are some exmaple in following code: +/// ```rust, ignore +/// let b = 1; +/// let a = b; // a -> b +/// let (c, d) = (a, b); // c -> b, d -> b +/// +/// let e = if a == 0 { // e -> a +/// c // e -> c +/// } else { +/// d // e -> d +/// }; +/// ``` +pub struct SideEffectVisit<'tcx> { + graph: FxHashMap>, + has_side_effect: FxHashSet, + // bool for if the variable was dereferenced from mutable reference + ret_vars: Vec<(HirId, bool)>, + contains_side_effect: bool, + // break label + break_vars: FxHashMap>, + params: Vec<&'tcx Pat<'tcx>>, + fn_ident: Ident, + fn_def_id: DefId, + is_method: bool, + has_self: bool, + ty_res: &'tcx TypeckResults<'tcx>, + ty_ctx: TyCtxt<'tcx>, +} + +impl<'tcx> Visitor<'tcx> for SideEffectVisit<'tcx> { + fn visit_block(&mut self, b: &'tcx Block<'tcx>) { + b.stmts.iter().for_each(|stmt| { + self.visit_stmt(stmt); + self.ret_vars.clear(); + }); + walk_list!(self, visit_expr, b.expr); + } + + fn visit_stmt(&mut self, s: &'tcx Stmt<'tcx>) { + match s.kind { + StmtKind::Local(Local { + pat, init: Some(init), .. + }) => { + self.visit_pat_expr(pat, init, false); + self.ret_vars.clear(); + }, + StmtKind::Item(i) => { + let item = self.ty_ctx.hir().item(i); + self.visit_item(item); + self.ret_vars.clear(); + }, + StmtKind::Expr(e) | StmtKind::Semi(e) => { + self.visit_expr(e); + self.ret_vars.clear(); + }, + StmtKind::Local(_) => {}, + } + } + + fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) { + match ex.kind { + ExprKind::Array(exprs) | ExprKind::Tup(exprs) => { + self.ret_vars = exprs + .iter() + .flat_map(|expr| { + self.visit_expr(expr); + std::mem::take(&mut self.ret_vars) + }) + .collect(); + }, + ExprKind::Call(callee, args) => self.visit_fn(callee, args), + ExprKind::MethodCall(path, args, _) => self.visit_method_call(path, args), + ExprKind::Binary(_, lhs, rhs) => { + self.visit_bin_op(lhs, rhs); + }, + ExprKind::Unary(op, expr) => self.visit_un_op(op, expr), + ExprKind::Let(Let { pat, init, .. }) => self.visit_pat_expr(pat, init, false), + ExprKind::If(bind, then_expr, else_expr) => { + self.visit_if(bind, then_expr, else_expr); + }, + ExprKind::Match(expr, arms, _) => self.visit_match(expr, arms), + // since analysing the closure is not easy, just set all variables in it to side-effect + ExprKind::Closure(_, _, body_id, _, _) => { + let body = self.ty_ctx.hir().body(body_id); + self.visit_body(body); + let vars = std::mem::take(&mut self.ret_vars); + self.add_side_effect(vars); + }, + ExprKind::Loop(block, label, _, _) | ExprKind::Block(block, label) => { + self.visit_block_label(block, label); + }, + ExprKind::Assign(bind, expr, _) => { + self.visit_assign(bind, expr); + }, + ExprKind::AssignOp(_, bind, expr) => { + self.visit_assign(bind, expr); + self.visit_bin_op(bind, expr); + }, + ExprKind::Field(expr, _) => { + self.visit_expr(expr); + if matches!(self.ty_res.expr_ty(expr).kind(), ty::Ref(_, _, Mutability::Mut)) { + self.ret_vars.iter_mut().for_each(|(_, b)| *b = true); + } + }, + ExprKind::Index(expr, index) => { + self.visit_expr(expr); + let mut vars = std::mem::take(&mut self.ret_vars); + self.visit_expr(index); + self.ret_vars.append(&mut vars); + + if !is_array(self.ty_res.expr_ty(expr)) { + self.add_side_effect(self.ret_vars.clone()); + } else if matches!(self.ty_res.expr_ty(expr).kind(), ty::Ref(_, _, Mutability::Mut)) { + self.ret_vars.iter_mut().for_each(|(_, b)| *b = true); + } + }, + ExprKind::Break(dest, Some(expr)) => { + self.visit_expr(expr); + if let Some(label) = dest.label { + self.break_vars + .entry(label.ident) + .or_insert(Vec::new()) + .append(&mut self.ret_vars); + } + self.contains_side_effect = true; + }, + ExprKind::Ret(Some(expr)) => { + self.visit_expr(expr); + let vars = std::mem::take(&mut self.ret_vars); + self.add_side_effect(vars); + self.contains_side_effect = true; + }, + ExprKind::Break(_, None) | ExprKind::Continue(_) | ExprKind::Ret(None) => { + self.contains_side_effect = true; + }, + ExprKind::Struct(_, exprs, expr) => { + let mut ret_vars = exprs + .iter() + .flat_map(|field| { + self.visit_expr(field.expr); + std::mem::take(&mut self.ret_vars) + }) + .collect(); + + walk_list!(self, visit_expr, expr); + self.ret_vars.append(&mut ret_vars); + }, + _ => walk_expr(self, ex), + } + } + + fn visit_path(&mut self, path: &'tcx Path<'tcx>, _id: HirId) { + if let Res::Local(id) = path.res { + self.ret_vars.push((id, false)); + } + } +} + +impl<'tcx> SideEffectVisit<'tcx> { + fn visit_assign(&mut self, lhs: &'tcx Expr<'tcx>, rhs: &'tcx Expr<'tcx>) { + // Just support array and tuple unwrapping for now. + // + // ex) `(a, b) = (c, d);` + // The graph would look like this: + // a -> c + // b -> d + // + // This would minimize the connection of the side-effect graph. + match (&lhs.kind, &rhs.kind) { + (ExprKind::Array(lhs), ExprKind::Array(rhs)) | (ExprKind::Tup(lhs), ExprKind::Tup(rhs)) => { + // if not, it is a compile error + debug_assert!(lhs.len() == rhs.len()); + izip!(*lhs, *rhs).for_each(|(lhs, rhs)| self.visit_assign(lhs, rhs)); + }, + // in other assigns, we have to connect all each other + // because they can be connected somehow + _ => { + self.visit_expr(lhs); + let lhs_vars = std::mem::take(&mut self.ret_vars); + self.visit_expr(rhs); + let rhs_vars = std::mem::take(&mut self.ret_vars); + self.connect_assign(&lhs_vars, &rhs_vars, false); + }, + } + } + + fn visit_block_label(&mut self, block: &'tcx Block<'tcx>, label: Option