Auto merge of #8289 - jubnzv:unspecified-layout-union, r=camsteffen

Add `default_union_representation` lint

Closes #8235

changelog: Added a new lint  [`default_union_representation`]

CHANGELOG.md

@@ -2930,6 +2930,7 @@ Released 2018-09-13
 [`declare_interior_mutable_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#declare_interior_mutable_const
 [`default_numeric_fallback`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_numeric_fallback
 [`default_trait_access`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_trait_access
+[`default_union_representation`]: https://rust-lang.github.io/rust-clippy/master/index.html#default_union_representation
 [`deprecated_cfg_attr`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_cfg_attr
 [`deprecated_semver`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver
 [`deref_addrof`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_addrof

clippy_lints/src/default_union_representation.rs

@@ -0,0 +1,105 @@
+use clippy_utils::diagnostics::span_lint_and_help;
+use rustc_hir::{self as hir, HirId, Item, ItemKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::layout::LayoutOf;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+use rustc_typeck::hir_ty_to_ty;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Displays a warning when a union is declared with the default representation (without a `#[repr(C)]` attribute).
+    ///
+    /// ### Why is this bad?
+    /// Unions in Rust have unspecified layout by default, despite many people thinking that they
+    /// lay out each field at the start of the union (like C does). That is, there are no guarantees
+    /// about the offset of the fields for unions with multiple non-ZST fields without an explicitly
+    /// specified layout. These cases may lead to undefined behavior in unsafe blocks.
+    ///
+    /// ### Example
+    /// ```rust
+    /// union Foo {
+    ///     a: i32,
+    ///     b: u32,
+    /// }
+    ///
+    /// fn main() {
+    ///     let _x: u32 = unsafe {
+    ///         Foo { a: 0_i32 }.b // Undefined behaviour: `b` is allowed to be padding
+    ///     };
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// #[repr(C)]
+    /// union Foo {
+    ///     a: i32,
+    ///     b: u32,
+    /// }
+    ///
+    /// fn main() {
+    ///     let _x: u32 = unsafe {
+    ///         Foo { a: 0_i32 }.b // Now defined behaviour, this is just an i32 -> u32 transmute
+    ///     };
+    /// }
+    /// ```
+    #[clippy::version = "1.60.0"]
+    pub DEFAULT_UNION_REPRESENTATION,
+    restriction,
+    "unions without a `#[repr(C)]` attribute"
+}
+declare_lint_pass!(DefaultUnionRepresentation => [DEFAULT_UNION_REPRESENTATION]);
+
+impl<'tcx> LateLintPass<'tcx> for DefaultUnionRepresentation {
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
+        if is_union_with_two_non_zst_fields(cx, item) && !has_c_repr_attr(cx, item.hir_id()) {
+            span_lint_and_help(
+                cx,
+                DEFAULT_UNION_REPRESENTATION,
+                item.span,
+                "this union has the default representation",
+                None,
+                &format!(
+                    "consider annotating `{}` with `#[repr(C)]` to explicitly specify memory layout",
+                    cx.tcx.def_path_str(item.def_id.to_def_id())
+                ),
+            );
+        }
+    }
+}
+
+/// Returns true if the given item is a union with at least two non-ZST fields.
+fn is_union_with_two_non_zst_fields(cx: &LateContext<'_>, item: &Item<'_>) -> bool {
+    if let ItemKind::Union(data, _) = &item.kind {
+        data.fields().iter().filter(|f| !is_zst(cx, f.ty)).count() >= 2
+    } else {
+        false
+    }
+}
+
+fn is_zst(cx: &LateContext<'_>, hir_ty: &hir::Ty<'_>) -> bool {
+    if hir_ty.span.from_expansion() {
+        return false;
+    }
+    let ty = hir_ty_to_ty(cx.tcx, hir_ty);
+    if let Ok(layout) = cx.layout_of(ty) {
+        layout.is_zst()
+    } else {
+        false
+    }
+}
+
+fn has_c_repr_attr(cx: &LateContext<'_>, hir_id: HirId) -> bool {
+    cx.tcx.hir().attrs(hir_id).iter().any(|attr| {
+        if attr.has_name(sym::repr) {
+            if let Some(items) = attr.meta_item_list() {
+                for item in items {
+                    if item.is_word() && matches!(item.name_or_empty(), sym::C) {
+                        return true;
+                    }
+                }
+            }
+        }
+        false
+    })
+}

clippy_lints/src/lib.register_lints.rs

@@ -92,6 +92,7 @@
     default::DEFAULT_TRAIT_ACCESS,
     default::FIELD_REASSIGN_WITH_DEFAULT,
     default_numeric_fallback::DEFAULT_NUMERIC_FALLBACK,
+    default_union_representation::DEFAULT_UNION_REPRESENTATION,
     dereference::EXPLICIT_DEREF_METHODS,
     dereference::NEEDLESS_BORROW,
     dereference::REF_BINDING_TO_REFERENCE,

clippy_lints/src/lib.register_restriction.rs

@@ -12,6 +12,7 @@
     LintId::of(create_dir::CREATE_DIR),
     LintId::of(dbg_macro::DBG_MACRO),
     LintId::of(default_numeric_fallback::DEFAULT_NUMERIC_FALLBACK),
+    LintId::of(default_union_representation::DEFAULT_UNION_REPRESENTATION),
     LintId::of(disallowed_script_idents::DISALLOWED_SCRIPT_IDENTS),
     LintId::of(else_if_without_else::ELSE_IF_WITHOUT_ELSE),
     LintId::of(exhaustive_items::EXHAUSTIVE_ENUMS),

clippy_lints/src/lib.rs

@@ -189,6 +189,7 @@ macro_rules! declare_clippy_lint {
 mod dbg_macro;
 mod default;
 mod default_numeric_fallback;
+mod default_union_representation;
 mod dereference;
 mod derivable_impls;
 mod derive;
@@ -859,6 +860,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(|| Box::new(single_char_lifetime_names::SingleCharLifetimeNames));
     store.register_late_pass(move || Box::new(borrow_as_ptr::BorrowAsPtr::new(msrv)));
     store.register_late_pass(move || Box::new(manual_bits::ManualBits::new(msrv)));
+    store.register_late_pass(|| Box::new(default_union_representation::DefaultUnionRepresentation));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

tests/ui/default_union_representation.rs

@@ -0,0 +1,78 @@
+#![feature(transparent_unions)]
+#![warn(clippy::default_union_representation)]
+
+union NoAttribute {
+    a: i32,
+    b: u32,
+}
+
+#[repr(C)]
+union ReprC {
+    a: i32,
+    b: u32,
+}
+
+#[repr(packed)]
+union ReprPacked {
+    a: i32,
+    b: u32,
+}
+
+#[repr(C, packed)]
+union ReprCPacked {
+    a: i32,
+    b: u32,
+}
+
+#[repr(C, align(32))]
+union ReprCAlign {
+    a: i32,
+    b: u32,
+}
+
+#[repr(align(32))]
+union ReprAlign {
+    a: i32,
+    b: u32,
+}
+
+union SingleZST {
+    f0: (),
+}
+union ZSTsAndField1 {
+    f0: u32,
+    f1: (),
+    f2: (),
+    f3: (),
+}
+union ZSTsAndField2 {
+    f0: (),
+    f1: (),
+    f2: u32,
+    f3: (),
+}
+union ZSTAndTwoFields {
+    f0: u32,
+    f1: u64,
+    f2: (),
+}
+
+#[repr(C)]
+union CZSTAndTwoFields {
+    f0: u32,
+    f1: u64,
+    f2: (),
+}
+
+#[repr(transparent)]
+union ReprTransparent {
+    a: i32,
+}
+
+#[repr(transparent)]
+union ReprTransparentZST {
+    a: i32,
+    b: (),
+}
+
+fn main() {}

tests/ui/default_union_representation.stderr

@@ -0,0 +1,48 @@
+error: this union has the default representation
+  --> $DIR/default_union_representation.rs:4:1
+   |
+LL | / union NoAttribute {
+LL | |     a: i32,
+LL | |     b: u32,
+LL | | }
+   | |_^
+   |
+   = note: `-D clippy::default-union-representation` implied by `-D warnings`
+   = help: consider annotating `NoAttribute` with `#[repr(C)]` to explicitly specify memory layout
+
+error: this union has the default representation
+  --> $DIR/default_union_representation.rs:16:1
+   |
+LL | / union ReprPacked {
+LL | |     a: i32,
+LL | |     b: u32,
+LL | | }
+   | |_^
+   |
+   = help: consider annotating `ReprPacked` with `#[repr(C)]` to explicitly specify memory layout
+
+error: this union has the default representation
+  --> $DIR/default_union_representation.rs:34:1
+   |
+LL | / union ReprAlign {
+LL | |     a: i32,
+LL | |     b: u32,
+LL | | }
+   | |_^
+   |
+   = help: consider annotating `ReprAlign` with `#[repr(C)]` to explicitly specify memory layout
+
+error: this union has the default representation
+  --> $DIR/default_union_representation.rs:54:1
+   |
+LL | / union ZSTAndTwoFields {
+LL | |     f0: u32,
+LL | |     f1: u64,
+LL | |     f2: (),
+LL | | }
+   | |_^
+   |
+   = help: consider annotating `ZSTAndTwoFields` with `#[repr(C)]` to explicitly specify memory layout
+
+error: aborting due to 4 previous errors
+