From 19f94d52b336a8535c6df1670ed6eb56c08acb55 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Sat, 28 May 2022 01:36:05 +0200 Subject: [PATCH 1/2] remove `large_enum_variant` suggestion for `Copy` types --- clippy_lints/src/large_enum_variant.rs | 66 ++++++++++++++------------ tests/ui/large_enum_variant.rs | 18 +++++++ tests/ui/large_enum_variant.stderr | 46 +++++++++++++++++- 3 files changed, 99 insertions(+), 31 deletions(-) diff --git a/clippy_lints/src/large_enum_variant.rs b/clippy_lints/src/large_enum_variant.rs index 0f3889a29..34b7cf67d 100644 --- a/clippy_lints/src/large_enum_variant.rs +++ b/clippy_lints/src/large_enum_variant.rs @@ -1,7 +1,7 @@ //! lint when there is a large size difference between variants on an enum -use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet_with_applicability; +use clippy_utils::{diagnostics::span_lint_and_then, ty::is_copy}; use rustc_errors::Applicability; use rustc_hir::{Item, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; @@ -132,37 +132,43 @@ impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant { let fields = def.variants[variants_size[0].ind].data.fields(); variants_size[0].fields_size.sort_by(|a, b| (a.size.cmp(&b.size))); let mut applicability = Applicability::MaybeIncorrect; - let sugg: Vec<(Span, String)> = variants_size[0] - .fields_size - .iter() - .rev() - .map_while(|val| { - if difference > self.maximum_size_difference_allowed { - difference = difference.saturating_sub(val.size); - Some(( - fields[val.ind].ty.span, - format!( - "Box<{}>", - snippet_with_applicability( - cx, - fields[val.ind].ty.span, - "..", - &mut applicability - ) - .into_owned() - ), - )) - } else { - None - } - }) - .collect(); + if is_copy(cx, ty) { + diag.span_note( + item.ident.span, + "boxing a variant would require the type no longer be `Copy`", + ); + } else { + let sugg: Vec<(Span, String)> = variants_size[0] + .fields_size + .iter() + .rev() + .map_while(|val| { + if difference > self.maximum_size_difference_allowed { + difference = difference.saturating_sub(val.size); + Some(( + fields[val.ind].ty.span, + format!( + "Box<{}>", + snippet_with_applicability( + cx, + fields[val.ind].ty.span, + "..", + &mut applicability + ) + .into_owned() + ), + )) + } else { + None + } + }) + .collect(); - if !sugg.is_empty() { - diag.multipart_suggestion(help_text, sugg, Applicability::MaybeIncorrect); - return; + if !sugg.is_empty() { + diag.multipart_suggestion(help_text, sugg, Applicability::MaybeIncorrect); + return; + } } - diag.span_help(def.variants[variants_size[0].ind].span, help_text); }, ); diff --git a/tests/ui/large_enum_variant.rs b/tests/ui/large_enum_variant.rs index cee9e2372..42563383f 100644 --- a/tests/ui/large_enum_variant.rs +++ b/tests/ui/large_enum_variant.rs @@ -98,6 +98,24 @@ struct Struct2 { a: [i32; 8000], } +#[derive(Copy, Clone)] +enum CopyableLargeEnum { + A(bool), + B([u128; 4000]), +} + +enum ManuallyCopyLargeEnum { + A(bool), + B([u128; 4000]), +} + +impl Clone for ManuallyCopyLargeEnum { + fn clone(&self) -> Self { + *self + } +} +impl Copy for ManuallyCopyLargeEnum {} + fn main() { large_enum_variant!(); } diff --git a/tests/ui/large_enum_variant.stderr b/tests/ui/large_enum_variant.stderr index cbf2ac972..397d263ae 100644 --- a/tests/ui/large_enum_variant.stderr +++ b/tests/ui/large_enum_variant.stderr @@ -127,5 +127,49 @@ help: consider boxing the large fields to reduce the total size of the enum LL | B(Box), | ~~~~~~~~~~~~ -error: aborting due to 8 previous errors +error: large size difference between variants + --> $DIR/large_enum_variant.rs:104:5 + | +LL | B([u128; 4000]), + | ^^^^^^^^^^^^^^^ this variant is 64000 bytes + | +note: and the second-largest variant is 1 bytes: + --> $DIR/large_enum_variant.rs:103:5 + | +LL | A(bool), + | ^^^^^^^ +note: boxing a variant would require the type no longer be `Copy` + --> $DIR/large_enum_variant.rs:102:6 + | +LL | enum CopyableLargeEnum { + | ^^^^^^^^^^^^^^^^^ +help: consider boxing the large fields to reduce the total size of the enum + --> $DIR/large_enum_variant.rs:104:5 + | +LL | B([u128; 4000]), + | ^^^^^^^^^^^^^^^ + +error: large size difference between variants + --> $DIR/large_enum_variant.rs:109:5 + | +LL | B([u128; 4000]), + | ^^^^^^^^^^^^^^^ this variant is 64000 bytes + | +note: and the second-largest variant is 1 bytes: + --> $DIR/large_enum_variant.rs:108:5 + | +LL | A(bool), + | ^^^^^^^ +note: boxing a variant would require the type no longer be `Copy` + --> $DIR/large_enum_variant.rs:107:6 + | +LL | enum ManuallyCopyLargeEnum { + | ^^^^^^^^^^^^^^^^^^^^^ +help: consider boxing the large fields to reduce the total size of the enum + --> $DIR/large_enum_variant.rs:109:5 + | +LL | B([u128; 4000]), + | ^^^^^^^^^^^^^^^ + +error: aborting due to 10 previous errors From 756caf79e64598886551325dbf9ab7eccee03328 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Tue, 31 May 2022 09:23:17 +0200 Subject: [PATCH 2/2] account for generics --- clippy_lints/src/large_enum_variant.rs | 24 ++++++++++++++++++++++-- tests/ui/large_enum_variant.rs | 14 ++++++++++++++ tests/ui/large_enum_variant.stderr | 24 +++++++++++++++++++++++- 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/large_enum_variant.rs b/clippy_lints/src/large_enum_variant.rs index 34b7cf67d..63ac092df 100644 --- a/clippy_lints/src/large_enum_variant.rs +++ b/clippy_lints/src/large_enum_variant.rs @@ -7,6 +7,7 @@ use rustc_hir::{Item, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::lint::in_external_macro; use rustc_middle::ty::layout::LayoutOf; +use rustc_middle::ty::{Adt, Ty}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; @@ -26,6 +27,15 @@ declare_clippy_lint! { /// the overhead is negligible and the boxing is counter-productive. Always /// measure the change this lint suggests. /// + /// For types that implement `Copy`, the suggestion to `Box` a variant's + /// data would require removing the trait impl. The types can of course + /// still be `Clone`, but that is worse ergonomically. Depending on the + /// use case it may be possible to store the large data in an auxillary + /// structure (e.g. Arena or ECS). + /// + /// The lint will ignore generic types if the layout depends on the + /// generics, even if the size difference will be large anyway. + /// /// ### Example /// ```rust /// // Bad @@ -74,7 +84,7 @@ struct VariantInfo { impl_lint_pass!(LargeEnumVariant => [LARGE_ENUM_VARIANT]); impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant { - fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &Item<'tcx>) { if in_external_macro(cx.tcx.sess, item.span) { return; } @@ -132,7 +142,7 @@ impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant { let fields = def.variants[variants_size[0].ind].data.fields(); variants_size[0].fields_size.sort_by(|a, b| (a.size.cmp(&b.size))); let mut applicability = Applicability::MaybeIncorrect; - if is_copy(cx, ty) { + if is_copy(cx, ty) || maybe_copy(cx, ty) { diag.span_note( item.ident.span, "boxing a variant would require the type no longer be `Copy`", @@ -176,3 +186,13 @@ impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant { } } } + +fn maybe_copy<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { + if let Adt(_def, substs) = ty.kind() + && substs.types().next().is_some() + && let Some(copy_trait) = cx.tcx.lang_items().copy_trait() + { + return cx.tcx.non_blanket_impls_for_ty(copy_trait, ty).next().is_some(); + } + false +} diff --git a/tests/ui/large_enum_variant.rs b/tests/ui/large_enum_variant.rs index 42563383f..23152a133 100644 --- a/tests/ui/large_enum_variant.rs +++ b/tests/ui/large_enum_variant.rs @@ -114,8 +114,22 @@ impl Clone for ManuallyCopyLargeEnum { *self } } + impl Copy for ManuallyCopyLargeEnum {} +enum SomeGenericPossiblyCopyEnum { + A(bool, std::marker::PhantomData), + B([u64; 4000]), +} + +impl Clone for SomeGenericPossiblyCopyEnum { + fn clone(&self) -> Self { + *self + } +} + +impl Copy for SomeGenericPossiblyCopyEnum {} + fn main() { large_enum_variant!(); } diff --git a/tests/ui/large_enum_variant.stderr b/tests/ui/large_enum_variant.stderr index 397d263ae..024832726 100644 --- a/tests/ui/large_enum_variant.stderr +++ b/tests/ui/large_enum_variant.stderr @@ -171,5 +171,27 @@ help: consider boxing the large fields to reduce the total size of the enum LL | B([u128; 4000]), | ^^^^^^^^^^^^^^^ -error: aborting due to 10 previous errors +error: large size difference between variants + --> $DIR/large_enum_variant.rs:122:5 + | +LL | B([u64; 4000]), + | ^^^^^^^^^^^^^^ this variant is 32000 bytes + | +note: and the second-largest variant is 1 bytes: + --> $DIR/large_enum_variant.rs:121:5 + | +LL | A(bool, std::marker::PhantomData), + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +note: boxing a variant would require the type no longer be `Copy` + --> $DIR/large_enum_variant.rs:120:6 + | +LL | enum SomeGenericPossiblyCopyEnum { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: consider boxing the large fields to reduce the total size of the enum + --> $DIR/large_enum_variant.rs:122:5 + | +LL | B([u64; 4000]), + | ^^^^^^^^^^^^^^ + +error: aborting due to 11 previous errors