From a3420f70043c19165a4e44639ef4e6f3c156a174 Mon Sep 17 00:00:00 2001 From: Nathaniel Hamovitz <18648574+nhamovitz@users.noreply.github.com> Date: Sat, 16 Oct 2021 22:03:08 -0700 Subject: [PATCH] Tidy comments + tests; revert 'size-is-zero' detection --- ...railing_zero_sized_array_without_repr_c.rs | 35 ++++--- ...railing_zero_sized_array_without_repr_c.rs | 36 +++---- ...ing_zero_sized_array_without_repr_c.stderr | 97 ++++++++----------- 3 files changed, 75 insertions(+), 93 deletions(-) diff --git a/clippy_lints/src/trailing_zero_sized_array_without_repr_c.rs b/clippy_lints/src/trailing_zero_sized_array_without_repr_c.rs index 913812126..bc055307d 100644 --- a/clippy_lints/src/trailing_zero_sized_array_without_repr_c.rs +++ b/clippy_lints/src/trailing_zero_sized_array_without_repr_c.rs @@ -36,16 +36,15 @@ declare_clippy_lint! { } declare_lint_pass!(TrailingZeroSizedArrayWithoutReprC => [TRAILING_ZERO_SIZED_ARRAY_WITHOUT_REPR_C]); -// TESTNAME=trailing_zero_sized_array_without_repr_c cargo uitest - impl<'tcx> LateLintPass<'tcx> for TrailingZeroSizedArrayWithoutReprC { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) { if is_struct_with_trailing_zero_sized_array(cx, item) { + // NOTE: This is to include attributes on the definition when we print the lint. If the convention + // is to not do that with struct definitions (I'm not sure), then this isn't necessary. let attrs = cx.tcx.get_attrs(item.def_id.to_def_id()); - let first_attr = attrs.first(); // Actually, I've no idea if this is guaranteed to be the first one in the source code. - + let first_attr = attrs.iter().min_by_key(|attr| attr.span.lo()); let lint_span = if let Some(first_attr) = first_attr { - first_attr.span.until(item.span) + first_attr.span.to(item.span) } else { item.span }; @@ -66,18 +65,29 @@ impl<'tcx> LateLintPass<'tcx> for TrailingZeroSizedArrayWithoutReprC { fn is_struct_with_trailing_zero_sized_array(cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) -> bool { if_chain! { - // Check if last field is an array + // First check if last field is an array if let ItemKind::Struct(data, _) = &item.kind; if let VariantData::Struct(field_defs, _) = data; if let Some(last_field) = field_defs.last(); if let rustc_hir::TyKind::Array(_, length) = last_field.ty.kind; - // Check if that that array zero-sized. - let length_ldid = cx.tcx.hir().local_def_id(length.hir_id); - let length = Const::from_anon_const(cx.tcx, length_ldid); - if let Some(Constant::Int(length)) = miri_to_const(length); - if length == 0; + // Then check if that that array zero-sized + // This is pretty much copied from `enum_clike.rs` and I don't fully understand it, so let me know + // if there's a better way. I tried `Const::from_anon_const` but it didn't fold in the values + // on the `ZeroSizedWithConst` and `ZeroSizedWithConstFunction` tests. + + // This line in particular seems convoluted. + let length_did = cx.tcx.hir().body_owner_def_id(length.body).to_def_id(); + let length_ty = cx.tcx.type_of(length_did); + let length = cx + .tcx + .const_eval_poly(length_did) + .ok() + .map(|val| Const::from_value(cx.tcx, val, length_ty)) + .and_then(miri_to_const); + if let Some(Constant::Int(length)) = length; + if length == 0; then { true } else { @@ -88,7 +98,8 @@ fn is_struct_with_trailing_zero_sized_array(cx: &LateContext<'tcx>, item: &'tcx fn has_repr_attr(cx: &LateContext<'tcx>, attrs: &[Attribute]) -> bool { // NOTE: there's at least four other ways to do this but I liked this one the best. (All five agreed - // on all testcases.) Happy to use another; they're in the commit history. + // on all testcases.) Happy to use another; they're in the commit history if you want to look (or I + // can go find them). attrs .iter() .any(|attr| !rustc_attr::find_repr_attrs(cx.tcx.sess(), attr).is_empty()) diff --git a/tests/ui/trailing_zero_sized_array_without_repr_c.rs b/tests/ui/trailing_zero_sized_array_without_repr_c.rs index 6ab96c2eb..77b2c29b2 100644 --- a/tests/ui/trailing_zero_sized_array_without_repr_c.rs +++ b/tests/ui/trailing_zero_sized_array_without_repr_c.rs @@ -1,5 +1,4 @@ #![warn(clippy::trailing_zero_sized_array_without_repr_c)] -#![feature(const_generics_defaults)] // see below // Do lint: @@ -17,14 +16,16 @@ struct GenericArrayType { last: [T; 0], } -#[derive(Debug)] -struct OnlyAnotherAttributeDerive { +#[must_use] +struct OnlyAnotherAttributeMustUse { field: i32, last: [usize; 0], } -#[must_use] -struct OnlyAnotherAttributeMustUse { +// NOTE: Unfortunately the attribute isn't included in the lint output. I'm not sure how to make it +// show up. +#[derive(Debug)] +struct OnlyAnotherAttributeDerive { field: i32, last: [usize; 0], } @@ -82,6 +83,12 @@ struct NonZeroSizedArray { last: [usize; 1], } +struct NotLastField { + f1: u32, + zero_sized: [usize; 0], + last: i32, +} + const ONE: usize = 1; struct NonZeroSizedWithConst { field: i32, @@ -133,21 +140,4 @@ enum DontLintAnonymousStructsFromDesuraging { C { x: u32, y: [u64; 0] }, } -// NOTE: including these (along with the required feature) triggers an ICE. Not sure why. Should -// make sure the const generics people are aware of that if they weren't already. - -// #[repr(C)] -// struct ConstParamOk { -// field: i32, -// last: [usize; N] -// } - -// struct ConstParamLint { -// field: i32, -// last: [usize; N] -// } - -fn main() { - let _ = OnlyAnotherAttributeMustUse { field: 0, last: [] }; - let _ = OtherAttributesMustUse { field: 0, last: [] }; -} +fn main() {} diff --git a/tests/ui/trailing_zero_sized_array_without_repr_c.stderr b/tests/ui/trailing_zero_sized_array_without_repr_c.stderr index 84606ed61..ee8182cdc 100644 --- a/tests/ui/trailing_zero_sized_array_without_repr_c.stderr +++ b/tests/ui/trailing_zero_sized_array_without_repr_c.stderr @@ -1,5 +1,5 @@ error: trailing zero-sized array in a struct which is not marked `#[repr(C)]` - --> $DIR/trailing_zero_sized_array_without_repr_c.rs:4:1 + --> $DIR/trailing_zero_sized_array_without_repr_c.rs:5:1 | LL | / struct RarelyUseful { LL | | field: i32, @@ -8,33 +8,20 @@ LL | | } | |_^ | = note: `-D clippy::trailing-zero-sized-array-without-repr-c` implied by `-D warnings` -help: try annotating the struct definition with `#[repr(C)]` (or another `repr` attribute): - | -LL + #[repr(C)] -LL + struct RarelyUseful { -LL + field: i32, -LL + last: [usize; 0], -LL + } - | + = help: consider annotating the struct definition with `#[repr(C)]` (or another `repr` attribute) error: trailing zero-sized array in a struct which is not marked `#[repr(C)]` - --> $DIR/trailing_zero_sized_array_without_repr_c.rs:15:1 + --> $DIR/trailing_zero_sized_array_without_repr_c.rs:10:1 | -LL | / struct OnlyFieldIsZeroSizeArray { +LL | / struct OnlyField { LL | | first_and_last: [usize; 0], LL | | } | |_^ | -help: try annotating the struct definition with `#[repr(C)]` (or another `repr` attribute): - | -LL + #[repr(C)] -LL + struct OnlyFieldIsZeroSizeArray { -LL + first_and_last: [usize; 0], -LL + } - | + = help: consider annotating the struct definition with `#[repr(C)]` (or another `repr` attribute) error: trailing zero-sized array in a struct which is not marked `#[repr(C)]` - --> $DIR/trailing_zero_sized_array_without_repr_c.rs:19:1 + --> $DIR/trailing_zero_sized_array_without_repr_c.rs:14:1 | LL | / struct GenericArrayType { LL | | field: i32, @@ -42,53 +29,55 @@ LL | | last: [T; 0], LL | | } | |_^ | -help: try annotating the struct definition with `#[repr(C)]` (or another `repr` attribute): - | -LL + #[repr(C)] -LL + struct GenericArrayType { -LL + field: i32, -LL + last: [T; 0], -LL + } - | + = help: consider annotating the struct definition with `#[repr(C)]` (or another `repr` attribute) error: trailing zero-sized array in a struct which is not marked `#[repr(C)]` - --> $DIR/trailing_zero_sized_array_without_repr_c.rs:30:1 + --> $DIR/trailing_zero_sized_array_without_repr_c.rs:19:1 | -LL | / struct ZeroSizedFromExternalConst { +LL | / #[must_use] +LL | | struct OnlyAnotherAttributeMustUse { +LL | | field: i32, +LL | | last: [usize; 0], +LL | | } + | |_^ + | + = help: consider annotating the struct definition with `#[repr(C)]` (or another `repr` attribute) + +error: trailing zero-sized array in a struct which is not marked `#[repr(C)]` + --> $DIR/trailing_zero_sized_array_without_repr_c.rs:28:1 + | +LL | / struct OnlyAnotherAttributeDerive { +LL | | field: i32, +LL | | last: [usize; 0], +LL | | } + | |_^ + | + = help: consider annotating the struct definition with `#[repr(C)]` (or another `repr` attribute) + +error: trailing zero-sized array in a struct which is not marked `#[repr(C)]` + --> $DIR/trailing_zero_sized_array_without_repr_c.rs:34:1 + | +LL | / struct ZeroSizedWithConst { LL | | field: i32, LL | | last: [usize; ZERO], LL | | } | |_^ | -help: try annotating the struct definition with `#[repr(C)]` (or another `repr` attribute): - | -LL + #[repr(C)] -LL + struct ZeroSizedFromExternalConst { -LL + field: i32, -LL + last: [usize; ZERO], -LL + } - | + = help: consider annotating the struct definition with `#[repr(C)]` (or another `repr` attribute) error: trailing zero-sized array in a struct which is not marked `#[repr(C)]` - --> $DIR/trailing_zero_sized_array_without_repr_c.rs:45:1 + --> $DIR/trailing_zero_sized_array_without_repr_c.rs:43:1 | -LL | / struct UsingFunction { +LL | / struct ZeroSizedWithConstFunction { LL | | field: i32, LL | | last: [usize; compute_zero()], LL | | } | |_^ | -help: try annotating the struct definition with `#[repr(C)]` (or another `repr` attribute): - | -LL + #[repr(C)] -LL + struct UsingFunction { -LL + field: i32, -LL + last: [usize; compute_zero()], -LL + } - | + = help: consider annotating the struct definition with `#[repr(C)]` (or another `repr` attribute) error: trailing zero-sized array in a struct which is not marked `#[repr(C)]` - --> $DIR/trailing_zero_sized_array_without_repr_c.rs:94:1 + --> $DIR/trailing_zero_sized_array_without_repr_c.rs:48:1 | LL | / struct LotsOfFields { LL | | f1: u32, @@ -99,15 +88,7 @@ LL | | last: [usize; 0], LL | | } | |_^ | -help: try annotating the struct definition with `#[repr(C)]` (or another `repr` attribute): - | -LL + #[repr(C)] -LL + struct LotsOfFields { -LL + f1: u32, -LL + f2: u32, -LL + f3: u32, -LL + f4: u32, - ... + = help: consider annotating the struct definition with `#[repr(C)]` (or another `repr` attribute) -error: aborting due to 6 previous errors +error: aborting due to 8 previous errors