From ab070508be3fbf02619f5f109ece829243a751e8 Mon Sep 17 00:00:00 2001 From: Kampfkarren Date: Thu, 13 Dec 2018 07:43:13 -0800 Subject: [PATCH 1/6] Lint for Vec> - Closes #3530 --- clippy_lints/src/lib.rs | 2 ++ clippy_lints/src/types.rs | 65 +++++++++++++++++++++++++++++++++-- tests/ui/vec_box_sized.rs | 17 +++++++++ tests/ui/vec_box_sized.stderr | 11 ++++++ 4 files changed, 93 insertions(+), 2 deletions(-) create mode 100644 tests/ui/vec_box_sized.rs create mode 100644 tests/ui/vec_box_sized.stderr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a862d774174..9e3f0a6505d 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -766,6 +766,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { types::UNIT_ARG, types::UNIT_CMP, types::UNNECESSARY_CAST, + types::VEC_BOX_SIZED, unicode::ZERO_WIDTH_SPACE, unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, unused_io_amount::UNUSED_IO_AMOUNT, @@ -931,6 +932,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { types::TYPE_COMPLEXITY, types::UNIT_ARG, types::UNNECESSARY_CAST, + types::VEC_BOX_SIZED, unused_label::UNUSED_LABEL, zero_div_zero::ZERO_DIVIDED_BY_ZERO, ]); diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 820f2fdf32d..b85f21ce970 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -24,7 +24,7 @@ use crate::rustc_typeck::hir_ty_to_ty; use crate::syntax::ast::{FloatTy, IntTy, UintTy}; use crate::syntax::errors::DiagnosticBuilder; -use crate::syntax::source_map::Span; +use crate::syntax::source_map::{DUMMY_SP, Span}; use crate::utils::paths; use crate::utils::{ clip, comparisons, differing_macro_contexts, higher, in_constant, in_macro, int_bits, last_path_segment, @@ -68,6 +68,33 @@ "usage of `Box>`, vector elements are already on the heap" } +/// **What it does:** Checks for use of `Vec>` where T: Sized anywhere in the code. +/// +/// **Why is this bad?** `Vec` already keeps its contents in a separate area on +/// the heap. So if you `Box` its contents, you just add another level of indirection. +/// +/// **Known problems:** Vec> makes sense if T is a large type (see #3530, 1st comment). +/// +/// **Example:** +/// ```rust +/// struct X { +/// values: Vec>, +/// } +/// ``` +/// +/// Better: +/// +/// ```rust +/// struct X { +/// values: Vec, +/// } +/// ``` +declare_clippy_lint! { + pub VEC_BOX_SIZED, + complexity, + "usage of `Vec>` where T: Sized, vector elements are already on the heap" +} + /// **What it does:** Checks for use of `Option>` in function signatures and type /// definitions /// @@ -148,7 +175,7 @@ impl LintPass for TypePass { fn get_lints(&self) -> LintArray { - lint_array!(BOX_VEC, OPTION_OPTION, LINKEDLIST, BORROWED_BOX) + lint_array!(BOX_VEC, VEC_BOX_SIZED, OPTION_OPTION, LINKEDLIST, BORROWED_BOX) } } @@ -238,6 +265,40 @@ fn check_ty(cx: &LateContext<'_, '_>, ast_ty: &hir::Ty, is_local: bool) { ); return; // don't recurse into the type } + } else if match_def_path(cx.tcx, def_id, &paths::VEC) { + if_chain! { + // Get the _ part of Vec<_> + if let Some(ref last) = last_path_segment(qpath).args; + if let Some(ty) = last.args.iter().find_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + GenericArg::Lifetime(_) => None, + }); + // ty is now _ at this point + if let TyKind::Path(ref ty_qpath) = ty.node; + let def = cx.tables.qpath_def(ty_qpath, ty.hir_id); + if let Some(def_id) = opt_def_id(def); + if Some(def_id) == cx.tcx.lang_items().owned_box(); + // At this point, we know ty is Box, now get T + if let Some(ref last) = last_path_segment(ty_qpath).args; + if let Some(ty) = last.args.iter().find_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + GenericArg::Lifetime(_) => None, + }); + if let TyKind::Path(ref ty_qpath) = ty.node; + let def = cx.tables.qpath_def(ty_qpath, ty.hir_id); + if let Some(def_id) = opt_def_id(def); + let boxed_type = cx.tcx.type_of(def_id); + if boxed_type.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env); + then { + span_help_and_lint( + cx, + VEC_BOX_SIZED, + ast_ty.span, + "you seem to be trying to use `Vec>`, but T is Sized. Consider using just `Vec`", + "`Vec` is already on the heap, `Vec>` makes an extra allocation.", + ) + } + } } else if match_def_path(cx.tcx, def_id, &paths::OPTION) { if match_type_parameter(cx, qpath, &paths::OPTION) { span_lint( diff --git a/tests/ui/vec_box_sized.rs b/tests/ui/vec_box_sized.rs new file mode 100644 index 00000000000..d740f95edfe --- /dev/null +++ b/tests/ui/vec_box_sized.rs @@ -0,0 +1,17 @@ +struct SizedStruct { + _a: i32, +} + +struct UnsizedStruct { + _a: [i32], +} + +struct StructWithVecBox { + sized_type: Vec>, +} + +struct StructWithVecBoxButItsUnsized { + unsized_type: Vec>, +} + +fn main() {} diff --git a/tests/ui/vec_box_sized.stderr b/tests/ui/vec_box_sized.stderr new file mode 100644 index 00000000000..80f54b51a40 --- /dev/null +++ b/tests/ui/vec_box_sized.stderr @@ -0,0 +1,11 @@ +error: you seem to be trying to use `Vec>`, but T is Sized. Consider using just `Vec` + --> $DIR/vec_box_sized.rs:10:14 + | +10 | sized_type: Vec>, + | ^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::vec-box-sized` implied by `-D warnings` + = help: `Vec` is already on the heap, `Vec>` makes an extra allocation. + +error: aborting due to previous error + From e5ea5395b971930d12e8be36ef124fa7f115f296 Mon Sep 17 00:00:00 2001 From: Kampfkarren Date: Thu, 13 Dec 2018 09:14:01 -0800 Subject: [PATCH 2/6] Update lint definitions --- CHANGELOG.md | 1 + README.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e691ec9412f..977a4d4a514 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -899,6 +899,7 @@ All notable changes to this project will be documented in this file. [`useless_let_if_seq`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_let_if_seq [`useless_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_transmute [`useless_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_vec +[`vec_box_sized`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_box_sized [`verbose_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_bit_mask [`while_immutable_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_immutable_condition [`while_let_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_loop diff --git a/README.md b/README.md index f42771709fb..5c5d32e4a89 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 290 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 291 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: From 616395f40b54d4340348f6b811d5fe2497761f30 Mon Sep 17 00:00:00 2001 From: Kampfkarren Date: Thu, 13 Dec 2018 09:34:16 -0800 Subject: [PATCH 3/6] Add suggestion for replacement --- clippy_lints/src/types.rs | 8 +++++--- tests/ui/vec_box_sized.stderr | 5 ++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index b85f21ce970..5b652985b3d 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -290,12 +290,14 @@ fn check_ty(cx: &LateContext<'_, '_>, ast_ty: &hir::Ty, is_local: bool) { let boxed_type = cx.tcx.type_of(def_id); if boxed_type.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env); then { - span_help_and_lint( + span_lint_and_sugg( cx, VEC_BOX_SIZED, ast_ty.span, - "you seem to be trying to use `Vec>`, but T is Sized. Consider using just `Vec`", - "`Vec` is already on the heap, `Vec>` makes an extra allocation.", + "you seem to be trying to use `Vec>`, but T is Sized. `Vec` is already on the heap, `Vec>` makes an extra allocation.", + "try", + format!("Vec<{}>", boxed_type), + Applicability::MachineApplicable ) } } diff --git a/tests/ui/vec_box_sized.stderr b/tests/ui/vec_box_sized.stderr index 80f54b51a40..ae7171fdb31 100644 --- a/tests/ui/vec_box_sized.stderr +++ b/tests/ui/vec_box_sized.stderr @@ -1,11 +1,10 @@ -error: you seem to be trying to use `Vec>`, but T is Sized. Consider using just `Vec` +error: you seem to be trying to use `Vec>`, but T is Sized. `Vec` is already on the heap, `Vec>` makes an extra allocation. --> $DIR/vec_box_sized.rs:10:14 | 10 | sized_type: Vec>, - | ^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec` | = note: `-D clippy::vec-box-sized` implied by `-D warnings` - = help: `Vec` is already on the heap, `Vec>` makes an extra allocation. error: aborting due to previous error From 9fc914cf4d6c508d1d07a2ff94ee23d1840eb5e5 Mon Sep 17 00:00:00 2001 From: Kampfkarren Date: Thu, 13 Dec 2018 09:37:00 -0800 Subject: [PATCH 4/6] Remove DUMMY_SP --- clippy_lints/src/types.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 5b652985b3d..fdaa21f6c55 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -24,7 +24,7 @@ use crate::rustc_typeck::hir_ty_to_ty; use crate::syntax::ast::{FloatTy, IntTy, UintTy}; use crate::syntax::errors::DiagnosticBuilder; -use crate::syntax::source_map::{DUMMY_SP, Span}; +use crate::syntax::source_map::Span; use crate::utils::paths; use crate::utils::{ clip, comparisons, differing_macro_contexts, higher, in_constant, in_macro, int_bits, last_path_segment, @@ -288,7 +288,7 @@ fn check_ty(cx: &LateContext<'_, '_>, ast_ty: &hir::Ty, is_local: bool) { let def = cx.tables.qpath_def(ty_qpath, ty.hir_id); if let Some(def_id) = opt_def_id(def); let boxed_type = cx.tcx.type_of(def_id); - if boxed_type.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env); + if boxed_type.is_sized(cx.tcx.at(ty.span), cx.param_env); then { span_lint_and_sugg( cx, From db00c3320f8f0235f2ae022de7ecac65bfd80ff8 Mon Sep 17 00:00:00 2001 From: Kampfkarren Date: Thu, 13 Dec 2018 10:15:56 -0800 Subject: [PATCH 5/6] Remove references to sized for end users --- CHANGELOG.md | 2 +- clippy_lints/src/lib.rs | 4 ++-- clippy_lints/src/types.rs | 13 +++++++------ tests/ui/vec_box_sized.stderr | 4 ++-- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 977a4d4a514..1713fda031f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -899,7 +899,7 @@ All notable changes to this project will be documented in this file. [`useless_let_if_seq`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_let_if_seq [`useless_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_transmute [`useless_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#useless_vec -[`vec_box_sized`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_box_sized +[`vec_box`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_box [`verbose_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_bit_mask [`while_immutable_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_immutable_condition [`while_let_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_loop diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 9e3f0a6505d..f4b5edee84c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -766,7 +766,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { types::UNIT_ARG, types::UNIT_CMP, types::UNNECESSARY_CAST, - types::VEC_BOX_SIZED, + types::VEC_BOX, unicode::ZERO_WIDTH_SPACE, unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, unused_io_amount::UNUSED_IO_AMOUNT, @@ -932,7 +932,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { types::TYPE_COMPLEXITY, types::UNIT_ARG, types::UNNECESSARY_CAST, - types::VEC_BOX_SIZED, + types::VEC_BOX, unused_label::UNUSED_LABEL, zero_div_zero::ZERO_DIVIDED_BY_ZERO, ]); diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index fdaa21f6c55..62e99b92d35 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -90,7 +90,7 @@ /// } /// ``` declare_clippy_lint! { - pub VEC_BOX_SIZED, + pub VEC_BOX, complexity, "usage of `Vec>` where T: Sized, vector elements are already on the heap" } @@ -175,7 +175,7 @@ impl LintPass for TypePass { fn get_lints(&self) -> LintArray { - lint_array!(BOX_VEC, VEC_BOX_SIZED, OPTION_OPTION, LINKEDLIST, BORROWED_BOX) + lint_array!(BOX_VEC, VEC_BOX, OPTION_OPTION, LINKEDLIST, BORROWED_BOX) } } @@ -292,13 +292,14 @@ fn check_ty(cx: &LateContext<'_, '_>, ast_ty: &hir::Ty, is_local: bool) { then { span_lint_and_sugg( cx, - VEC_BOX_SIZED, + VEC_BOX, ast_ty.span, - "you seem to be trying to use `Vec>`, but T is Sized. `Vec` is already on the heap, `Vec>` makes an extra allocation.", + "`Vec` is already on the heap, the boxing is unnecessary.", "try", format!("Vec<{}>", boxed_type), - Applicability::MachineApplicable - ) + Applicability::MaybeIncorrect, + ); + return; // don't recurse into the type } } } else if match_def_path(cx.tcx, def_id, &paths::OPTION) { diff --git a/tests/ui/vec_box_sized.stderr b/tests/ui/vec_box_sized.stderr index ae7171fdb31..7f4bdfb5aed 100644 --- a/tests/ui/vec_box_sized.stderr +++ b/tests/ui/vec_box_sized.stderr @@ -1,10 +1,10 @@ -error: you seem to be trying to use `Vec>`, but T is Sized. `Vec` is already on the heap, `Vec>` makes an extra allocation. +error: `Vec` is already on the heap, the boxing is unnecessary. --> $DIR/vec_box_sized.rs:10:14 | 10 | sized_type: Vec>, | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec` | - = note: `-D clippy::vec-box-sized` implied by `-D warnings` + = note: `-D clippy::vec-box` implied by `-D warnings` error: aborting due to previous error From 985eba08a558bae9a9042b65df59340d227d7673 Mon Sep 17 00:00:00 2001 From: Kampfkarren Date: Thu, 13 Dec 2018 10:46:21 -0800 Subject: [PATCH 6/6] Line length fix --- clippy_lints/src/types.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 62e99b92d35..dfa4cfdcf94 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -73,7 +73,8 @@ /// **Why is this bad?** `Vec` already keeps its contents in a separate area on /// the heap. So if you `Box` its contents, you just add another level of indirection. /// -/// **Known problems:** Vec> makes sense if T is a large type (see #3530, 1st comment). +/// **Known problems:** Vec> makes sense if T is a large type (see #3530, +/// 1st comment). /// /// **Example:** /// ```rust