diff --git a/CHANGELOG.md b/CHANGELOG.md index 72fe6ae31..59a3dc651 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1050,7 +1050,7 @@ Released 2020-11-19 [#5913](https://github.com/rust-lang/rust-clippy/pull/5913) * Add example of false positive to [`ptr_arg`] docs. [#5885](https://github.com/rust-lang/rust-clippy/pull/5885) -* [`box_vec`], [`vec_box`] and [`borrowed_box`]: add link to the documentation of `Box` +* [`box_vec`](https://rust-lang.github.io/rust-clippy/master/index.html#box_collection), [`vec_box`] and [`borrowed_box`]: add link to the documentation of `Box` [#6023](https://github.com/rust-lang/rust-clippy/pull/6023) ## Rust 1.47 @@ -2570,7 +2570,7 @@ Released 2018-09-13 [`bool_comparison`]: https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison [`borrow_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const [`borrowed_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#borrowed_box -[`box_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_vec +[`box_collection`]: https://rust-lang.github.io/rust-clippy/master/index.html#box_collection [`boxed_local`]: https://rust-lang.github.io/rust-clippy/master/index.html#boxed_local [`branches_sharing_code`]: https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code [`builtin_type_shadow`]: https://rust-lang.github.io/rust-clippy/master/index.html#builtin_type_shadow diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 59d87aa96..c7e59cb16 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -956,7 +956,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: transmuting_null::TRANSMUTING_NULL, try_err::TRY_ERR, types::BORROWED_BOX, - types::BOX_VEC, + types::BOX_COLLECTION, types::LINKEDLIST, types::OPTION_OPTION, types::RC_BUFFER, @@ -1454,7 +1454,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(transmuting_null::TRANSMUTING_NULL), LintId::of(try_err::TRY_ERR), LintId::of(types::BORROWED_BOX), - LintId::of(types::BOX_VEC), + LintId::of(types::BOX_COLLECTION), LintId::of(types::REDUNDANT_ALLOCATION), LintId::of(types::TYPE_COMPLEXITY), LintId::of(types::VEC_BOX), @@ -1792,7 +1792,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(redundant_clone::REDUNDANT_CLONE), LintId::of(slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), LintId::of(stable_sort_primitive::STABLE_SORT_PRIMITIVE), - LintId::of(types::BOX_VEC), + LintId::of(types::BOX_COLLECTION), LintId::of(types::REDUNDANT_ALLOCATION), LintId::of(vec::USELESS_VEC), LintId::of(vec_init_then_push::VEC_INIT_THEN_PUSH), @@ -2193,6 +2193,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) { ls.register_renamed("clippy::cyclomatic_complexity", "clippy::cognitive_complexity"); ls.register_renamed("clippy::const_static_lifetime", "clippy::redundant_static_lifetimes"); ls.register_renamed("clippy::option_and_then_some", "clippy::bind_instead_of_map"); + ls.register_renamed("clippy::box_vec", "clippy::box_collection"); ls.register_renamed("clippy::block_in_if_condition_expr", "clippy::blocks_in_if_conditions"); ls.register_renamed("clippy::block_in_if_condition_stmt", "clippy::blocks_in_if_conditions"); ls.register_renamed("clippy::option_map_unwrap_or", "clippy::map_unwrap_or"); diff --git a/clippy_lints/src/types/box_collection.rs b/clippy_lints/src/types/box_collection.rs new file mode 100644 index 000000000..718aea471 --- /dev/null +++ b/clippy_lints/src/types/box_collection.rs @@ -0,0 +1,50 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::is_ty_param_diagnostic_item; +use rustc_hir::{self as hir, def_id::DefId, QPath}; +use rustc_lint::LateContext; +use rustc_span::symbol::sym; + +use super::BOX_COLLECTION; + +pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, qpath: &QPath<'_>, def_id: DefId) -> bool { + if_chain! { + if Some(def_id) == cx.tcx.lang_items().owned_box(); + if let Some(item_type) = get_std_collection(cx, qpath); + then { + let generic = if item_type == "String" { + "" + } else { + "<..>" + }; + span_lint_and_help( + cx, + BOX_COLLECTION, + hir_ty.span, + &format!( + "you seem to be trying to use `Box<{outer}{generic}>`. Consider using just `{outer}{generic}`", + outer=item_type, + generic = generic), + None, + &format!( + "`{outer}{generic}` is already on the heap, `Box<{outer}{generic}>` makes an extra allocation", + outer=item_type, + generic = generic) + ); + true + } else { + false + } + } +} + +fn get_std_collection(cx: &LateContext<'_>, qpath: &QPath<'_>) -> Option { + if is_ty_param_diagnostic_item(cx, qpath, sym::vec_type).is_some() { + Some(String::from("Vec")) + } else if is_ty_param_diagnostic_item(cx, qpath, sym::string_type).is_some() { + Some(String::from("String")) + } else if is_ty_param_diagnostic_item(cx, qpath, sym::hashmap_type).is_some() { + Some(String::from("HashMap")) + } else { + None + } +} diff --git a/clippy_lints/src/types/box_vec.rs b/clippy_lints/src/types/box_vec.rs deleted file mode 100644 index d8b195345..000000000 --- a/clippy_lints/src/types/box_vec.rs +++ /dev/null @@ -1,25 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_help; -use clippy_utils::is_ty_param_diagnostic_item; -use rustc_hir::{self as hir, def_id::DefId, QPath}; -use rustc_lint::LateContext; -use rustc_span::symbol::sym; - -use super::BOX_VEC; - -pub(super) fn check(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>, qpath: &QPath<'_>, def_id: DefId) -> bool { - if Some(def_id) == cx.tcx.lang_items().owned_box() - && is_ty_param_diagnostic_item(cx, qpath, sym::vec_type).is_some() - { - span_lint_and_help( - cx, - BOX_VEC, - hir_ty.span, - "you seem to be trying to use `Box>`. Consider using just `Vec`", - None, - "`Vec` is already on the heap, `Box>` makes an extra allocation", - ); - true - } else { - false - } -} diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index 9588de845..bbe07db53 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -1,5 +1,5 @@ mod borrowed_box; -mod box_vec; +mod box_collection; mod linked_list; mod option_option; mod rc_buffer; @@ -21,12 +21,12 @@ use rustc_span::source_map::Span; declare_clippy_lint! { /// ### What it does - /// Checks for use of `Box>` anywhere in the code. + /// Checks for use of `Box` where T is a collection such as Vec anywhere in the code. /// Check the [Box documentation](https://doc.rust-lang.org/std/boxed/index.html) for more information. /// /// ### Why is this bad? - /// `Vec` already keeps its contents in a separate area on - /// the heap. So if you `Box` it, you just add another level of indirection + /// Collections already keeps their contents in a separate area on + /// the heap. So if you `Box` them, you just add another level of indirection /// without any benefit whatsoever. /// /// ### Example @@ -43,7 +43,7 @@ declare_clippy_lint! { /// values: Vec, /// } /// ``` - pub BOX_VEC, + pub BOX_COLLECTION, perf, "usage of `Box>`, vector elements are already on the heap" } @@ -298,7 +298,7 @@ pub struct Types { avoid_breaking_exported_api: bool, } -impl_lint_pass!(Types => [BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]); +impl_lint_pass!(Types => [BOX_COLLECTION, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX, REDUNDANT_ALLOCATION, RC_BUFFER, RC_MUTEX, TYPE_COMPLEXITY]); impl<'tcx> LateLintPass<'tcx> for Types { fn check_fn(&mut self, cx: &LateContext<'_>, _: FnKind<'_>, decl: &FnDecl<'_>, _: &Body<'_>, _: Span, id: HirId) { @@ -447,7 +447,7 @@ impl Types { // in `clippy_lints::utils::conf.rs` let mut triggered = false; - triggered |= box_vec::check(cx, hir_ty, qpath, def_id); + triggered |= box_collection::check(cx, hir_ty, qpath, def_id); triggered |= redundant_allocation::check(cx, hir_ty, qpath, def_id); triggered |= rc_buffer::check(cx, hir_ty, qpath, def_id); triggered |= vec_box::check(cx, hir_ty, qpath, def_id, self.vec_box_size_threshold); diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 8b087507b..1e0447239 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -136,7 +136,7 @@ macro_rules! define_Conf { } define_Conf! { - /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_VEC, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX. + /// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION, BOX_COLLECTION, REDUNDANT_ALLOCATION, RC_BUFFER, VEC_BOX, OPTION_OPTION, LINKEDLIST, RC_MUTEX. /// /// Suppress lints whenever the suggested change would cause breakage for other crates. (avoid_breaking_exported_api: bool = true), diff --git a/tests/ui/box_vec.rs b/tests/ui/box_collection.rs similarity index 86% rename from tests/ui/box_vec.rs rename to tests/ui/box_collection.rs index 1d6366972..e00f061f2 100644 --- a/tests/ui/box_vec.rs +++ b/tests/ui/box_collection.rs @@ -6,6 +6,8 @@ unused )] +use std::collections::HashMap; + macro_rules! boxit { ($init:expr, $x:ty) => { let _: Box<$x> = Box::new($init); @@ -15,6 +17,7 @@ macro_rules! boxit { fn test_macro() { boxit!(Vec::new(), Vec); } + fn test(foo: Box>) {} fn test2(foo: Box)>) { @@ -22,6 +25,10 @@ fn test2(foo: Box)>) { foo(vec![1, 2, 3]) } +fn test3(foo: Box) {} + +fn test4(foo: Box>) {} + fn test_local_not_linted() { let _: Box>; } @@ -29,6 +36,7 @@ fn test_local_not_linted() { // All of these test should be allowed because they are part of the // public api and `avoid_breaking_exported_api` is `false` by default. pub fn pub_test(foo: Box>) {} + pub fn pub_test_ret() -> Box> { Box::new(Vec::new()) } diff --git a/tests/ui/box_collection.stderr b/tests/ui/box_collection.stderr new file mode 100644 index 000000000..6de85d05a --- /dev/null +++ b/tests/ui/box_collection.stderr @@ -0,0 +1,27 @@ +error: you seem to be trying to use `Box>`. Consider using just `Vec<..>` + --> $DIR/box_collection.rs:21:14 + | +LL | fn test(foo: Box>) {} + | ^^^^^^^^^^^^^^ + | + = note: `-D clippy::box-collection` implied by `-D warnings` + = help: `Vec<..>` is already on the heap, `Box>` makes an extra allocation + +error: you seem to be trying to use `Box`. Consider using just `String` + --> $DIR/box_collection.rs:28:15 + | +LL | fn test3(foo: Box) {} + | ^^^^^^^^^^^ + | + = help: `String` is already on the heap, `Box` makes an extra allocation + +error: you seem to be trying to use `Box>`. Consider using just `HashMap<..>` + --> $DIR/box_collection.rs:30:15 + | +LL | fn test4(foo: Box>) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: `HashMap<..>` is already on the heap, `Box>` makes an extra allocation + +error: aborting due to 3 previous errors + diff --git a/tests/ui/box_vec.stderr b/tests/ui/box_vec.stderr deleted file mode 100644 index 58c1f13fb..000000000 --- a/tests/ui/box_vec.stderr +++ /dev/null @@ -1,11 +0,0 @@ -error: you seem to be trying to use `Box>`. Consider using just `Vec` - --> $DIR/box_vec.rs:18:14 - | -LL | fn test(foo: Box>) {} - | ^^^^^^^^^^^^^^ - | - = note: `-D clippy::box-vec` implied by `-D warnings` - = help: `Vec` is already on the heap, `Box>` makes an extra allocation - -error: aborting due to previous error -