-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Promote uninhabited_static lint to a hard error
#149518
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
base: main
Are you sure you want to change the base?
Promote uninhabited_static lint to a hard error
#149518
Conversation
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
T-lang nominationThis PR promotes the enum Void {}
unsafe extern {
static EXTERN: Void;
}The lint was reported as FCW for 5 years now. |
| return; | ||
| } | ||
| }; | ||
| if layout.is_uninhabited() { |
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.
This obviously doesn't account for visibility meaning if upstream makes a public type with private fields uninhabited it could break downstream.
Is that already considered to be a breaking change due to other features? I see a bunch of uses of is_uninhabited in the compiler, so maybe?
Counterexamples: Exhaustiveness checking accounts for visibility at module granularity even, #[repr(transparent)] no longer permits external types with private fields (well, it's still a future-incompat deny-by-default lint, repr_transparent_non_zst_fields).
Edit: This concern probably doesn't really matter actually, it's super theoretical. It's more important that we reject the bad statics.
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 thought about it a bit and I think for this lint specifically this is fine.
a static implies the existence of the value, so there is either
- Some library that provides a value of such type (
externstatic) - Some (accessible) function that returns a value of such type (normal
static)
In both cases making the value uncostructable would already be a breaking change.
Another way to look at this: having a static of an uninhabited type is unsound and leads to UB on basically any use of such static. We shouldn't compile such programs, period.
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.
We shouldn't compile such programs, period.
I do agree.
having a static of an uninhabited type is unsound and leads to UB on basically any use of such static
Re. supposed unsoundness tho, is that really the case? Merely defining such a static isn't insta-UB or is it? And since extern statics are unsafe to reference, no language invariant is broken in safe code, so it's not unsound; sure it is UB but that would be the user's fault.
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.
Fair enough, it slipped from my mind that those accesses are unsafe.
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.
r=me for the code changes, just waiting on lang approval.
894c449 to
0a6b3e8
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors2 try |
This comment has been minimized.
This comment has been minimized.
…=<try> Promote `uninhabited_static` lint to a hard error
|
I was debating how necessary it would be to have this be a hard error, since arguably it's "just" someone lying in their And most importantly, in any place where this fires it seems clearly like using (The meeting brought up that we should probably crater check this just in case, but I expect that to go through fine.) |
|
It's worth noting here that, though this has been an FCW for a long time, it's only set to warn and is not set to report in deps. If we were to decide to not make this a hard error immediately, it'd seem that would be the natural next step. In the meeting, we talked about how perhaps if there is no observed breakage in crater that we might be OK with going to a hard error directly regardless. |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
The lint was introduced as a FCW in #78324 and landed in rust 1.49.0 (5 years ago); see also #74840. I think we can promote this to a hard error ^^'