rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
93.93k stars 12.09k forks source link

regression: the type `(dyn Any + 'static)` may contain interior mutability #125193

Closed BoxyUwU closed 1 week ago

BoxyUwU commented 2 weeks ago
lqd commented 2 weeks ago

I'll need to re-check later but this seems to bisect to #123203 somehow.

inquisitivecrystal commented 2 weeks ago

I'll need to re-check later but this seems to bisect to #123203 somehow.

The UI test changes from that PR appear to back this up, as confusing as it is.

lqd commented 2 weeks ago

I'm not sure what's going on in that crate, if it built successfully by mistake, or how that PR interacts with it, so let's cc the PR author @jkarneges and reviewer @Amanieu for validation.

lukas-code commented 2 weeks ago

After https://github.com/rust-lang/rust/pull/123203, Context no longer implements UnwindSafe and RefUnwindSafe, because the context now contains a &mut dyn Any which doesn't implement those traits. So this is an issue with the standard library and not the crate being built.

Repro: playground

use std::panic::UnwindSafe;
use std::task::Context;

fn unwind_safe<T: UnwindSafe>() {}

fn main() {
    unwind_safe::<Context<'_>>(); // test UnwindSafe
    unwind_safe::<&Context<'_>>(); // test RefUnwindSafe
}

@rustbot label -T-compiler +T-libs +S-has-mcve

apiraino commented 1 week ago

WG-prioritization assigning priority (Zulip discussion).

Noticing this comment on #123303 which says that was meant to be confined on nightly.

Unsure about the fallout of this regression but for now marking as critical for tracking purposes.

@rustbot label -I-prioritize +P-critical

Amanieu commented 1 week ago

Revert up: https://github.com/rust-lang/rust/pull/125377

jkarneges commented 1 week ago

Isn't Context already not unwind safe?

workingjubilee commented 1 week ago

@jkarneges That line was added by your PR, so while what you say is technically true, talking about what "already happened" when we are traveling back and forth through time via version control may be more confusing than helpful.

jkarneges commented 1 week ago

That line was already there. I added the line below it though. Original PR: https://github.com/rust-lang/rust/pull/123203/files#diff-c5319e149ad3e9d38c996d031a756a8437a533ea9f8bbb2cdfdb8f4c4a423b82

workingjubilee commented 1 week ago

Apologies, I guess I can't count numbers today?

Hm, so, the full error of the regression is this:

error[E0277]: the type `(dyn Any + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
  --> src/main.rs:13:1
   |
13 | / lucet_hostcalls! {
14 | |     #[no_mangle]
15 | |     pub unsafe extern "C" fn two(&mut _vmctx,) -> *mut AssertUnwindSafe<Box<dyn Future<Output = u64>>> {
16 | |         async fn body() -> u64 {
...  |
59 | |     }
60 | | }
   | | ^
   | | |
   | |_`(dyn Any + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   |   required by a bound introduced by this  @call
   |
   = help: within `Context<'_>`, the trait `RefUnwindSafe` is not implemented for `(dyn Any + 'static)`, which is required by `{closure@/home/jubilee/.cargo/registry/src/index.crates.io-6f17d22bba15001f/lucet-runtime-internals-0.4.1/src/hostcall_macros.rs:54:56: 54:63}: UnwindSafe`
   = note: required because it appears within the type `&mut (dyn Any + 'static)`
note: required because it appears within the type `core::task::wake::ExtData<'_>`
  --> /rustc/a26981974230110fa8fb15e1cf04d05b9a2103f9/library/core/src/task/wake.rs:225:6
note: required because it appears within the type `Context<'_>`
  --> /rustc/a26981974230110fa8fb15e1cf04d05b9a2103f9/library/core/src/task/wake.rs:236:12
   = note: required for `*mut Context<'_>` to implement `UnwindSafe`

Note that it's trying to make a pointer implement UnwindSafe. And for what it's worth, stable does indeed document Context<'_> is UnwindSafe. What implements !UnwindSafe though, also on stable, is &mut T.

Thus unfortunately that line you pointed to, as opposed to the one I thought you were referring to, is effectively just documenting that all &mut T are !UnwindSafe, and that will include Context<'_> regardless of whether it is is UnwindSafe.

And confusingly, *mut T is UnwindSafe if T is RefUnwindSafe, unlike &mut T.

workingjubilee commented 1 week ago

I have opened #125392 as an alternative.