Auto merge of #8906 - rust-lang:copy-large-enum-variants, r=Jarcho

bors · bors · commit e32b66c871cc · 2022-06-02T16:37:09.000Z
remove `large_enum_variant` suggestion for `Copy` types Replaces the (erroneous) suggestion on `large_enum_variant` for `Copy` types by a note. This fixes #8894. --- changelog: none
diff --git a/clippy_lints/src/large_enum_variant.rs b/clippy_lints/src/large_enum_variant.rs
@@ -1,12 +1,13 @@
 //! 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};
 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,41 +142,57 @@ 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) || maybe_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);
                     },
                 );
             }
         }
     }
 }
+
+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
@@ -98,6 +98,38 @@ 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 {}
+
+enum SomeGenericPossiblyCopyEnum<T> {
+    A(bool, std::marker::PhantomData<T>),
+    B([u64; 4000]),
+}
+
+impl<T: Copy> Clone for SomeGenericPossiblyCopyEnum<T> {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+
+impl<T: Copy> Copy for SomeGenericPossiblyCopyEnum<T> {}
+
 fn main() {
     large_enum_variant!();
 }
diff --git a/tests/ui/large_enum_variant.stderr b/tests/ui/large_enum_variant.stderr
@@ -127,5 +127,71 @@ help: consider boxing the large fields to reduce the total size of the enum
 LL |     B(Box<Struct2>),
    |       ~~~~~~~~~~~~
 
-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: 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<T>),
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+note: boxing a variant would require the type no longer be `Copy`
+  --> $DIR/large_enum_variant.rs:120:6
+   |
+LL | enum SomeGenericPossiblyCopyEnum<T> {
+   |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+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
 
