From 4aff8711f0a047fca0f06627e94618f4da1f7e4f Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 1 Feb 2019 08:21:32 +0100 Subject: [PATCH] Fix ICE in vec_box lint and add run-rustfix `hir::Ty` doesn't seem to know anything about type bounds and `cx.tcx.type_of(def_id)` caused an ICE when it was passed a generic type with a bound: ``` src/librustc_typeck/collect.rs:1311: unexpected non-type Node::GenericParam: Type { default: None, synthetic: None } ``` Converting it to a proper `Ty` fixes the ICE and catches a few more places where the lint applies. --- clippy_lints/src/types.rs | 30 +++++++++++++-------------- tests/ui/complex_types.rs | 2 +- tests/ui/vec_box_sized.fixed | 36 ++++++++++++++++++++++++++++++++ tests/ui/vec_box_sized.rs | 39 ++++++++++++++++++++++++++--------- tests/ui/vec_box_sized.stderr | 20 ++++++++++++++---- 5 files changed, 96 insertions(+), 31 deletions(-) create mode 100644 tests/ui/vec_box_sized.fixed diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index 939dd27c441..75e2f75ffcd 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -275,26 +275,24 @@ fn check_ty(cx: &LateContext<'_, '_>, hir_ty: &hir::Ty, is_local: bool) { 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 { + if let Some(boxed_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(ty.span), cx.param_env); then { - span_lint_and_sugg( - cx, - VEC_BOX, - hir_ty.span, - "`Vec` is already on the heap, the boxing is unnecessary.", - "try", - format!("Vec<{}>", boxed_type), - Applicability::MaybeIncorrect, - ); - return; // don't recurse into the type + let ty_ty = hir_ty_to_ty(cx.tcx, boxed_ty); + if ty_ty.is_sized(cx.tcx.at(ty.span), cx.param_env) { + span_lint_and_sugg( + cx, + VEC_BOX, + hir_ty.span, + "`Vec` is already on the heap, the boxing is unnecessary.", + "try", + format!("Vec<{}>", ty_ty), + 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/complex_types.rs b/tests/ui/complex_types.rs index cfece3768ef..be61fb6b9be 100644 --- a/tests/ui/complex_types.rs +++ b/tests/ui/complex_types.rs @@ -1,5 +1,5 @@ #![warn(clippy::all)] -#![allow(unused, clippy::needless_pass_by_value)] +#![allow(unused, clippy::needless_pass_by_value, clippy::vec_box)] #![feature(associated_type_defaults)] type Alias = Vec>>; // no warning here diff --git a/tests/ui/vec_box_sized.fixed b/tests/ui/vec_box_sized.fixed new file mode 100644 index 00000000000..a56dac8aa23 --- /dev/null +++ b/tests/ui/vec_box_sized.fixed @@ -0,0 +1,36 @@ +// run-rustfix + +#![allow(dead_code)] + +struct SizedStruct(i32); +struct UnsizedStruct([i32]); + +/// The following should trigger the lint +mod should_trigger { + use super::SizedStruct; + + struct StructWithVecBox { + sized_type: Vec, + } + + struct A(Vec); + struct B(Vec>); +} + +/// The following should not trigger the lint +mod should_not_trigger { + use super::UnsizedStruct; + + struct C(Vec>); + + struct StructWithVecBoxButItsUnsized { + unsized_type: Vec>, + } + + struct TraitVec { + // Regression test for #3720. This was causing an ICE. + inner: Vec>, + } +} + +fn main() {} diff --git a/tests/ui/vec_box_sized.rs b/tests/ui/vec_box_sized.rs index 884761675fc..32d1e940f27 100644 --- a/tests/ui/vec_box_sized.rs +++ b/tests/ui/vec_box_sized.rs @@ -1,17 +1,36 @@ -struct SizedStruct { - _a: i32, +// run-rustfix + +#![allow(dead_code)] + +struct SizedStruct(i32); +struct UnsizedStruct([i32]); + +/// The following should trigger the lint +mod should_trigger { + use super::SizedStruct; + + struct StructWithVecBox { + sized_type: Vec>, + } + + struct A(Vec>); + struct B(Vec>>); } -struct UnsizedStruct { - _a: [i32], -} +/// The following should not trigger the lint +mod should_not_trigger { + use super::UnsizedStruct; -struct StructWithVecBox { - sized_type: Vec>, -} + struct C(Vec>); -struct StructWithVecBoxButItsUnsized { - unsized_type: Vec>, + struct StructWithVecBoxButItsUnsized { + unsized_type: Vec>, + } + + struct TraitVec { + // Regression test for #3720. This was causing an ICE. + inner: Vec>, + } } fn main() {} diff --git a/tests/ui/vec_box_sized.stderr b/tests/ui/vec_box_sized.stderr index 21b515fa486..b33880b46bd 100644 --- a/tests/ui/vec_box_sized.stderr +++ b/tests/ui/vec_box_sized.stderr @@ -1,10 +1,22 @@ error: `Vec` is already on the heap, the boxing is unnecessary. - --> $DIR/vec_box_sized.rs:10:17 + --> $DIR/vec_box_sized.rs:13:21 | -LL | sized_type: Vec>, - | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec` +LL | sized_type: Vec>, + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec` | = note: `-D clippy::vec-box` implied by `-D warnings` -error: aborting due to previous error +error: `Vec` is already on the heap, the boxing is unnecessary. + --> $DIR/vec_box_sized.rs:16:14 + | +LL | struct A(Vec>); + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `Vec` + +error: `Vec` is already on the heap, the boxing is unnecessary. + --> $DIR/vec_box_sized.rs:17:18 + | +LL | struct B(Vec>>); + | ^^^^^^^^^^^^^^^ help: try: `Vec` + +error: aborting due to 3 previous errors