From 80e5856b02060f1185b5976b515eeada18811756 Mon Sep 17 00:00:00 2001 From: Elliot Bobrow Date: Sun, 16 Oct 2022 11:37:26 -0700 Subject: [PATCH] `result_large_err` show largest variants in err msg --- clippy_lints/src/functions/result.rs | 68 ++++++++++++++++++++------ clippy_lints/src/large_enum_variant.rs | 60 ++++------------------- clippy_utils/src/ty.rs | 42 ++++++++++++++-- tests/ui/result_large_err.rs | 12 +++++ tests/ui/result_large_err.stderr | 30 +++++++++--- 5 files changed, 138 insertions(+), 74 deletions(-) diff --git a/clippy_lints/src/functions/result.rs b/clippy_lints/src/functions/result.rs index 113c4e9f509..3e288467ba1 100644 --- 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 index 8ed7e4bb196..fd82d9f80f9 100644 --- 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, -} - -fn variants_size<'tcx>( - cx: &LateContext<'tcx>, - adt: AdtDef<'tcx>, - subst: &'tcx List>, -) -> Vec { - 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::>(); - 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::>(); - 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 index a022aac4bfe..3e9c605e3f9 100644 --- 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( 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>) -> Vec { + 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::>(); + 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::>(); + 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 index f7df3b85655..9dd27d6dc01 100644 --- 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 index bea101fe20b..c386edfd215 100644 --- a/tests/ui/result_large_err.stderr +++ b/tests/ui/result_large_err.stderr @@ -42,13 +42,29 @@ LL | pub fn param_large_error() -> 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>` 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` + +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 @@ -56,7 +72,7 @@ LL | fn large_error() -> Result<(), [u8; 512]> { = 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 @@ -64,7 +80,7 @@ LL | pub fn large_union_err() -> Result<(), FullyDefinedUnionError> { = help: try reducing the size of `FullyDefinedUnionError`, for example by boxing large elements or replacing it with `Box` 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() -> Result<(), UnionError> { | ^^^^^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 512 bytes @@ -72,7 +88,7 @@ LL | pub fn param_large_union() -> Result<(), UnionError> { = help: try reducing the size of `UnionError`, for example by boxing large elements or replacing it with `Box>` 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() -> Result<(), ArrayError> { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 128 bytes @@ -80,12 +96,12 @@ LL | pub fn array_error_subst() -> Result<(), ArrayError> { = help: try reducing the size of `ArrayError`, for example by boxing large elements or replacing it with `Box>` 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() -> 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>` -error: aborting due to 11 previous errors +error: aborting due to 12 previous errors