Initial impl repr_packed_without_abi

lukaslueg · lukaslueg · commit 7a80f7b79033 · 2024-12-15T20:36:47.000+01:00
Fixes #13375
diff --git a/CHANGELOG.md b/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
diff --git a/clippy_lints/src/attrs/mod.rs b/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<'_>) {
diff --git a/clippy_lints/src/attrs/repr_attributes.rs b/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");
+            },
+        );
+    }
+}
diff --git a/clippy_lints/src/declared_lints.rs b/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,
diff --git a/clippy_utils/src/msrvs.rs b/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 }
diff --git a/tests/ui/default_union_representation.rs b/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
diff --git a/tests/ui/default_union_representation.stderr b/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 | |
diff --git a/tests/ui/derive.rs b/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)]
diff --git a/tests/ui/derive.stderr b/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 | |
diff --git a/tests/ui/repr_packed_without_abi.rs b/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,
+}
diff --git a/tests/ui/repr_packed_without_abi.stderr b/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
+
diff --git a/tests/ui/trailing_empty_array.rs b/tests/ui/trailing_empty_array.rs
@@ -1,4 +1,5 @@
 #![warn(clippy::trailing_empty_array)]
+#![allow(clippy::repr_packed_without_abi)]
 
 // Do lint:
 
diff --git a/tests/ui/trailing_empty_array.stderr b/tests/ui/trailing_empty_array.stderr

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	error: you are implementing `Clone` explicitly on a `Copy` type
`2`		`- --> tests/ui/derive.rs:12:1`
	`2`	`+ --> tests/ui/derive.rs:13:1`
`3`	`3`	`\|`
`4`	`4`	`LL \| / impl Clone for Qux {`
`5`	`5`	`LL \| \|`
`@@ -10,7 +10,7 @@ LL \| \| }`
`10`	`10`	`\| \|_^`
`11`	`11`	`\|`
`12`	`12`	note: consider deriving `Clone` or removing `Copy`
`13`		`- --> tests/ui/derive.rs:12:1`
	`13`	`+ --> tests/ui/derive.rs:13:1`
`14`	`14`	`\|`
`15`	`15`	`LL \| / impl Clone for Qux {`
`16`	`16`	`LL \| \|`
`@@ -23,7 +23,7 @@ LL \| \| }`
`23`	`23`	= help: to override `-D warnings` add `#[allow(clippy::expl_impl_clone_on_copy)]`
`24`	`24`
`25`	`25`	error: you are implementing `Clone` explicitly on a `Copy` type
`26`		`- --> tests/ui/derive.rs:37:1`
	`26`	`+ --> tests/ui/derive.rs:38:1`
`27`	`27`	`\|`
`28`	`28`	`LL \| / impl<'a> Clone for Lt<'a> {`
`29`	`29`	`LL \| \|`
`@@ -34,7 +34,7 @@ LL \| \| }`
`34`	`34`	`\| \|_^`
`35`	`35`	`\|`
`36`	`36`	note: consider deriving `Clone` or removing `Copy`
`37`		`- --> tests/ui/derive.rs:37:1`
	`37`	`+ --> tests/ui/derive.rs:38:1`
`38`	`38`	`\|`
`39`	`39`	`LL \| / impl<'a> Clone for Lt<'a> {`
`40`	`40`	`LL \| \|`
`@@ -45,7 +45,7 @@ LL \| \| }`
`45`	`45`	`\| \|_^`
`46`	`46`
`47`	`47`	error: you are implementing `Clone` explicitly on a `Copy` type
`48`		`- --> tests/ui/derive.rs:49:1`
	`48`	`+ --> tests/ui/derive.rs:50:1`
`49`	`49`	`\|`
`50`	`50`	`LL \| / impl Clone for BigArray {`
`51`	`51`	`LL \| \|`
`@@ -56,7 +56,7 @@ LL \| \| }`
`56`	`56`	`\| \|_^`
`57`	`57`	`\|`
`58`	`58`	note: consider deriving `Clone` or removing `Copy`
`59`		`- --> tests/ui/derive.rs:49:1`
	`59`	`+ --> tests/ui/derive.rs:50:1`
`60`	`60`	`\|`
`61`	`61`	`LL \| / impl Clone for BigArray {`
`62`	`62`	`LL \| \|`
`@@ -67,7 +67,7 @@ LL \| \| }`
`67`	`67`	`\| \|_^`
`68`	`68`
`69`	`69`	error: you are implementing `Clone` explicitly on a `Copy` type
`70`		`- --> tests/ui/derive.rs:61:1`
	`70`	`+ --> tests/ui/derive.rs:62:1`
`71`	`71`	`\|`
`72`	`72`	`LL \| / impl Clone for FnPtr {`
`73`	`73`	`LL \| \|`
`@@ -78,7 +78,7 @@ LL \| \| }`
`78`	`78`	`\| \|_^`
`79`	`79`	`\|`
`80`	`80`	note: consider deriving `Clone` or removing `Copy`
`81`		`- --> tests/ui/derive.rs:61:1`
	`81`	`+ --> tests/ui/derive.rs:62:1`
`82`	`82`	`\|`
`83`	`83`	`LL \| / impl Clone for FnPtr {`
`84`	`84`	`LL \| \|`
`@@ -89,7 +89,7 @@ LL \| \| }`
`89`	`89`	`\| \|_^`
`90`	`90`
`91`	`91`	error: you are implementing `Clone` explicitly on a `Copy` type
`92`		`- --> tests/ui/derive.rs:82:1`
	`92`	`+ --> tests/ui/derive.rs:83:1`
`93`	`93`	`\|`
`94`	`94`	`LL \| / impl<T: Clone> Clone for Generic2<T> {`
`95`	`95`	`LL \| \|`
`@@ -100,7 +100,7 @@ LL \| \| }`
`100`	`100`	`\| \|_^`
`101`	`101`	`\|`
`102`	`102`	note: consider deriving `Clone` or removing `Copy`
`103`		`- --> tests/ui/derive.rs:82:1`
	`103`	`+ --> tests/ui/derive.rs:83:1`
`104`	`104`	`\|`
`105`	`105`	`LL \| / impl<T: Clone> Clone for Generic2<T> {`
`106`	`106`	`LL \| \|`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,5 @@`
`1`	`1`	`#![warn(clippy::trailing_empty_array)]`
	`2`	`+#![allow(clippy::repr_packed_without_abi)]`
`2`	`3`
`3`	`4`	`// Do lint:`
`4`	`5`