ReStatic ICE with nll and thread_local

[robsmith11]

I'm getting a ICE when compiling my crate with the current nightly. This minimal example reproduces it:

#![feature(nll)]
#![feature(thread_local)]

#[thread_local] static mut X1:u64 = 0;

struct S1 {
    a:&'static mut u64,
}

impl S1 {
    fn new(_x:u64) -> S1 {
        S1 { a:unsafe { &mut X1 } }
    }
}

fn main() {
    S1::new(0).a;
}

error: internal compiler error: unexpected region for local data ReStatic
  --> src/main.rs:12:30
   |
12 |         S1 { a:unsafe { &mut X1 } }
   |      

[robsmith11]

Note that it's the argument to S1::new() that triggers the error. I'm able to work around the issue by using a wrapper function with no arguments that constructs S1 and then another function that takes arguments and modifies S1.

[DutchGhost]

I got the exact same error with this code:

#![feature(nll)]

fn main() {
    let f: &'static () = &loop {break};
}

without nll, it fails to compile:

error[E0597]: borrowed value does not live long enough
 --> src/main.rs:4:27
  |
4 |     let f: &'static () = &loop {break};
  |                           ^^^^^^^^^^^^ temporary value does not live long enough
5 | }
  | - temporary value only lives until here
  |
  = note: borrowed value must be valid for the static lifetime...

error: aborting due to previous error

[robsmith11]

So is what I wrote not supposed to be allowed (pointing a mutable static reference to a mutable static thread_local value)?

After using the function wrapper workaround, it seems to work fine and allows me to use a thread_local value all over my crate without having to use unsafe every time.

[nikomatsakis]

We do have some special rules around #[thread_local] variables, yes, and I guess they haven't been ported to the MIR borrow checker. I'm not sure how that ought to work anyway. But I think this doesn't make the cut for an Edition Preview blocker -- going to put down as a release candidate blocker.

[matthewjasper]

The examples in this thread now compile, but it's not clear that they should.

For thead local statics are treated as going out of scope when the function ends by MIR borrowck to prevent creating a 'static reference to them. However static muts are unsafe to access, so no error is reported here.

For this example

fn main() {
    let f: &'static () = &loop {break};
}

In mir this looks no different to this

fn main() {
    let f: &'static (); {
        loop {break};
        f = &() // () is a constant expression, so can promote to static
    }
}

which should compile, but it's not clear whether we should guarantee that the first example is equivalent to this one.

[nikomatsakis]

hmm, for the original example at least, the AST-based borrow checker still forbids it -- presumably beacuse the #[thread_local] restrictions apply regardless of whether the variable is static or static mut.

[nikomatsakis]

However static muts are unsafe to access, so no error is reported here.

I investigated briefly changing this, because I feel like I would rather have an error here than not. It's a bit tricky though. We currently ignore borrows of unsafe places -- I removed that, but then we also consider paths rooted in "static mut" values to not conflict with one another. I can remove that but I think it will lead to errors being reported (e.g., if you borrow a static mut more than once and those borrows overlap).

Now, admittedly, those errors may indicate UB, so maybe that's ok.

In fact, in the original borrow checker, I think we ignored borrows of all statics. This would actually be a nice extension to #53177. But in the new one, if we do that, we'll not be able to enforce the "borrows of thread locals should not outlive current fn" rules, I don't think. Huh. Annoying.

[pnkfelix]

Nominating for discussion. This is a potentially subtle change to our static semantic, and I don't know if we can just push it under the umbrella of "oh well that's unsafe anyway" or even "oh well that's UB anyway"

[pnkfelix]

Discussed at WG-nll meeting. Setting milestone to "RC2" for us to make a decision (and implement it if necessary).

[pnkfelix]

(also @nikomatsakis openly mused about adding the T-lang label to this and popping it up to discussiom there... just making a note...)

[nikomatsakis]

@eddyb pointed out that #[thread_local] is unstable. So, I think that for the purposes of NLL, we should keep the current behavior, but add comprehensive tests around static, static mut, and #[thread_local] applied to both of them. Then we should file a follow-up issue around what rules (if any) to put on #[thread_local] when applied to static mut

[nikomatsakis]

I filed #54366 and added to the #[thread_local] tracking issue

[nikomatsakis]

@spastorino added a regression test in #54367, I think we can close this for now (with final resolution of the thread-local question deferred)