Skip to content

Conversation

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 30, 2025

Context: rust-lang/unsafe-code-guidelines#552

This experiments with a suggestion by @RustyYato to stop considering repr(C) types as 1-ZST for the purpose of repr(transparent). If we go with rust-lang/rfcs#3845 (or another approach for fixing repr(C)), they will anyway not be ZST on all targets any more, so this removes a portability hazard. Furthermore, zero-sized repr(C) structs may have to be treated as non-ZST for the win64 ABI (at least that's what gcc/clang do), so allowing them to be ignored in repr(transparent) types is not entirely coherent.

Turns out we already have an FCW for repr(transparent), namely #78586. This extends that lint to also check for repr(C).

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 30, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
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

@RalfJung
Copy link
Member Author

@bors try

rust-bors bot added a commit that referenced this pull request Sep 30, 2025
@rust-bors

This comment has been minimized.

@RalfJung RalfJung changed the title Repr c not zst repr(transparent): do not consider repr(C) types to be 1-ZST Sep 30, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Sep 30, 2025

☀️ Try build successful (CI)
Build commit: 0de49ae (0de49ae9e81f9a1e7df6f0783824ce94ed18e8a9, parent: a2db9280539229a3b8a084a09886670a57bc7e9c)

@petrochenkov
Copy link
Contributor

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-147185 created and queued.
🤖 Automatically detected try build 0de49ae
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-147185 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-147185 is completed!
📊 8 regressed and 3 fixed (708741 total)
📊 1547 spurious results on the retry-regessed-list.txt, consider a retry1 if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

  1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-147185/retry-regressed-list.txt

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 3, 2025
@RalfJung
Copy link
Member Author

RalfJung commented Oct 5, 2025

This found

  • a few cases of abi_stable-0.9.3, an outdated crate version where the lint probably has been fixed in more recent versions (and I highly doubt there's any repr(C) here, this is going to be about the non-exhaustive part of the lint)
  • dynamic_graph-0.1.5, a crate that didn't have any updates in 4 years, here, involving a use of generativity::Id. I don't actually understand why this happens as the Id type is repr(transparent) itself... but nothing here is repr(C) so this is also almost certainly about the non-exhaustive part of the lint, not the repr(C) part.

EDIT: It later got confirmed that these crates indeed already trigger the lint before this PR.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 5, 2025

@bors try

rust-bors bot added a commit that referenced this pull request Oct 5, 2025
repr(transparent): do not consider repr(C) types to be 1-ZST
@rust-bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Oct 5, 2025

☀️ Try build successful (CI)
Build commit: e19b5cc (e19b5cc0f024358aaaecf2776a5291fa5a95b623, parent: e2c96cc06bdbdbc6f59c7551194d6a742260d6ff)

@RalfJung
Copy link
Member Author

RalfJung commented Oct 5, 2025

@craterbot
Copy link
Collaborator

👌 Experiment pr-147185-1 created and queued.
🤖 Automatically detected try build e19b5cc
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-147185-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-147185-1 is completed!
📊 6 regressed and 0 fixed (1534 total)
📊 90 spurious results on the retry-regessed-list.txt, consider a retry1 if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

  1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-147185-1/retry-regressed-list.txt

@RalfJung
Copy link
Member Author

@petrochenkov @rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2025
@RalfJung RalfJung removed the S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. label Oct 27, 2025
@petrochenkov
Copy link
Contributor

r=me with the outdated comment updated.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2025
@RalfJung
Copy link
Member Author

Thanks!

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Oct 27, 2025

📌 Commit b9b29c4 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 27, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 28, 2025
…enkov

repr(transparent): do not consider repr(C) types to be 1-ZST

Context: rust-lang/unsafe-code-guidelines#552

This experiments with a [suggestion](rust-lang/rfcs#3845 (comment)) by `@RustyYato` to stop considering repr(C) types as 1-ZST for the purpose of repr(transparent). If we go with rust-lang/rfcs#3845 (or another approach for fixing repr(C)), they will anyway not be ZST on all targets any more, so this removes a portability hazard. Furthermore, zero-sized repr(C) structs [may have to be treated](rust-lang/unsafe-code-guidelines#552 (comment)) as non-ZST for the win64 ABI (at least that's what gcc/clang do), so allowing them to be ignored in repr(transparent) types is not entirely coherent.

Turns out we already have an FCW for repr(transparent), namely rust-lang#78586. This extends that lint to also check for repr(C).
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 28, 2025
…enkov

repr(transparent): do not consider repr(C) types to be 1-ZST

Context: rust-lang/unsafe-code-guidelines#552

This experiments with a [suggestion](rust-lang/rfcs#3845 (comment)) by ``@RustyYato`` to stop considering repr(C) types as 1-ZST for the purpose of repr(transparent). If we go with rust-lang/rfcs#3845 (or another approach for fixing repr(C)), they will anyway not be ZST on all targets any more, so this removes a portability hazard. Furthermore, zero-sized repr(C) structs [may have to be treated](rust-lang/unsafe-code-guidelines#552 (comment)) as non-ZST for the win64 ABI (at least that's what gcc/clang do), so allowing them to be ignored in repr(transparent) types is not entirely coherent.

Turns out we already have an FCW for repr(transparent), namely rust-lang#78586. This extends that lint to also check for repr(C).
bors added a commit that referenced this pull request Oct 28, 2025
Rollup of 14 pull requests

Successful merges:

 - #144936 (CFI: Fix types that implement Fn, FnMut, or FnOnce)
 - #147185 (repr(transparent): do not consider repr(C) types to be 1-ZST)
 - #147840 (Rework unsizing coercions in the new solver)
 - #147915 (Update target maintainers android.md)
 - #148013 (1.91.0 release notes)
 - #148044 (compiletest: show output in debug logging)
 - #148057 (tests/ui/sanitizer/hwaddress.rs: Run on aarch64 and remove cgu hack)
 - #148139 (Add `coverage` scope for controlling paths in code coverage)
 - #148154 (Add a mailmap entry)
 - #148158 (ci: loongarch64: use medium code model to avoid relocation overflows)
 - #148166 (Re-enable macro-stepping test for AArch64)
 - #148172 (rustc-dev-guide subtree update)
 - #148175 (Fix typos: duplicate words in comments)
 - #148186 (rustdoc-search: add an integration test for CCI)

Failed merges:

 - #147935 (Add LLVM realtime sanitizer)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ef8003b into rust-lang:master Oct 28, 2025
11 checks passed
@rustbot rustbot added this to the 1.93.0 milestone Oct 28, 2025
rust-timer added a commit that referenced this pull request Oct 28, 2025
Rollup merge of #147185 - RalfJung:repr-c-not-zst, r=petrochenkov

repr(transparent): do not consider repr(C) types to be 1-ZST

Context: rust-lang/unsafe-code-guidelines#552

This experiments with a [suggestion](rust-lang/rfcs#3845 (comment)) by ```@RustyYato``` to stop considering repr(C) types as 1-ZST for the purpose of repr(transparent). If we go with rust-lang/rfcs#3845 (or another approach for fixing repr(C)), they will anyway not be ZST on all targets any more, so this removes a portability hazard. Furthermore, zero-sized repr(C) structs [may have to be treated](rust-lang/unsafe-code-guidelines#552 (comment)) as non-ZST for the win64 ABI (at least that's what gcc/clang do), so allowing them to be ignored in repr(transparent) types is not entirely coherent.

Turns out we already have an FCW for repr(transparent), namely #78586. This extends that lint to also check for repr(C).
@Zalathar
Copy link
Contributor

This appears to be the cause of a few perf regressions: #148202 (comment)

@RalfJung
Copy link
Member Author

Are you sure? This code only affects the repr(transparent) check, which shouldn't run that often -- and it only affects it in fairly trivial ways, too; def.repr().c() is a trivial flag check.

We do run that iterator multiple times, but that was already the case before this PR.

@RalfJung RalfJung deleted the repr-c-not-zst branch October 29, 2025 07:32
rust-bors bot added a commit that referenced this pull request Oct 29, 2025
rust-bors bot added a commit that referenced this pull request Oct 29, 2025
rust-bors bot added a commit that referenced this pull request Oct 29, 2025
@RalfJung
Copy link
Member Author

That regression is caused by the lint rename, which triggers the "lint has been renamed" lint in syn, and it takes some work to render those nice diagnostics we have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. I-lang-radar Items that are on lang's radar and will need eventual work or consideration. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team to-announce Announce this issue on triage meeting

Projects

None yet

Development

Successfully merging this pull request may close these issues.