Auto merge of rust-lang#9662 - ebobrow:result-large-err, r=dswij

bors · bors · commit cad0d3d6da87 · 2022-11-11T06:58:59.000Z
`result_large_err` show largest variants in err msg fixes rust-lang#9538 changelog: Sugg: [`result_large_err`]: Now show largest enum variants in error message
diff --git a/clippy_lints/src/functions/result.rs b/clippy_lints/src/functions/result.rs
@@ -2,12 +2,12 @@ use rustc_errors::Diagnostic;
 use rustc_hir as hir;
 use rustc_lint::{LateContext, LintContext};
 use rustc_middle::lint::in_external_macro;
-use rustc_middle::ty::{self, Ty};
+use rustc_middle::ty::{self, Adt, Ty};
 use rustc_span::{sym, Span};
 
 use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_then};
 use clippy_utils::trait_ref_of_method;
-use clippy_utils::ty::{approx_ty_size, is_type_diagnostic_item};
+use clippy_utils::ty::{approx_ty_size, is_type_diagnostic_item, AdtVariantInfo};
 
 use super::{RESULT_LARGE_ERR, RESULT_UNIT_ERR};
 
@@ -84,17 +84,57 @@ fn check_result_unit_err(cx: &LateContext<'_>, err_ty: Ty<'_>, fn_header_span: S
 }
 
 fn check_result_large_err<'tcx>(cx: &LateContext<'tcx>, err_ty: Ty<'tcx>, hir_ty_span: Span, large_err_threshold: u64) {
-    let ty_size = approx_ty_size(cx, err_ty);
-    if ty_size >= large_err_threshold {
-        span_lint_and_then(
-            cx,
-            RESULT_LARGE_ERR,
-            hir_ty_span,
-            "the `Err`-variant returned from this function is very large",
-            |diag: &mut Diagnostic| {
-                diag.span_label(hir_ty_span, format!("the `Err`-variant is at least {ty_size} bytes"));
-                diag.help(format!("try reducing the size of `{err_ty}`, for example by boxing large elements or replacing it with `Box<{err_ty}>`"));
-            },
-        );
+    if_chain! {
+        if let Adt(adt, subst) = err_ty.kind();
+        if let Some(local_def_id) = err_ty.ty_adt_def().expect("already checked this is adt").did().as_local();
+        if let Some(hir::Node::Item(item)) = cx
+            .tcx
+            .hir()
+            .find_by_def_id(local_def_id);
+        if let hir::ItemKind::Enum(ref def, _) = item.kind;
+        then {
+            let variants_size = AdtVariantInfo::new(cx, *adt, subst);
+            if variants_size[0].size >= large_err_threshold {
+                span_lint_and_then(
+                    cx,
+                    RESULT_LARGE_ERR,
+                    hir_ty_span,
+                    "the `Err`-variant returned from this function is very large",
+                    |diag| {
+                        diag.span_label(
+                            def.variants[variants_size[0].ind].span,
+                            format!("the largest variant contains at least {} bytes", variants_size[0].size),
+                        );
+
+                        for variant in &variants_size[1..] {
+                            if variant.size >= large_err_threshold {
+                                let variant_def = &def.variants[variant.ind];
+                                diag.span_label(
+                                    variant_def.span,
+                                    format!("the variant `{}` contains at least {} bytes", variant_def.ident, variant.size),
+                                );
+                            }
+                        }
+
+                        diag.help(format!("try reducing the size of `{err_ty}`, for example by boxing large elements or replacing it with `Box<{err_ty}>`"));
+                    }
+                );
+            }
+        }
+        else {
+            let ty_size = approx_ty_size(cx, err_ty);
+            if ty_size >= large_err_threshold {
+                span_lint_and_then(
+                    cx,
+                    RESULT_LARGE_ERR,
+                    hir_ty_span,
+                    "the `Err`-variant returned from this function is very large",
+                    |diag: &mut Diagnostic| {
+                        diag.span_label(hir_ty_span, format!("the `Err`-variant is at least {ty_size} bytes"));
+                        diag.help(format!("try reducing the size of `{err_ty}`, for example by boxing large elements or replacing it with `Box<{err_ty}>`"));
+                    },
+                );
+            }
+        }
     }
 }
diff --git a/clippy_lints/src/large_enum_variant.rs b/clippy_lints/src/large_enum_variant.rs
@@ -1,12 +1,15 @@
 //! lint when there is a large size difference between variants on an enum
 
 use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::{diagnostics::span_lint_and_then, ty::approx_ty_size, ty::is_copy};
+use clippy_utils::{
+    diagnostics::span_lint_and_then,
+    ty::{approx_ty_size, is_copy, AdtVariantInfo},
+};
 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::{Adt, AdtDef, GenericArg, List, Ty};
+use rustc_middle::ty::{Adt, Ty};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::source_map::Span;
 
@@ -72,49 +75,6 @@ impl LargeEnumVariant {
     }
 }
 
-struct FieldInfo {
-    ind: usize,
-    size: u64,
-}
-
-struct VariantInfo {
-    ind: usize,
-    size: u64,
-    fields_size: Vec<FieldInfo>,
-}
-
-fn variants_size<'tcx>(
-    cx: &LateContext<'tcx>,
-    adt: AdtDef<'tcx>,
-    subst: &'tcx List<GenericArg<'tcx>>,
-) -> Vec<VariantInfo> {
-    let mut variants_size = adt
-        .variants()
-        .iter()
-        .enumerate()
-        .map(|(i, variant)| {
-            let mut fields_size = variant
-                .fields
-                .iter()
-                .enumerate()
-                .map(|(i, f)| FieldInfo {
-                    ind: i,
-                    size: approx_ty_size(cx, f.ty(cx.tcx, subst)),
-                })
-                .collect::<Vec<_>>();
-            fields_size.sort_by(|a, b| (a.size.cmp(&b.size)));
-
-            VariantInfo {
-                ind: i,
-                size: fields_size.iter().map(|info| info.size).sum(),
-                fields_size,
-            }
-        })
-        .collect::<Vec<_>>();
-    variants_size.sort_by(|a, b| (b.size.cmp(&a.size)));
-    variants_size
-}
-
 impl_lint_pass!(LargeEnumVariant => [LARGE_ENUM_VARIANT]);
 
 impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant {
@@ -130,7 +90,7 @@ impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant {
             if adt.variants().len() <= 1 {
                 return;
             }
-            let variants_size = variants_size(cx, *adt, subst);
+            let variants_size = AdtVariantInfo::new(cx, *adt, subst);
 
             let mut difference = variants_size[0].size - variants_size[1].size;
             if difference > self.maximum_size_difference_allowed {
@@ -173,16 +133,16 @@ impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant {
                                 .fields_size
                                 .iter()
                                 .rev()
-                                .map_while(|val| {
+                                .map_while(|&(ind, size)| {
                                     if difference > self.maximum_size_difference_allowed {
-                                        difference = difference.saturating_sub(val.size);
+                                        difference = difference.saturating_sub(size);
                                         Some((
-                                            fields[val.ind].ty.span,
+                                            fields[ind].ty.span,
                                             format!(
                                                 "Box<{}>",
                                                 snippet_with_applicability(
                                                     cx,
-                                                    fields[val.ind].ty.span,
+                                                    fields[ind].ty.span,
                                                     "..",
                                                     &mut applicability
                                                 )
diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs
@@ -13,9 +13,9 @@ use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::LateContext;
 use rustc_middle::mir::interpret::{ConstValue, Scalar};
 use rustc_middle::ty::{
-    self, AdtDef, AssocKind, Binder, BoundRegion, DefIdTree, FnSig, GenericParamDefKind, IntTy, ParamEnv, Predicate,
-    PredicateKind, ProjectionTy, Region, RegionKind, SubstsRef, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable,
-    TypeVisitor, UintTy, VariantDef, VariantDiscr,
+    self, AdtDef, AssocKind, Binder, BoundRegion, DefIdTree, FnSig, GenericParamDefKind, IntTy, List, ParamEnv,
+    Predicate, PredicateKind, ProjectionTy, Region, RegionKind, SubstsRef, Ty, TyCtxt, TypeSuperVisitable,
+    TypeVisitable, TypeVisitor, UintTy, VariantDef, VariantDiscr,
 };
 use rustc_middle::ty::{GenericArg, GenericArgKind};
 use rustc_span::symbol::Ident;
@@ -845,6 +845,42 @@ pub fn for_each_top_level_late_bound_region<B>(
     ty.visit_with(&mut V { index: 0, f })
 }
 
+pub struct AdtVariantInfo {
+    pub ind: usize,
+    pub size: u64,
+
+    /// (ind, size)
+    pub fields_size: Vec<(usize, u64)>,
+}
+
+impl AdtVariantInfo {
+    /// Returns ADT variants ordered by size
+    pub fn new<'tcx>(cx: &LateContext<'tcx>, adt: AdtDef<'tcx>, subst: &'tcx List<GenericArg<'tcx>>) -> Vec<Self> {
+        let mut variants_size = adt
+            .variants()
+            .iter()
+            .enumerate()
+            .map(|(i, variant)| {
+                let mut fields_size = variant
+                    .fields
+                    .iter()
+                    .enumerate()
+                    .map(|(i, f)| (i, approx_ty_size(cx, f.ty(cx.tcx, subst))))
+                    .collect::<Vec<_>>();
+                fields_size.sort_by(|(_, a_size), (_, b_size)| (a_size.cmp(b_size)));
+
+                Self {
+                    ind: i,
+                    size: fields_size.iter().map(|(_, size)| size).sum(),
+                    fields_size,
+                }
+            })
+            .collect::<Vec<_>>();
+        variants_size.sort_by(|a, b| (b.size.cmp(&a.size)));
+        variants_size
+    }
+}
+
 /// Gets the struct or enum variant from the given `Res`
 pub fn variant_of_res<'tcx>(cx: &LateContext<'tcx>, res: Res) -> Option<&'tcx VariantDef> {
     match res {
diff --git a/tests/ui/result_large_err.rs b/tests/ui/result_large_err.rs
@@ -50,6 +50,18 @@ impl LargeErrorVariants<()> {
     }
 }
 
+enum MultipleLargeVariants {
+    _Biggest([u8; 1024]),
+    _AlsoBig([u8; 512]),
+    _Ok(usize),
+}
+
+impl MultipleLargeVariants {
+    fn large_enum_error() -> Result<(), Self> {
+        Ok(())
+    }
+}
+
 trait TraitForcesLargeError {
     fn large_error() -> Result<(), [u8; 512]> {
         Ok(())
diff --git a/tests/ui/result_large_err.stderr b/tests/ui/result_large_err.stderr
@@ -42,50 +42,66 @@ LL | pub fn param_large_error<R>() -> Result<(), (u128, R, FullyDefinedLargeErro
 error: the `Err`-variant returned from this function is very large
   --> $DIR/result_large_err.rs:48:34
    |
+LL |     _Omg([u8; 512]),
+   |     --------------- the largest variant contains at least 512 bytes
+...
 LL |     pub fn large_enum_error() -> Result<(), Self> {
-   |                                  ^^^^^^^^^^^^^^^^ the `Err`-variant is at least 513 bytes
+   |                                  ^^^^^^^^^^^^^^^^
    |
    = help: try reducing the size of `LargeErrorVariants<()>`, for example by boxing large elements or replacing it with `Box<LargeErrorVariants<()>>`
 
 error: the `Err`-variant returned from this function is very large
-  --> $DIR/result_large_err.rs:54:25
+  --> $DIR/result_large_err.rs:60:30
+   |
+LL |     _Biggest([u8; 1024]),
+   |     -------------------- the largest variant contains at least 1024 bytes
+LL |     _AlsoBig([u8; 512]),
+   |     ------------------- the variant `_AlsoBig` contains at least 512 bytes
+...
+LL |     fn large_enum_error() -> Result<(), Self> {
+   |                              ^^^^^^^^^^^^^^^^
+   |
+   = help: try reducing the size of `MultipleLargeVariants`, for example by boxing large elements or replacing it with `Box<MultipleLargeVariants>`
+
+error: the `Err`-variant returned from this function is very large
+  --> $DIR/result_large_err.rs:66:25
    |
 LL |     fn large_error() -> Result<(), [u8; 512]> {
    |                         ^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 512 bytes
    |
    = help: try reducing the size of `[u8; 512]`, for example by boxing large elements or replacing it with `Box<[u8; 512]>`
 
 error: the `Err`-variant returned from this function is very large
-  --> $DIR/result_large_err.rs:73:29
+  --> $DIR/result_large_err.rs:85:29
    |
 LL | pub fn large_union_err() -> Result<(), FullyDefinedUnionError> {
    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 512 bytes
    |
    = help: try reducing the size of `FullyDefinedUnionError`, for example by boxing large elements or replacing it with `Box<FullyDefinedUnionError>`
 
 error: the `Err`-variant returned from this function is very large
-  --> $DIR/result_large_err.rs:82:40
+  --> $DIR/result_large_err.rs:94:40
    |
 LL | pub fn param_large_union<T: Copy>() -> Result<(), UnionError<T>> {
    |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 512 bytes
    |
    = help: try reducing the size of `UnionError<T>`, for example by boxing large elements or replacing it with `Box<UnionError<T>>`
 
 error: the `Err`-variant returned from this function is very large
-  --> $DIR/result_large_err.rs:91:34
+  --> $DIR/result_large_err.rs:103:34
    |
 LL | pub fn array_error_subst<U>() -> Result<(), ArrayError<i32, U>> {
    |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 128 bytes
    |
    = help: try reducing the size of `ArrayError<i32, U>`, for example by boxing large elements or replacing it with `Box<ArrayError<i32, U>>`
 
 error: the `Err`-variant returned from this function is very large
-  --> $DIR/result_large_err.rs:95:31
+  --> $DIR/result_large_err.rs:107:31
    |
 LL | pub fn array_error<T, U>() -> Result<(), ArrayError<(i32, T), U>> {
    |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 128 bytes
    |
    = help: try reducing the size of `ArrayError<(i32, T), U>`, for example by boxing large elements or replacing it with `Box<ArrayError<(i32, T), U>>`
 
-error: aborting due to 11 previous errors
+error: aborting due to 12 previous errors
 
