Initial impl repr_packed_without_abi

Fixes #13375

Author: lukaslueg

CHANGELOG.md

@@ -5969,6 +5969,7 @@ Released 2018-09-13
 [`repeat_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_once
 [`repeat_vec_with_capacity`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_vec_with_capacity
 [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts
+[`repr_packed_without_abi`]: https://rust-lang.github.io/rust-clippy/master/index.html#repr_packed_without_abi
 [`reserve_after_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#reserve_after_initialization
 [`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs
 [`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used

clippy_lints/src/attrs/mod.rs

@@ -7,6 +7,7 @@ mod duplicated_attributes;
 mod inline_always;
 mod mixed_attributes_style;
 mod non_minimal_cfg;
+mod repr_attributes;
 mod should_panic_without_expect;
 mod unnecessary_clippy_cfg;
 mod useless_attribute;
@@ -272,6 +273,44 @@ declare_clippy_lint! {
     "ensures that all `should_panic` attributes specify its expected panic message"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for items with `#[repr(packed)]`-attribute without ABI qualification
+    ///
+    /// ### Why is this bad?
+    /// Without qualification, `repr(packed)` implies `repr(Rust)`. The Rust-ABI is inherently unstable.
+    /// While this is fine as long as the type is accessed correctly within Rust-code, most uses
+    /// of `#[repr(packed)]` involve FFI and/or data structures specified by network-protocols or
+    /// other external specifications. In such situations, the unstable Rust-ABI implied in
+    /// `#[repr(packed)]` may lead to future bugs should the Rust-ABI change.
+    ///
+    /// In case you are relying on a well defined and stable memory layout, qualify the type's
+    /// representation using the `C`-ABI. Otherwise, if the type in question is only ever
+    /// accessed from Rust-code according to Rust's rules, use the `Rust`-ABI explicitly.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// #[repr(packed)]
+    /// struct NetworkPacketHeader {
+    ///     header_length: u8,
+    ///     header_version: u16
+    /// }
+    /// ```
+    ///
+    /// Use instead:
+    /// ```no_run
+    /// #[repr(C, packed)]
+    /// struct NetworkPacketHeader {
+    ///     header_length: u8,
+    ///     header_version: u16
+    /// }
+    /// ```
+    #[clippy::version = "1.84.0"]
+    pub REPR_PACKED_WITHOUT_ABI,
+    suspicious,
+    "ensures that `repr(packed)` always comes with a qualified ABI"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for `any` and `all` combinators in `cfg` with only one condition.
@@ -496,6 +535,7 @@ impl_lint_pass!(PostExpansionEarlyAttributes => [
     USELESS_ATTRIBUTE,
     BLANKET_CLIPPY_RESTRICTION_LINTS,
     SHOULD_PANIC_WITHOUT_EXPECT,
+    REPR_PACKED_WITHOUT_ABI,
     MIXED_ATTRIBUTES_STYLE,
     DUPLICATED_ATTRIBUTES,
 ]);
@@ -546,6 +586,7 @@ impl EarlyLintPass for PostExpansionEarlyAttributes {
 
         mixed_attributes_style::check(cx, item.span, &item.attrs);
         duplicated_attributes::check(cx, &item.attrs);
+        repr_attributes::check(cx, item.span, &item.attrs, &self.msrv);
     }
 
     extract_msrv_attr!(EarlyContext);

clippy_lints/src/attrs/repr_attributes.rs

@@ -0,0 +1,43 @@
+use rustc_ast::Attribute;
+use rustc_lint::EarlyContext;
+use rustc_span::{Span, sym};
+
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::msrvs;
+
+use super::REPR_PACKED_WITHOUT_ABI;
+
+pub(super) fn check(cx: &EarlyContext<'_>, item_span: Span, attrs: &[Attribute], msrv: &msrvs::Msrv) {
+    if msrv.meets(msrvs::REPR_RUST) {
+        check_packed(cx, item_span, attrs);
+    }
+}
+
+fn check_packed(cx: &EarlyContext<'_>, item_span: Span, attrs: &[Attribute]) {
+    if let Some(items) = attrs.iter().find_map(|attr| {
+        if attr.ident().is_some_and(|ident| matches!(ident.name, sym::repr)) {
+            attr.meta_item_list()
+        } else {
+            None
+        }
+    }) && let Some(packed) = items
+        .iter()
+        .find(|item| item.ident().is_some_and(|ident| matches!(ident.name, sym::packed)))
+        && !items.iter().any(|item| {
+            item.ident()
+                .is_some_and(|ident| matches!(ident.name, sym::C | sym::Rust))
+        })
+    {
+        span_lint_and_then(
+            cx,
+            REPR_PACKED_WITHOUT_ABI,
+            item_span,
+            "item uses `packed` representation without ABI-qualification",
+            |diag| {
+                diag.warn("unqualified `#[repr(packed)]` defaults to `#[repr(Rust, packed)]`, which has no stable ABI")
+                    .help("qualify the desired ABI explicity via `#[repr(C, packed)]` or `#[repr(Rust, packed)]`")
+                    .span_label(packed.span(), "`packed` representation set here");
+            },
+        );
+    }
+}

clippy_lints/src/declared_lints.rs

@@ -55,6 +55,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::attrs::INLINE_ALWAYS_INFO,
     crate::attrs::MIXED_ATTRIBUTES_STYLE_INFO,
     crate::attrs::NON_MINIMAL_CFG_INFO,
+    crate::attrs::REPR_PACKED_WITHOUT_ABI_INFO,
     crate::attrs::SHOULD_PANIC_WITHOUT_EXPECT_INFO,
     crate::attrs::UNNECESSARY_CLIPPY_CFG_INFO,
     crate::attrs::USELESS_ATTRIBUTE_INFO,

clippy_utils/src/msrvs.rs

@@ -24,6 +24,7 @@ msrv_aliases! {
     1,80,0 { BOX_INTO_ITER }
     1,77,0 { C_STR_LITERALS }
     1,76,0 { PTR_FROM_REF, OPTION_RESULT_INSPECT }
+    1,74,0 { REPR_RUST }
     1,73,0 { MANUAL_DIV_CEIL }
     1,71,0 { TUPLE_ARRAY_CONVERSIONS, BUILD_HASHER_HASH_ONE }
     1,70,0 { OPTION_RESULT_IS_VARIANT_AND, BINARY_HEAP_RETAIN }

tests/ui/default_union_representation.rs

@@ -1,5 +1,6 @@
 #![feature(transparent_unions)]
 #![warn(clippy::default_union_representation)]
+#![allow(clippy::repr_packed_without_abi)]
 
 union NoAttribute {
     //~^ ERROR: this union has the default representation

tests/ui/default_union_representation.stderr

@@ -1,5 +1,5 @@
 error: this union has the default representation
-  --> tests/ui/default_union_representation.rs:4:1
+  --> tests/ui/default_union_representation.rs:5:1
    |
 LL | / union NoAttribute {
 LL | |
@@ -13,7 +13,7 @@ LL | | }
    = help: to override `-D warnings` add `#[allow(clippy::default_union_representation)]`
 
 error: this union has the default representation
-  --> tests/ui/default_union_representation.rs:17:1
+  --> tests/ui/default_union_representation.rs:18:1
    |
 LL | / union ReprPacked {
 LL | |
@@ -25,7 +25,7 @@ LL | | }
    = help: consider annotating `ReprPacked` with `#[repr(C)]` to explicitly specify memory layout
 
 error: this union has the default representation
-  --> tests/ui/default_union_representation.rs:36:1
+  --> tests/ui/default_union_representation.rs:37:1
    |
 LL | / union ReprAlign {
 LL | |
@@ -37,7 +37,7 @@ LL | | }
    = help: consider annotating `ReprAlign` with `#[repr(C)]` to explicitly specify memory layout
 
 error: this union has the default representation
-  --> tests/ui/default_union_representation.rs:57:1
+  --> tests/ui/default_union_representation.rs:58:1
    |
 LL | / union ZSTAndTwoFields {
 LL | |

tests/ui/derive.rs

@@ -2,6 +2,7 @@
     clippy::non_canonical_clone_impl,
     clippy::non_canonical_partial_ord_impl,
     clippy::needless_lifetimes,
+    clippy::repr_packed_without_abi,
     dead_code
 )]
 #![warn(clippy::expl_impl_clone_on_copy)]

tests/ui/derive.stderr

@@ -1,5 +1,5 @@
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> tests/ui/derive.rs:12:1
+  --> tests/ui/derive.rs:13:1
    |
 LL | / impl Clone for Qux {
 LL | |
@@ -10,7 +10,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> tests/ui/derive.rs:12:1
+  --> tests/ui/derive.rs:13:1
    |
 LL | / impl Clone for Qux {
 LL | |
@@ -23,7 +23,7 @@ LL | | }
    = help: to override `-D warnings` add `#[allow(clippy::expl_impl_clone_on_copy)]`
 
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> tests/ui/derive.rs:37:1
+  --> tests/ui/derive.rs:38:1
    |
 LL | / impl<'a> Clone for Lt<'a> {
 LL | |
@@ -34,7 +34,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> tests/ui/derive.rs:37:1
+  --> tests/ui/derive.rs:38:1
    |
 LL | / impl<'a> Clone for Lt<'a> {
 LL | |
@@ -45,7 +45,7 @@ LL | | }
    | |_^
 
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> tests/ui/derive.rs:49:1
+  --> tests/ui/derive.rs:50:1
    |
 LL | / impl Clone for BigArray {
 LL | |
@@ -56,7 +56,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> tests/ui/derive.rs:49:1
+  --> tests/ui/derive.rs:50:1
    |
 LL | / impl Clone for BigArray {
 LL | |
@@ -67,7 +67,7 @@ LL | | }
    | |_^
 
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> tests/ui/derive.rs:61:1
+  --> tests/ui/derive.rs:62:1
    |
 LL | / impl Clone for FnPtr {
 LL | |
@@ -78,7 +78,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> tests/ui/derive.rs:61:1
+  --> tests/ui/derive.rs:62:1
    |
 LL | / impl Clone for FnPtr {
 LL | |
@@ -89,7 +89,7 @@ LL | | }
    | |_^
 
 error: you are implementing `Clone` explicitly on a `Copy` type
-  --> tests/ui/derive.rs:82:1
+  --> tests/ui/derive.rs:83:1
    |
 LL | / impl<T: Clone> Clone for Generic2<T> {
 LL | |
@@ -100,7 +100,7 @@ LL | | }
    | |_^
    |
 note: consider deriving `Clone` or removing `Copy`
-  --> tests/ui/derive.rs:82:1
+  --> tests/ui/derive.rs:83:1
    |
 LL | / impl<T: Clone> Clone for Generic2<T> {
 LL | |

tests/ui/repr_packed_without_abi.rs

@@ -0,0 +1,37 @@
+#![deny(clippy::repr_packed_without_abi)]
+
+#[repr(packed)]
+struct NetworkPacketHeader {
+    header_length: u8,
+    header_version: u16,
+}
+
+#[repr(packed)]
+union Foo {
+    a: u8,
+    b: u16,
+}
+
+#[repr(C, packed)]
+struct NoLintCNetworkPacketHeader {
+    header_length: u8,
+    header_version: u16,
+}
+
+#[repr(Rust, packed)]
+struct NoLintRustNetworkPacketHeader {
+    header_length: u8,
+    header_version: u16,
+}
+
+#[repr(packed, C)]
+union NotLintCFoo {
+    a: u8,
+    b: u16,
+}
+
+#[repr(packed, Rust)]
+union NotLintRustFoo {
+    a: u8,
+    b: u16,
+}

tests/ui/repr_packed_without_abi.stderr

@@ -0,0 +1,35 @@
+error: item uses `packed` representation without ABI-qualification
+  --> tests/ui/repr_packed_without_abi.rs:4:1
+   |
+LL |   #[repr(packed)]
+   |          ------ `packed` representation set here
+LL | / struct NetworkPacketHeader {
+LL | |     header_length: u8,
+LL | |     header_version: u16,
+LL | | }
+   | |_^
+   |
+   = warning: unqualified `#[repr(packed)]` defaults to `#[repr(Rust, packed)]`, which has no stable ABI
+   = help: qualify the desired ABI explicity via `#[repr(C, packed)]` or `#[repr(Rust, packed)]`
+note: the lint level is defined here
+  --> tests/ui/repr_packed_without_abi.rs:1:9
+   |
+LL | #![deny(clippy::repr_packed_without_abi)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: item uses `packed` representation without ABI-qualification
+  --> tests/ui/repr_packed_without_abi.rs:10:1
+   |
+LL |   #[repr(packed)]
+   |          ------ `packed` representation set here
+LL | / union Foo {
+LL | |     a: u8,
+LL | |     b: u16,
+LL | | }
+   | |_^
+   |
+   = warning: unqualified `#[repr(packed)]` defaults to `#[repr(Rust, packed)]`, which has no stable ABI
+   = help: qualify the desired ABI explicity via `#[repr(C, packed)]` or `#[repr(Rust, packed)]`
+
+error: aborting due to 2 previous errors
+

tests/ui/trailing_empty_array.rs

@@ -1,4 +1,5 @@
 #![warn(clippy::trailing_empty_array)]
+#![allow(clippy::repr_packed_without_abi)]
 
 // Do lint: