Skip to content

Commit 8da8da8

Browse files
AlexendooAlexendoo
authored
Initial impl of repr_packed_without_abi (rust-lang#13398)
Fixes rust-lang#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 ```
2 parents 60dbda2 + 7a80f7b commit 8da8da8

13 files changed

+187
-25
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -5969,6 +5969,7 @@ Released 2018-09-13
59695969
[`repeat_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_once
59705970
[`repeat_vec_with_capacity`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_vec_with_capacity
59715971
[`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts
5972+
[`repr_packed_without_abi`]: https://rust-lang.github.io/rust-clippy/master/index.html#repr_packed_without_abi
59725973
[`reserve_after_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#reserve_after_initialization
59735974
[`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs
59745975
[`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used

‎clippy_lints/src/attrs/mod.rs

+41
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ mod duplicated_attributes;
77
mod inline_always;
88
mod mixed_attributes_style;
99
mod non_minimal_cfg;
10+
mod repr_attributes;
1011
mod should_panic_without_expect;
1112
mod unnecessary_clippy_cfg;
1213
mod useless_attribute;
@@ -272,6 +273,44 @@ declare_clippy_lint! {
272273
"ensures that all `should_panic` attributes specify its expected panic message"
273274
}
274275

276+
declare_clippy_lint! {
277+
/// ### What it does
278+
/// Checks for items with `#[repr(packed)]`-attribute without ABI qualification
279+
///
280+
/// ### Why is this bad?
281+
/// Without qualification, `repr(packed)` implies `repr(Rust)`. The Rust-ABI is inherently unstable.
282+
/// While this is fine as long as the type is accessed correctly within Rust-code, most uses
283+
/// of `#[repr(packed)]` involve FFI and/or data structures specified by network-protocols or
284+
/// other external specifications. In such situations, the unstable Rust-ABI implied in
285+
/// `#[repr(packed)]` may lead to future bugs should the Rust-ABI change.
286+
///
287+
/// In case you are relying on a well defined and stable memory layout, qualify the type's
288+
/// representation using the `C`-ABI. Otherwise, if the type in question is only ever
289+
/// accessed from Rust-code according to Rust's rules, use the `Rust`-ABI explicitly.
290+
///
291+
/// ### Example
292+
/// ```no_run
293+
/// #[repr(packed)]
294+
/// struct NetworkPacketHeader {
295+
/// header_length: u8,
296+
/// header_version: u16
297+
/// }
298+
/// ```
299+
///
300+
/// Use instead:
301+
/// ```no_run
302+
/// #[repr(C, packed)]
303+
/// struct NetworkPacketHeader {
304+
/// header_length: u8,
305+
/// header_version: u16
306+
/// }
307+
/// ```
308+
#[clippy::version = "1.84.0"]
309+
pub REPR_PACKED_WITHOUT_ABI,
310+
suspicious,
311+
"ensures that `repr(packed)` always comes with a qualified ABI"
312+
}
313+
275314
declare_clippy_lint! {
276315
/// ### What it does
277316
/// Checks for `any` and `all` combinators in `cfg` with only one condition.
@@ -415,6 +454,7 @@ pub struct Attributes {
415454

416455
impl_lint_pass!(Attributes => [
417456
INLINE_ALWAYS,
457+
REPR_PACKED_WITHOUT_ABI,
418458
]);
419459

420460
impl Attributes {
@@ -431,6 +471,7 @@ impl<'tcx> LateLintPass<'tcx> for Attributes {
431471
if is_relevant_item(cx, item) {
432472
inline_always::check(cx, item.span, item.ident.name, attrs);
433473
}
474+
repr_attributes::check(cx, item.span, attrs, &self.msrv);
434475
}
435476

436477
fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'_>) {

‎clippy_lints/src/attrs/repr_attributes.rs

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
use rustc_ast::Attribute;
2+
use rustc_lint::LateContext;
3+
use rustc_span::{Span, sym};
4+
5+
use clippy_utils::diagnostics::span_lint_and_then;
6+
use clippy_utils::msrvs;
7+
8+
use super::REPR_PACKED_WITHOUT_ABI;
9+
10+
pub(super) fn check(cx: &LateContext<'_>, item_span: Span, attrs: &[Attribute], msrv: &msrvs::Msrv) {
11+
if msrv.meets(msrvs::REPR_RUST) {
12+
check_packed(cx, item_span, attrs);
13+
}
14+
}
15+
16+
fn check_packed(cx: &LateContext<'_>, item_span: Span, attrs: &[Attribute]) {
17+
if let Some(items) = attrs.iter().find_map(|attr| {
18+
if attr.ident().is_some_and(|ident| matches!(ident.name, sym::repr)) {
19+
attr.meta_item_list()
20+
} else {
21+
None
22+
}
23+
}) && let Some(packed) = items
24+
.iter()
25+
.find(|item| item.ident().is_some_and(|ident| matches!(ident.name, sym::packed)))
26+
&& !items.iter().any(|item| {
27+
item.ident()
28+
.is_some_and(|ident| matches!(ident.name, sym::C | sym::Rust))
29+
})
30+
{
31+
span_lint_and_then(
32+
cx,
33+
REPR_PACKED_WITHOUT_ABI,
34+
item_span,
35+
"item uses `packed` representation without ABI-qualification",
36+
|diag| {
37+
diag.warn("unqualified `#[repr(packed)]` defaults to `#[repr(Rust, packed)]`, which has no stable ABI")
38+
.help("qualify the desired ABI explicity via `#[repr(C, packed)]` or `#[repr(Rust, packed)]`")
39+
.span_label(packed.span(), "`packed` representation set here");
40+
},
41+
);
42+
}
43+
}

‎clippy_lints/src/declared_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
5555
crate::attrs::INLINE_ALWAYS_INFO,
5656
crate::attrs::MIXED_ATTRIBUTES_STYLE_INFO,
5757
crate::attrs::NON_MINIMAL_CFG_INFO,
58+
crate::attrs::REPR_PACKED_WITHOUT_ABI_INFO,
5859
crate::attrs::SHOULD_PANIC_WITHOUT_EXPECT_INFO,
5960
crate::attrs::UNNECESSARY_CLIPPY_CFG_INFO,
6061
crate::attrs::USELESS_ATTRIBUTE_INFO,

‎clippy_utils/src/msrvs.rs

+1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ msrv_aliases! {
2424
1,80,0 { BOX_INTO_ITER }
2525
1,77,0 { C_STR_LITERALS }
2626
1,76,0 { PTR_FROM_REF, OPTION_RESULT_INSPECT }
27+
1,74,0 { REPR_RUST }
2728
1,73,0 { MANUAL_DIV_CEIL }
2829
1,71,0 { TUPLE_ARRAY_CONVERSIONS, BUILD_HASHER_HASH_ONE }
2930
1,70,0 { OPTION_RESULT_IS_VARIANT_AND, BINARY_HEAP_RETAIN }

‎tests/ui/default_union_representation.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#![feature(transparent_unions)]
22
#![warn(clippy::default_union_representation)]
3+
#![allow(clippy::repr_packed_without_abi)]
34

45
union NoAttribute {
56
//~^ ERROR: this union has the default representation

‎tests/ui/default_union_representation.stderr

+4-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: this union has the default representation
2-
--> tests/ui/default_union_representation.rs:4:1
2+
--> tests/ui/default_union_representation.rs:5:1
33
|
44
LL | / union NoAttribute {
55
LL | |
@@ -13,7 +13,7 @@ LL | | }
1313
= help: to override `-D warnings` add `#[allow(clippy::default_union_representation)]`
1414

1515
error: this union has the default representation
16-
--> tests/ui/default_union_representation.rs:17:1
16+
--> tests/ui/default_union_representation.rs:18:1
1717
|
1818
LL | / union ReprPacked {
1919
LL | |
@@ -25,7 +25,7 @@ LL | | }
2525
= help: consider annotating `ReprPacked` with `#[repr(C)]` to explicitly specify memory layout
2626

2727
error: this union has the default representation
28-
--> tests/ui/default_union_representation.rs:36:1
28+
--> tests/ui/default_union_representation.rs:37:1
2929
|
3030
LL | / union ReprAlign {
3131
LL | |
@@ -37,7 +37,7 @@ LL | | }
3737
= help: consider annotating `ReprAlign` with `#[repr(C)]` to explicitly specify memory layout
3838

3939
error: this union has the default representation
40-
--> tests/ui/default_union_representation.rs:57:1
40+
--> tests/ui/default_union_representation.rs:58:1
4141
|
4242
LL | / union ZSTAndTwoFields {
4343
LL | |

‎tests/ui/derive.rs

+1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
clippy::non_canonical_clone_impl,
33
clippy::non_canonical_partial_ord_impl,
44
clippy::needless_lifetimes,
5+
clippy::repr_packed_without_abi,
56
dead_code
67
)]
78
#![warn(clippy::expl_impl_clone_on_copy)]

‎tests/ui/derive.stderr

+10-10
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: you are implementing `Clone` explicitly on a `Copy` type
2-
--> tests/ui/derive.rs:12:1
2+
--> tests/ui/derive.rs:13:1
33
|
44
LL | / impl Clone for Qux {
55
LL | |
@@ -10,7 +10,7 @@ LL | | }
1010
| |_^
1111
|
1212
note: consider deriving `Clone` or removing `Copy`
13-
--> tests/ui/derive.rs:12:1
13+
--> tests/ui/derive.rs:13:1
1414
|
1515
LL | / impl Clone for Qux {
1616
LL | |
@@ -23,7 +23,7 @@ LL | | }
2323
= help: to override `-D warnings` add `#[allow(clippy::expl_impl_clone_on_copy)]`
2424

2525
error: you are implementing `Clone` explicitly on a `Copy` type
26-
--> tests/ui/derive.rs:37:1
26+
--> tests/ui/derive.rs:38:1
2727
|
2828
LL | / impl<'a> Clone for Lt<'a> {
2929
LL | |
@@ -34,7 +34,7 @@ LL | | }
3434
| |_^
3535
|
3636
note: consider deriving `Clone` or removing `Copy`
37-
--> tests/ui/derive.rs:37:1
37+
--> tests/ui/derive.rs:38:1
3838
|
3939
LL | / impl<'a> Clone for Lt<'a> {
4040
LL | |
@@ -45,7 +45,7 @@ LL | | }
4545
| |_^
4646

4747
error: you are implementing `Clone` explicitly on a `Copy` type
48-
--> tests/ui/derive.rs:49:1
48+
--> tests/ui/derive.rs:50:1
4949
|
5050
LL | / impl Clone for BigArray {
5151
LL | |
@@ -56,7 +56,7 @@ LL | | }
5656
| |_^
5757
|
5858
note: consider deriving `Clone` or removing `Copy`
59-
--> tests/ui/derive.rs:49:1
59+
--> tests/ui/derive.rs:50:1
6060
|
6161
LL | / impl Clone for BigArray {
6262
LL | |
@@ -67,7 +67,7 @@ LL | | }
6767
| |_^
6868

6969
error: you are implementing `Clone` explicitly on a `Copy` type
70-
--> tests/ui/derive.rs:61:1
70+
--> tests/ui/derive.rs:62:1
7171
|
7272
LL | / impl Clone for FnPtr {
7373
LL | |
@@ -78,7 +78,7 @@ LL | | }
7878
| |_^
7979
|
8080
note: consider deriving `Clone` or removing `Copy`
81-
--> tests/ui/derive.rs:61:1
81+
--> tests/ui/derive.rs:62:1
8282
|
8383
LL | / impl Clone for FnPtr {
8484
LL | |
@@ -89,7 +89,7 @@ LL | | }
8989
| |_^
9090

9191
error: you are implementing `Clone` explicitly on a `Copy` type
92-
--> tests/ui/derive.rs:82:1
92+
--> tests/ui/derive.rs:83:1
9393
|
9494
LL | / impl<T: Clone> Clone for Generic2<T> {
9595
LL | |
@@ -100,7 +100,7 @@ LL | | }
100100
| |_^
101101
|
102102
note: consider deriving `Clone` or removing `Copy`
103-
--> tests/ui/derive.rs:82:1
103+
--> tests/ui/derive.rs:83:1
104104
|
105105
LL | / impl<T: Clone> Clone for Generic2<T> {
106106
LL | |

‎tests/ui/repr_packed_without_abi.rs

+37
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
#![deny(clippy::repr_packed_without_abi)]
2+
3+
#[repr(packed)]
4+
struct NetworkPacketHeader {
5+
header_length: u8,
6+
header_version: u16,
7+
}
8+
9+
#[repr(packed)]
10+
union Foo {
11+
a: u8,
12+
b: u16,
13+
}
14+
15+
#[repr(C, packed)]
16+
struct NoLintCNetworkPacketHeader {
17+
header_length: u8,
18+
header_version: u16,
19+
}
20+
21+
#[repr(Rust, packed)]
22+
struct NoLintRustNetworkPacketHeader {
23+
header_length: u8,
24+
header_version: u16,
25+
}
26+
27+
#[repr(packed, C)]
28+
union NotLintCFoo {
29+
a: u8,
30+
b: u16,
31+
}
32+
33+
#[repr(packed, Rust)]
34+
union NotLintRustFoo {
35+
a: u8,
36+
b: u16,
37+
}

‎tests/ui/repr_packed_without_abi.stderr

+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
error: item uses `packed` representation without ABI-qualification
2+
--> tests/ui/repr_packed_without_abi.rs:4:1
3+
|
4+
LL | #[repr(packed)]
5+
| ------ `packed` representation set here
6+
LL | / struct NetworkPacketHeader {
7+
LL | | header_length: u8,
8+
LL | | header_version: u16,
9+
LL | | }
10+
| |_^
11+
|
12+
= warning: unqualified `#[repr(packed)]` defaults to `#[repr(Rust, packed)]`, which has no stable ABI
13+
= help: qualify the desired ABI explicity via `#[repr(C, packed)]` or `#[repr(Rust, packed)]`
14+
note: the lint level is defined here
15+
--> tests/ui/repr_packed_without_abi.rs:1:9
16+
|
17+
LL | #![deny(clippy::repr_packed_without_abi)]
18+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
19+
20+
error: item uses `packed` representation without ABI-qualification
21+
--> tests/ui/repr_packed_without_abi.rs:10:1
22+
|
23+
LL | #[repr(packed)]
24+
| ------ `packed` representation set here
25+
LL | / union Foo {
26+
LL | | a: u8,
27+
LL | | b: u16,
28+
LL | | }
29+
| |_^
30+
|
31+
= warning: unqualified `#[repr(packed)]` defaults to `#[repr(Rust, packed)]`, which has no stable ABI
32+
= help: qualify the desired ABI explicity via `#[repr(C, packed)]` or `#[repr(Rust, packed)]`
33+
34+
error: aborting due to 2 previous errors
35+

‎tests/ui/trailing_empty_array.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#![warn(clippy::trailing_empty_array)]
2+
#![allow(clippy::repr_packed_without_abi)]
23

34
// Do lint:
45

0 commit comments

Comments
 (0)