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

Initial impl of repr_packed_without_abi #13398

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

lukaslueg
Copy link
Contributor

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

@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2024

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 15, 2024
@bors
Copy link
Contributor

bors commented Oct 1, 2024

☔ The latest upstream changes (presumably #13286) made this pull request unmergeable. Please resolve the merge conflicts.

"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)]`")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the wait!

Before 1.74 you couldn't write repr(Rust), adding an MSRV check is an option but I think leaving it out is reasonable too since you could just disable the lint if you want repr(Rust, packed) and are targeting older versions

I'll leave that choice up to you, the rest looks great

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Force-pushed a rebase. I've added a msrv filter, although I'm not sure given the docs if msrv_aliases! is supposed to only name actual feature-names as used in rustc; as far as I can see, #[repr(Rust)] didn't get a name, so I made something up. Bumped to 1.84. Had to rebase beyond 63d0ba9, and I'm unsure if PostExpansionEarlyAttributes is where this lint should live according to 63d0ba9's intent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine to go in Attributes, the late pass

MSRV name is 👍, just has to be understandable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it over

@lukaslueg lukaslueg force-pushed the repr_packed_without_abi branch from c25cda9 to d6e7956 Compare December 15, 2024 00:11
@lukaslueg lukaslueg force-pushed the repr_packed_without_abi branch from d6e7956 to 7a80f7b Compare December 15, 2024 19:37
Copy link
Member

@Alexendoo Alexendoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Alexendoo Alexendoo added this pull request to the merge queue Dec 16, 2024
Merged via the queue into rust-lang:master with commit 8da8da8 Dec 16, 2024
9 checks passed
@lukaslueg lukaslueg deleted the repr_packed_without_abi branch December 16, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New lint: repr(packed) without ABI qualification
4 participants