Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint: repr(packed) without ABI qualification #13375

Closed
Manishearth opened this issue Sep 9, 2024 · 1 comment · Fixed by #13398
Closed

New lint: repr(packed) without ABI qualification #13375

Manishearth opened this issue Sep 9, 2024 · 1 comment · Fixed by #13398
Labels
A-lint Area: New lints

Comments

@Manishearth
Copy link
Member

What it does

This lints against #[repr(packed)] without specifying repr(C, packed) or repr(Rust, packed).

Advantage

repr(packed) by default implies repr(Rust, packed). However, this representation is not stable, which makes it less useful: the bulk of repr(packed) use cases are for interfacing with certain binary formats (where it needs to be repr(C, packed)) and for zero-copy deserialization (which needs to be stable). The best use case for repr(Rust, packed) is for reducing binary size

In fact, it is my experience that people tend to assume repr(packed) means "the same as C __attribute__((packed))", since it doesn't actually make much sense to introduce a new packed repr.

This means some of the ecosystem is using #[repr(packed)] to mean #[repr(C, packed)], and getting broken by changes like rust-lang/rust#125360.

It seems like good practice for everyone to explicitly specify if they want C or Rust when asking for repr(packed). Probably a "suspicious" category lint: it's not guaranteed broken, but it's likely to be, and it's also just misleading.

Drawbacks

No response

Example

#[repr(packed)]
pub struct Foo {
 ...
}

Could be written as:

#[repr(C, packed)]
pub struct Foo {
 ...
}
@Manishearth Manishearth added the A-lint Area: New lints label Sep 9, 2024
@GnomedDev
Copy link
Contributor

Even as someone who predominantely uses repr(packed) for memory usage optimisation, this sounds good.

lukaslueg added a commit to lukaslueg/rust-clippy that referenced this issue Dec 15, 2024
lukaslueg added a commit to lukaslueg/rust-clippy that referenced this issue Dec 15, 2024
github-merge-queue bot pushed a commit that referenced this issue Dec 16, 2024
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
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants