Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn on structs with a trailing zero-sized array but no repr attribute #7838

Merged
merged 37 commits into from
Oct 20, 2021
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
797507c
Add boilerplate and basic tests
nhamovitz Oct 15, 2021
c69387a
Well it builds
nhamovitz Oct 15, 2021
92d3b77
ayy it compiles! ship it, right? 😎 /s
nhamovitz Oct 15, 2021
7ee8e7a
Implement detecting trailing zero-sized array
nhamovitz Oct 15, 2021
523b013
Implement getting an array of attributes!
nhamovitz Oct 16, 2021
e53a4da
it works i think (incl some `dbg`s)
nhamovitz Oct 16, 2021
4b4db59
output looks fantastic
nhamovitz Oct 16, 2021
b9948c4
Ran `dev bless`!
nhamovitz Oct 16, 2021
003972f
add multiple `get_attrs` and `includes_repr` and they all work!
nhamovitz Oct 16, 2021
5fdf934
intermediate step
nhamovitz Oct 16, 2021
9b3f55e
tried to simplify but it doesn't work :/
nhamovitz Oct 17, 2021
a3420f7
Tidy comments + tests; revert 'size-is-zero' detection
nhamovitz Oct 17, 2021
c5d3167
update testsuite and expand `if_chain`
nhamovitz Oct 18, 2021
2a5a4f0
Refactor ZS array detection again and this one seems great 👍
nhamovitz Oct 18, 2021
9f402b3
Check for tuple structs
nhamovitz Oct 18, 2021
cd68622
Tidy imports
nhamovitz Oct 18, 2021
6377fb2
Tidy import + update expected stderr
nhamovitz Oct 18, 2021
149b372
run rustfmt
nhamovitz Oct 18, 2021
7f84e3d
Rename lint
nhamovitz Oct 18, 2021
a6aa986
Rename stderr
nhamovitz Oct 18, 2021
a54dbf6
Improve doc and span messages
nhamovitz Oct 18, 2021
5b78907
Still renaming lmao
nhamovitz Oct 18, 2021
d25b4ee
One more test
nhamovitz Oct 18, 2021
ab9fa25
Better testcase names
nhamovitz Oct 18, 2021
3a41f22
Exploring emitting other sorts of `span`s
nhamovitz Oct 18, 2021
d8bacf0
All five `has_repr_attr` agree + are correct
nhamovitz Oct 18, 2021
18c863d
Improve help message
nhamovitz Oct 18, 2021
48cf9c2
Don't need `rustc_attr` anymore
nhamovitz Oct 18, 2021
d85f903
!: this is the commit that demonstrates the ICE
nhamovitz Oct 19, 2021
6303d2d
Revert "!: this is the commit that demonstrates the ICE"
nhamovitz Oct 19, 2021
c654cc5
One more test + final tidying
nhamovitz Oct 19, 2021
bc32be0
Remove comment
nhamovitz Oct 19, 2021
02b1f26
Remove explicit lifetime
nhamovitz Oct 19, 2021
4c8e816
Use real type in doc examples
nhamovitz Oct 19, 2021
5283d24
formatting 🙃
nhamovitz Oct 19, 2021
60da4c9
remove resolved note
nhamovitz Oct 19, 2021
0f9f591
Rename lint
nhamovitz Oct 19, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3021,6 +3021,7 @@ Released 2018-09-13
[`too_many_arguments`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
[`too_many_lines`]: https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines
[`toplevel_ref_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#toplevel_ref_arg
[`trailing_zero_sized_array_without_repr`]: https://rust-lang.github.io/rust-clippy/master/index.html#trailing_zero_sized_array_without_repr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, might need a better name (@camsteffen @xFrednet thoughts?)

maybe repr_rust_trailing_empty_array? still kinda long

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, I can't think of a better name, we might be able to just call it trailing_empty_array but that would leave out the condition of having no repr attribute 🤔

Btw. nice commit messages @nhamovitz it's funny to see the full range of emotions that you went, though xD. Welcome to the project ^^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing_empty_array seems fine imo actually

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Changed to trailing_empty_array! (and updated the changelog in the initial PR message)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing_empty_array_no_repr or trailing_zst_no_repr

[`trait_duplication_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#trait_duplication_in_bounds
[`transmute_bytes_to_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_bytes_to_str
[`transmute_float_to_int`]: https://rust-lang.github.io/rust-clippy/master/index.html#transmute_float_to_int
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,7 @@ store.register_lints(&[
temporary_assignment::TEMPORARY_ASSIGNMENT,
to_digit_is_some::TO_DIGIT_IS_SOME,
to_string_in_display::TO_STRING_IN_DISPLAY,
trailing_zero_sized_array_without_repr::TRAILING_ZERO_SIZED_ARRAY_WITHOUT_REPR,
trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS,
trait_bounds::TYPE_REPETITION_IN_BOUNDS,
transmute::CROSSPOINTER_TRANSMUTE,
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/lib.register_nursery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![
LintId::of(regex::TRIVIAL_REGEX),
LintId::of(strings::STRING_LIT_AS_BYTES),
LintId::of(suspicious_operation_groupings::SUSPICIOUS_OPERATION_GROUPINGS),
LintId::of(trailing_zero_sized_array_without_repr::TRAILING_ZERO_SIZED_ARRAY_WITHOUT_REPR),
LintId::of(transmute::USELESS_TRANSMUTE),
LintId::of(use_self::USE_SELF),
])
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,7 @@ mod tabs_in_doc_comments;
mod temporary_assignment;
mod to_digit_is_some;
mod to_string_in_display;
mod trailing_zero_sized_array_without_repr;
mod trait_bounds;
mod transmute;
mod transmuting_null;
Expand Down Expand Up @@ -777,6 +778,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::default()));
store.register_late_pass(|| Box::new(match_str_case_mismatch::MatchStrCaseMismatch));
store.register_late_pass(move || Box::new(format_args::FormatArgs));
store.register_late_pass(|| Box::new(trailing_zero_sized_array_without_repr::TrailingZeroSizedArrayWithoutRepr));

}

#[rustfmt::skip]
Expand Down
77 changes: 77 additions & 0 deletions clippy_lints/src/trailing_zero_sized_array_without_repr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
use clippy_utils::diagnostics::span_lint_and_help;
use rustc_hir::{HirId, Item, ItemKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::Const;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::sym;

declare_clippy_lint! {
/// ### What it does
/// Displays a warning when a struct with a trailing zero-sized array is declared without a `repr` attribute.
///
/// ### Why is this bad?
/// Zero-sized arrays aren't very useful in Rust itself, so such a struct is likely being created to pass to C code or in some other situation where control over memory layout matters (for example, in conjuction with manual allocation to make it easy to compute the offset of the array). Either way, `#[repr(C)]` (or another `repr` attribute) is needed.
///
/// ### Example
/// ```rust
/// struct RarelyUseful {
/// some_field: u32,
/// last: [u32; 0],
/// }
/// ```
///
/// Use instead:
/// ```rust
/// #[repr(C)]
/// struct MoreOftenUseful {
/// some_field: usize,
/// last: [u32; 0],
/// }
/// ```
pub TRAILING_ZERO_SIZED_ARRAY_WITHOUT_REPR,
nursery,
"struct with a trailing zero-sized array but without `#[repr(C)]` or another `repr` attribute"
}
declare_lint_pass!(TrailingZeroSizedArrayWithoutRepr => [TRAILING_ZERO_SIZED_ARRAY_WITHOUT_REPR]);

impl<'tcx> LateLintPass<'tcx> for TrailingZeroSizedArrayWithoutRepr {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
if is_struct_with_trailing_zero_sized_array(cx, item) && !has_repr_attr(cx, item.hir_id()) {
span_lint_and_help(
cx,
TRAILING_ZERO_SIZED_ARRAY_WITHOUT_REPR,
item.span,
"trailing zero-sized array in a struct which is not marked with a `repr` attribute",
None,
&format!(
"consider annotating `{}` with `#[repr(C)]` or another `repr` attribute",
cx.tcx.def_path_str(item.def_id.to_def_id())
),
);
}
}
}

fn is_struct_with_trailing_zero_sized_array(cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) -> bool {
if_chain! {
// First check if last field is an array
if let ItemKind::Struct(data, _) = &item.kind;
if let Some(last_field) = data.fields().last();
if let rustc_hir::TyKind::Array(_, length) = last_field.ty.kind;

// Then 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);
let length = length.try_eval_usize(cx.tcx, cx.param_env);
if let Some(length) = length;
then {
length == 0
} else {
false
}
}
}

fn has_repr_attr(cx: &LateContext<'_>, hir_id: HirId) -> bool {
cx.tcx.hir().attrs(hir_id).iter().any(|attr| attr.has_name(sym::repr))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 if you want to look (or I can go find them).

}
188 changes: 188 additions & 0 deletions tests/ui/trailing_zero_sized_array_without_repr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
#![warn(clippy::trailing_zero_sized_array_without_repr)]
#![feature(const_generics_defaults)]

// Do lint:

struct RarelyUseful {
field: i32,
last: [usize; 0],
}

struct OnlyField {
first_and_last: [usize; 0],
}

struct GenericArrayType<T> {
field: i32,
last: [T; 0],
}

#[must_use]
struct OnlyAnotherAttribute {
field: i32,
last: [usize; 0],
}

// NOTE: Unfortunately the attribute isn't included in the lint output. I'm not sure how to make it
// show up.
#[derive(Debug)]
struct OnlyADeriveAttribute {
field: i32,
last: [usize; 0],
}

const ZERO: usize = 0;
struct ZeroSizedWithConst {
field: i32,
last: [usize; ZERO],
}

#[allow(clippy::eq_op)]
const fn compute_zero() -> usize {
(4 + 6) - (2 * 5)
}
struct ZeroSizedWithConstFunction {
field: i32,
last: [usize; compute_zero()],
}

const fn compute_zero_from_arg(x: usize) -> usize {
x - 1
}
struct ZeroSizedWithConstFunction2 {
field: i32,
last: [usize; compute_zero_from_arg(1)],
}

struct ZeroSizedArrayWrapper([usize; 0]);

struct TupleStruct(i32, [usize; 0]);

struct LotsOfFields {
f1: u32,
f2: u32,
f3: u32,
f4: u32,
f5: u32,
f6: u32,
f7: u32,
f8: u32,
f9: u32,
f10: u32,
f11: u32,
f12: u32,
f13: u32,
f14: u32,
f15: u32,
f16: u32,
last: [usize; 0],
}

// Don't lint

#[repr(C)]
struct GoodReason {
field: i32,
last: [usize; 0],
}

#[repr(C)]
struct OnlyFieldWithReprC {
first_and_last: [usize; 0],
}

struct NonZeroSizedArray {
field: i32,
last: [usize; 1],
}

struct NotLastField {
f1: u32,
zero_sized: [usize; 0],
last: i32,
}

const ONE: usize = 1;
struct NonZeroSizedWithConst {
field: i32,
last: [usize; ONE],
}

#[derive(Debug)]
#[repr(C)]
struct AlsoADeriveAttribute {
field: i32,
last: [usize; 0],
}

#[must_use]
#[repr(C)]
struct AlsoAnotherAttribute {
field: i32,
last: [usize; 0],
}

#[repr(packed)]
struct ReprPacked {
field: i32,
last: [usize; 0],
}

#[repr(C, packed)]
struct ReprCPacked {
field: i32,
last: [usize; 0],
}

#[repr(align(64))]
struct ReprAlign {
field: i32,
last: [usize; 0],
}
#[repr(C, align(64))]
struct ReprCAlign {
field: i32,
last: [usize; 0],
}

// NOTE: because of https://doc.rust-lang.org/stable/reference/type-layout.html#primitive-representation-of-enums-with-fields and I'm not sure when in the compilation pipeline that would happen
#[repr(C)]
enum DontLintAnonymousStructsFromDesuraging {
A(u32),
B(f32, [u64; 0]),
C { x: u32, y: [u64; 0] },
}

#[repr(C)]
struct TupleStructReprC(i32, [usize; 0]);

type NamedTuple = (i32, [usize; 0]);

#[rustfmt::skip] // [rustfmt#4995](https://github.com/rust-lang/rustfmt/issues/4995)
struct ConstParamZeroDefault<const N: usize = 0> {
field: i32,
last: [usize; N],
}

struct ConstParamNoDefault<const N: usize> {
field: i32,
last: [usize; N],
}

#[rustfmt::skip]
struct ConstParamNonZeroDefault<const N: usize = 1> {
field: i32,
last: [usize; N],
}

struct TwoGenericParams<T, const N: usize> {
field: i32,
last: [T; N],
}

type A = ConstParamZeroDefault;
type B = ConstParamZeroDefault<0>;
type C = ConstParamNoDefault<0>;
type D = ConstParamNonZeroDefault<0>;

fn main() {}
Loading