Unions require Copy fields, but ignore regions in the check

[Mark-Simulacrum]

union Bar<'a> {
    v: Foo<'a>,
}

struct Foo<'a> { s: &'a u32 }

impl Copy for Foo<'static> {}
impl Clone for Foo<'static> { fn clone(&self) -> Self { *self } }

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?

[eggyal]

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`.

[compiler-errors]

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

[Mark-Simulacrum]

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.

[ChayimFriedman2]

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
}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=e841837a7c635fb568db6814608b66cb

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

[compiler-errors]

@Mark-Simulacrum: In #55149 (comment), @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?

[Mark-Simulacrum]

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.

[eggyal]

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?

[ChayimFriedman2]

I can't find a way to instantiate with only safe code.

fn main() {
    let s = 123;
    let union = Bar { v: Foo { s: &s } };
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b8b1891b6c3f22593205919b0f070ede

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.

[ChayimFriedman2]

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 Foos. 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.

[eggyal]

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.

[Enselic]

Triage: There doesn't seem to be anything actionable here? Closing.

[compiler-errors]

Why is this not actionable? Someone should probably implement this check.