Initial impl of repr_packed_without_abi (#13398)

Fixes #13375

I've added the lint next to the other attribute-related ones. Not sure
if this is the correct place, since while we are looking after the
`packed`-attribute (there is nothing we can do about types defined
elsewhere), we are more concerned about the type's representation set by
the attribute (instead of "duplicate attributes" and such).

The lint simply looks at the attributes themselves without concern for
the item-kind, since items where `repr` is not allowed end up in a
compile-error anyway.

I'm somewhat concerned about the level of noise this lint would cause
if/when it goes into stable, although it does _not_ come up in
`lintcheck`.

```
changelog: [`repr_packed_without_abi`]: Initial implementation
```

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.
@@ -415,6 +454,7 @@ pub struct Attributes {
 
 impl_lint_pass!(Attributes => [
     INLINE_ALWAYS,
+    REPR_PACKED_WITHOUT_ABI,
 ]);
 
 impl Attributes {
@@ -431,6 +471,7 @@ impl<'tcx> LateLintPass<'tcx> for Attributes {
         if is_relevant_item(cx, item) {
             inline_always::check(cx, item.span, item.ident.name, attrs);
         }
+        repr_attributes::check(cx, item.span, attrs, &self.msrv);
     }
 
     fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) {

clippy_lints/src/attrs/repr_attributes.rs

@@ -0,0 +1,43 @@
+use rustc_ast::Attribute;
+use rustc_lint::LateContext;
+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: &LateContext<'_>, item_span: Span, attrs: &[Attribute], msrv: &msrvs::Msrv) {
+    if msrv.meets(msrvs::REPR_RUST) {
+        check_packed(cx, item_span, attrs);
+    }
+}
+
+fn check_packed(cx: &LateContext<'_>, 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: