-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Use #[non_exhaustive]
where appropriate
#86592
Conversation
Due to the std/alloc split, it is not possible to make `alloc::collections::TryReserveError::AllocError` non-exhaustive without having an unstable, doc-hidden method to construct (which negates the benefits from `#[non_exhaustive]`.
LGTM 👍 |
📌 Commit 3f14f4b has been approved by |
Use `#[non_exhaustive]` where appropriate Due to the std/alloc split, it is not possible to make `alloc::collections::TryReserveError::AllocError` non-exhaustive without having an unstable, doc-hidden method to construct (which negates the benefits from `#[non_exhaustive]`). `@rustbot` label +C-cleanup +T-libs +S-waiting-on-review
Use `#[non_exhaustive]` where appropriate Due to the std/alloc split, it is not possible to make `alloc::collections::TryReserveError::AllocError` non-exhaustive without having an unstable, doc-hidden method to construct (which negates the benefits from `#[non_exhaustive]`). `@rustbot` label +C-cleanup +T-libs +S-waiting-on-review
_priv: (), | ||
} | ||
#[non_exhaustive] | ||
pub struct Guard; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this permit construction of Guard
by users who can't access Guard's fields? Like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inside of a crate, #[non_exhaustive]
has no effect. To external users, you can effectively pretend there's a hidden private field. There's an RFC and presumably a fair amount of documentation about what this feature is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. My concern still applies equally much. rustc_metadata
has a bunch of stuff in it, not just the dynamic loading utilities, and so there's still value in not (easily) allowing other code in rustc_metadata
to accidentally shoot their foot off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose, but I don't particularly see that being an issue given the code reviews done on any PR.
I'm not sure I follow the motivation for most of these changes, tbh. |
Rollup of 5 pull requests Successful merges: - rust-lang#86330 (Change how edition based future compatibility warnings are handled) - rust-lang#86513 (Rustdoc: Do not list impl when trait has doc(hidden)) - rust-lang#86592 (Use `#[non_exhaustive]` where appropriate) - rust-lang#86608 (chore(rustdoc): remove unused members of RenderType) - rust-lang#86624 (Update compiler-builtins) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Due to the std/alloc split, it is not possible to make
alloc::collections::TryReserveError::AllocError
non-exhaustive without having an unstable, doc-hidden method to construct (which negates the benefits from#[non_exhaustive]
).@rustbot label +C-cleanup +T-libs +S-waiting-on-review