Add bounds for WorkerLocal's Send and Sync

[ammaraskar]

See rust-lang/rust#81425

We could let the auto impl take care of Send if that would be preferable.

[cuviper]

Could you add comments to the code justifying why these bounds are correct?
I agree with the change, but we need to remember the reason. :)

[ammaraskar]

Good point, added a comment justifying the Sync bound. I think Send is fine without one since it's the obvious auto-impl version?

[cuviper]

I think Send is fine without one since it's the obvious auto-impl version?

Let's just leave it to auto-impl then, as you proposed earlier.

[ammaraskar]

Done.

[cuviper]

Hmm, combing over this again -- Debug does allow shared access to all locals. So either we do need T: Sync, or we should not print the actual values in Debug.

[ammaraskar]

Good catch, I completely looked over the Deubg impl. I've changed it so that it doesn't actually print the underlying value to avoid accessing them concurrently. Instead, it now prints the registry that the WorkerLocal is associated with.

[jyn514]

Note that this causes compile failures in rustc_middle currently:

    Checking rustc_builtin_macros v0.0.0 (/home/joshua/rustc3/compiler/rustc_builtin_macros)
error[E0277]: `*mut u8` cannot be sent between threads safely
    --> compiler/rustc_middle/src/ty/mod.rs:2788:14
     |
2788 |             .for_each(|&body_id| f(self.hir().body_owner_def_id(body_id)));
     |              ^^^^^^^^ `*mut u8` cannot be sent between threads safely
     |
     = help: within `rustc_arena::DropType`, the trait `Send` is not implemented for `*mut u8`
     = note: required because it appears within the type `rustc_arena::DropType`
     = note: required because of the requirements on the impl of `Send` for `Unique<rustc_arena::DropType>`
     = note: required because it appears within the type `RawVec<rustc_arena::DropType>`
     = note: required because it appears within the type `Vec<rustc_arena::DropType>`
     = note: required because of the requirements on the impl of `Send` for `RefCell<Vec<rustc_arena::DropType>>`
     = note: required because it appears within the type `DropArena`
     = note: required because it appears within the type `Arena<'tcx>`
     = note: required because of the requirements on the impl of `std::marker::Sync` for `WorkerLocal<Arena<'tcx>>`
     = note: required because it appears within the type `&'tcx WorkerLocal<Arena<'tcx>>`
     = note: required because it appears within the type `GlobalCtxt<'tcx>`
     = note: required because it appears within the type `&'tcx GlobalCtxt<'tcx>`
     = note: required because it appears within the type `context::TyCtxt<'tcx>`
     = note: required because it appears within the type `&context::TyCtxt<'tcx>`
     = note: required because it appears within the type `[closure@compiler/rustc_middle/src/ty/mod.rs:2788:23: 2788:74]`

error[E0277]: `*mut u8` cannot be sent between threads safely
    --> compiler/rustc_middle/src/ty/context.rs:1738:13
     |
1738 |             sync::assert_sync::<ImplicitCtxt<'_, '_>>();
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut u8` cannot be sent between threads safely
     | 
    ::: /home/joshua/rustc3/compiler/rustc_data_structures/src/sync.rs:420:32
     |
420  | pub fn assert_sync<T: ?Sized + Sync>() {}
     |                                ---- required by this bound in `assert_sync`
     |
     = help: within `rustc_arena::DropType`, the trait `Send` is not implemented for `*mut u8`
     = note: required because it appears within the type `rustc_arena::DropType`
     = note: required because of the requirements on the impl of `Send` for `Unique<rustc_arena::DropType>`
     = note: required because it appears within the type `RawVec<rustc_arena::DropType>`
     = note: required because it appears within the type `Vec<rustc_arena::DropType>`
     = note: required because of the requirements on the impl of `Send` for `RefCell<Vec<rustc_arena::DropType>>`
     = note: required because it appears within the type `DropArena`
     = note: required because it appears within the type `Arena<'_>`
     = note: required because of the requirements on the impl of `std::marker::Sync` for `WorkerLocal<Arena<'_>>`
     = note: required because it appears within the type `&WorkerLocal<Arena<'_>>`
     = note: required because it appears within the type `GlobalCtxt<'_>`
     = note: required because it appears within the type `&GlobalCtxt<'_>`
     = note: required because it appears within the type `context::TyCtxt<'_>`
     = note: required because it appears within the type `ImplicitCtxt<'_, '_>`

error: aborting due to 2 previous errors

[cuviper]

@jyn514 Yikes! That looks like a real problem that will need to be fixed in DropType and DropArena. If the arena allocates a T: !Send in a thread, I think WorkerLocal will end up dropping those collectively on its original thread, or else into_inner will allow extracting them all together.

[jyn514]

I'm just the messenger, sorry, I don't know how to fix it. I found this PR originally because I was trying to fix rust-lang/rust#82636 before realizing it needed parallel-compiler = true.

[jyn514]

It looks like it was added by @Zoxc in rust-lang/rust@43e33ea with very few changes since then.

[cuviper]

It's difficult since it uses type erasure, and it can't always require Send since parallel_compiler toggles between thread-safe types or not, like Lrc = Rc/Arc. I think it would have to base constraints on rustc_data_structures::sync::Send, and then the change here would only surface when that matches the real Send.

[cuviper]

OK, I think I have the DropArena fix figured out. I'll publish a new release and open a rust PR with that.

Thanks for the fix, @ammaraskar!