From 56f0c9afdd4d85859e814e5dee503fd76cc3b2a2 Mon Sep 17 00:00:00 2001 From: surechen Date: Thu, 16 Sep 2021 16:42:36 +0800 Subject: [PATCH] fix bug for large_enum_variants --- clippy_lints/src/large_enum_variant.rs | 161 +++++++++++++++---------- tests/ui/large_enum_variant.rs | 18 +++ tests/ui/large_enum_variant.stderr | 63 ++++++++-- 3 files changed, 171 insertions(+), 71 deletions(-) diff --git a/clippy_lints/src/large_enum_variant.rs b/clippy_lints/src/large_enum_variant.rs index e4b8e754628..392166237be 100644 --- a/clippy_lints/src/large_enum_variant.rs +++ b/clippy_lints/src/large_enum_variant.rs @@ -1,13 +1,14 @@ //! 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_opt; +use clippy_utils::source::snippet_with_applicability; use rustc_errors::Applicability; -use rustc_hir::{Item, ItemKind, VariantData}; +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_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::source_map::Span; declare_clippy_lint! { /// ### What it does @@ -58,6 +59,17 @@ impl LargeEnumVariant { } } +struct FieldInfo { + ind: usize, + size: u64, +} + +struct VariantInfo { + ind: usize, + size: u64, + fields_size: Vec, +} + impl_lint_pass!(LargeEnumVariant => [LARGE_ENUM_VARIANT]); impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant { @@ -68,72 +80,95 @@ impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant { if let ItemKind::Enum(ref def, _) = item.kind { let ty = cx.tcx.type_of(item.def_id); let adt = ty.ty_adt_def().expect("already checked whether this is an enum"); - - let mut largest_variant: Option<(_, _)> = None; - let mut second_variant: Option<(_, _)> = None; - - for (i, variant) in adt.variants.iter().enumerate() { - let size: u64 = variant - .fields - .iter() - .filter_map(|f| { - let ty = cx.tcx.type_of(f.did); - // don't count generics by filtering out everything - // that does not have a layout - cx.layout_of(ty).ok().map(|l| l.size.bytes()) - }) - .sum(); - - let grouped = (size, (i, variant)); - - if grouped.0 >= largest_variant.map_or(0, |x| x.0) { - second_variant = largest_variant; - largest_variant = Some(grouped); - } + if adt.variants.len() <= 1 { + return; } + let mut variants_size: Vec = adt + .variants + .iter() + .enumerate() + .map(|(i, variant)| { + let mut fields_size = Vec::new(); + let size: u64 = variant + .fields + .iter() + .enumerate() + .filter_map(|(i, f)| { + let ty = cx.tcx.type_of(f.did); + // don't count generics by filtering out everything + // that does not have a layout + cx.layout_of(ty).ok().map(|l| { + let size = l.size.bytes(); + fields_size.push(FieldInfo { ind: i, size }); + size + }) + }) + .sum(); + VariantInfo { + ind: i, + size, + fields_size, + } + }) + .collect(); - if let (Some(largest), Some(second)) = (largest_variant, second_variant) { - let difference = largest.0 - second.0; + variants_size.sort_by(|a, b| (b.size.cmp(&a.size))); - if difference > self.maximum_size_difference_allowed { - let (i, variant) = largest.1; + let mut difference = variants_size[0].size - variants_size[1].size; + if difference > self.maximum_size_difference_allowed { + let help_text = "consider boxing the large fields to reduce the total size of the enum"; + span_lint_and_then( + cx, + LARGE_ENUM_VARIANT, + def.variants[variants_size[0].ind].span, + "large size difference between variants", + |diag| { + diag.span_label( + def.variants[variants_size[0].ind].span, + &format!("this variant is {} bytes", variants_size[0].size), + ); + diag.span_note( + def.variants[variants_size[1].ind].span, + &format!("and the second-largest variant is {} bytes:", variants_size[1].size), + ); - let help_text = "consider boxing the large fields to reduce the total size of the enum"; - span_lint_and_then( - cx, - LARGE_ENUM_VARIANT, - def.variants[i].span, - "large size difference between variants", - |diag| { - diag.span_label( - def.variants[(largest.1).0].span, - &format!("this variant is {} bytes", largest.0), - ); - diag.span_note( - def.variants[(second.1).0].span, - &format!("and the second-largest variant is {} bytes:", second.0), - ); - if variant.fields.len() == 1 { - let span = match def.variants[i].data { - VariantData::Struct(fields, ..) | VariantData::Tuple(fields, ..) => { - fields[0].ty.span - }, - VariantData::Unit(..) => unreachable!(), - }; - if let Some(snip) = snippet_opt(cx, span) { - diag.span_suggestion( - span, - help_text, - format!("Box<{}>", snip), - Applicability::MaybeIncorrect, - ); - return; + 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 } - } - diag.span_help(def.variants[i].span, help_text); - }, - ); - } + }) + .collect(); + + 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 d22fee3f27b..b45cc849eae 100644 --- a/tests/ui/large_enum_variant.rs +++ b/tests/ui/large_enum_variant.rs @@ -35,6 +35,7 @@ enum LargeEnum2 { VariantOk(i32, u32), ContainingLargeEnum(LargeEnum), } + enum LargeEnum3 { ContainingMoreThanOneField(i32, [i32; 8000], [i32; 9500]), VoidVariant, @@ -56,6 +57,23 @@ enum LargeEnumOk { LargeB([i32; 8001]), } +enum LargeEnum6 { + A, + B([u8; 255]), + C([u8; 200]), +} + +enum LargeEnum7 { + A, + B([u8; 1255]), + C([u8; 200]), +} + +enum LargeEnum8 { + VariantOk(i32, u32), + ContainingMoreThanOneField([i32; 8000], [i32; 2], [i32; 9500], [i32; 30]), +} + fn main() { large_enum_variant!(); } diff --git a/tests/ui/large_enum_variant.stderr b/tests/ui/large_enum_variant.stderr index 0eac28fbd35..899f97ce2e1 100644 --- a/tests/ui/large_enum_variant.stderr +++ b/tests/ui/large_enum_variant.stderr @@ -32,30 +32,45 @@ LL | ContainingLargeEnum(Box), | ~~~~~~~~~~~~~~ error: large size difference between variants - --> $DIR/large_enum_variant.rs:46:5 + --> $DIR/large_enum_variant.rs:40:5 + | +LL | ContainingMoreThanOneField(i32, [i32; 8000], [i32; 9500]), + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this variant is 70004 bytes + | +note: and the second-largest variant is 8 bytes: + --> $DIR/large_enum_variant.rs:42:5 + | +LL | StructLikeLittle { x: i32, y: i32 }, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: consider boxing the large fields to reduce the total size of the enum + | +LL | ContainingMoreThanOneField(i32, Box<[i32; 8000]>, Box<[i32; 9500]>), + | ~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~ + +error: large size difference between variants + --> $DIR/large_enum_variant.rs:47:5 | LL | StructLikeLarge { x: [i32; 8000], y: i32 }, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this variant is 32004 bytes | note: and the second-largest variant is 8 bytes: - --> $DIR/large_enum_variant.rs:45:5 + --> $DIR/large_enum_variant.rs:46:5 | LL | VariantOk(i32, u32), | ^^^^^^^^^^^^^^^^^^^ help: consider boxing the large fields to reduce the total size of the enum - --> $DIR/large_enum_variant.rs:46:5 | -LL | StructLikeLarge { x: [i32; 8000], y: i32 }, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | StructLikeLarge { x: Box<[i32; 8000]>, y: i32 }, + | ~~~~~~~~~~~~~~~~ error: large size difference between variants - --> $DIR/large_enum_variant.rs:51:5 + --> $DIR/large_enum_variant.rs:52:5 | LL | StructLikeLarge2 { x: [i32; 8000] }, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this variant is 32000 bytes | note: and the second-largest variant is 8 bytes: - --> $DIR/large_enum_variant.rs:50:5 + --> $DIR/large_enum_variant.rs:51:5 | LL | VariantOk(i32, u32), | ^^^^^^^^^^^^^^^^^^^ @@ -64,5 +79,37 @@ help: consider boxing the large fields to reduce the total size of the enum LL | StructLikeLarge2 { x: Box<[i32; 8000]> }, | ~~~~~~~~~~~~~~~~ -error: aborting due to 4 previous errors +error: large size difference between variants + --> $DIR/large_enum_variant.rs:68:5 + | +LL | B([u8; 1255]), + | ^^^^^^^^^^^^^ this variant is 1255 bytes + | +note: and the second-largest variant is 200 bytes: + --> $DIR/large_enum_variant.rs:69:5 + | +LL | C([u8; 200]), + | ^^^^^^^^^^^^ +help: consider boxing the large fields to reduce the total size of the enum + | +LL | B(Box<[u8; 1255]>), + | ~~~~~~~~~~~~~~~ + +error: large size difference between variants + --> $DIR/large_enum_variant.rs:74:5 + | +LL | ContainingMoreThanOneField([i32; 8000], [i32; 2], [i32; 9500], [i32; 30]), + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this variant is 70128 bytes + | +note: and the second-largest variant is 8 bytes: + --> $DIR/large_enum_variant.rs:73:5 + | +LL | VariantOk(i32, u32), + | ^^^^^^^^^^^^^^^^^^^ +help: consider boxing the large fields to reduce the total size of the enum + | +LL | ContainingMoreThanOneField(Box<[i32; 8000]>, [i32; 2], Box<[i32; 9500]>, [i32; 30]), + | ~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~ + +error: aborting due to 7 previous errors