Open Mark-Simulacrum opened 2 years ago
But you can't actually instantiate a Bar<'a>
unless 'a: 'static
(playground):
fn main() {
let s = 123;
let v = Foo { s: &s };
Bar { v };
}
results in—
error[E0597]: `s` does not live long enough --> src/main.rs:12:22 | 12 | let v = Foo { s: &s }; | ^^ borrowed value does not live long enough 13 | Bar { v }; | - copying this value requires that `s` is borrowed for `'static` 14 | } | - `s` dropped here while still borrowed For more information about this error, try `rustc --explain E0597`.
Removing A-diagnostics because I don' t think this is a diagnostics issue necessarily. I can modify the check that we use for checking the union's field are Copy
to consider regions, though.
@rustbot claim
Yeah. I think it's a breaking change potentially to change this now, so it might not be worth making any movement here.
I think in practice there's no risk here, since I believe any Copy impl does forbid all Drop impls, since Drop must be implemented modulo regions always.
But you can't actually instantiate a
Bar<'a>
unless'a: 'static
You can instantiate it, and even work with it, but you can't move the maybe-Copy
value. This is not related to the union even: for example,
fn main() {
let mut s = 123;
let v = Foo { s: &mut s };
let v = v; // Works if I comment this line
}
error[E0597]: `s` does not live long enough
--> src/main.rs:14:22
|
14 | let v = Foo { s: &mut s };
| ^^^^^^ borrowed value does not live long enough
15 | let v = v; // Works if I comment this line
| - copying this value requires that `s` is borrowed for `'static`
16 | }
| - `s` dropped here while still borrowed
@Mark-Simulacrum: In https://github.com/rust-lang/rust/issues/55149#issuecomment-1150197680, @joshtriplett said:
We'd like to close the remainder of this, and just not allow non-Copy union fields in general, changing that from a feature gate to an error.
I think promoting this to do a copy check considering regions is probably still worth considering here?
It's probably worth a crater run, at least, but I'm not sure how that comment is relevant. It would be (IMO) the right behavior, but it would also be a breaking change in theory, at least, which makes me reluctant to move ahead with it.
You can instantiate it, and even work with it
I can't find a way to instantiate with only safe code.
Doing it via unsafe code (at least the ways I've thought about, like transmute
) would violate documented safety conditions.
Is there actually some unsoundness here that I've overlooked? If so this should be I-unsound
no?
I can't find a way to instantiate with only safe code.
fn main() {
let s = 123;
let union = Bar { v: Foo { s: &s } };
}
But I don't think this deserves I-unsound since the moment you'll try to move the inner Foo
you'll get a "does not live long enough" error.
In a second thought, I'm not sure: you can impl Copy
for the union and then you can duplicate its instances and effectively also duplicate Foo
s. So maybe there is a soundness issue? We still need unsafe
code to access to the member, though, and it doesn't happen on structs.
Apologies, I can't have had enough coffee yet. Of course this isn't a soundness issue: it's only about ensuring that destructors are run, or that one is explicitly acknowledging with ManuallyDrop
that they won't be (unless, er, manually dropped). Like you say, copying the union isn't an issue because its fields can't be accessed without unsafe
.
Triage: There doesn't seem to be anything actionable here? Closing.
Why is this not actionable? Someone should probably implement this check.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4cc269cb234497a007039d6ddbc41943
It seems to me that the above code should probably be rejected, since in general
Foo<'a>
doesn't implement Copy in the above code.The compiler check is checking for "is_copy_modulo_regions" specifically https://github.com/rust-lang/rust/blob/master/compiler/rustc_passes/src/stability.rs#L780, which seems like it probably shouldn't be modulo regions?