Stream: t-compiler/wg-nll

Topic: #52663-thread-local-statics


davidtwco (Aug 06 2018 at 16:48, on Zulip):

I'd be interested in tackling this small diagnostic improvement. Would this be: tracking down where this error is reported, checking (somehow) if the value is thread local and must outlive static and then special casing based on that with a improved message that mentions that the static is thread local?

davidtwco (Aug 06 2018 at 16:48, on Zulip):

@pnkfelix or @nikomatsakis

nikomatsakis (Aug 06 2018 at 16:53, on Zulip):

hmm

nikomatsakis (Aug 06 2018 at 16:55, on Zulip):

probably.. I can't quite tell where that error comes from

nikomatsakis (Aug 06 2018 at 16:56, on Zulip):

I would probably start by running with RUST_BACKTRACE=1 and -Ztreat-err-as-bug

nikomatsakis (Aug 06 2018 at 16:56, on Zulip):

so you can get the backtrace where the error is reported

nikomatsakis (Aug 06 2018 at 16:57, on Zulip):

oh, I wonder if we want to modify the buffer method on diagnostics to respect that flag

davidtwco (Aug 06 2018 at 16:58, on Zulip):

@nikomatsakis https://github.com/rust-lang/rust/blob/9f9ac89d11a2afaeaa029738b38142124684c89b/src/librustc_mir/borrow_check/error_reporting.rs#L440 is where this is reported.

davidtwco (Aug 06 2018 at 16:59, on Zulip):

Though, the addition of buffered errors does make -Z treat-err-as-bug much less useful.

nikomatsakis (Aug 06 2018 at 17:00, on Zulip):

I think the caller report_borrowed_value_does_not_live_long_enough

nikomatsakis (Aug 06 2018 at 17:00, on Zulip):

maybe wants to be modified then

nikomatsakis (Aug 06 2018 at 17:01, on Zulip):

ultimately I think this is being invoked from check_invalidation_at_exit

nikomatsakis (Aug 06 2018 at 17:01, on Zulip):

i.e., https://github.com/rust-lang/rust/blob/9f9ac89d11a2afaeaa029738b38142124684c89b/src/librustc_mir/borrow_check/mod.rs#L1491-L1497

nikomatsakis (Aug 06 2018 at 17:01, on Zulip):

so you could thread some flag down

nikomatsakis (Aug 06 2018 at 17:01, on Zulip):

but I guess if we just check for Place::Static(statik) where is_thread_local is true

nikomatsakis (Aug 06 2018 at 17:01, on Zulip):

in the "report borrowed value" routine

nikomatsakis (Aug 06 2018 at 17:01, on Zulip):

we can handle it there

nikomatsakis (Aug 06 2018 at 17:02, on Zulip):

er, well, maybe in report_local_value_does_not_live_long_enough

nikomatsakis (Aug 06 2018 at 17:02, on Zulip):

anyway, somewhere :)

davidtwco (Aug 06 2018 at 17:48, on Zulip):

To save some time in the review, what would the ideal error for this be? Same as before but with a note saying (something along the lines of) "are you trying to reference this thread-local static from another thread?" or a completely different error message? This is the current message.

nikomatsakis (Aug 06 2018 at 18:18, on Zulip):

@davidtwco huh. It seems like both the AST and MIR-based messages are confusing

nikomatsakis (Aug 06 2018 at 18:18, on Zulip):

I would think something like:

nikomatsakis (Aug 06 2018 at 18:19, on Zulip):
error[E0597]: thread-local variable borrowed past end of function
  --> $DIR/issue-17954.rs:20:14
   |
LL |     let a = &FOO;
   |              ^^^ thread-local variables cannot be borrowed beyond the end of the function
...
LL | }
   | - end of enclosing function is here
   |
   = note: `FOO` is borrowed for the static lifetime

error: aborting due to previous error

For more information about this error, try `rustc --explain E0597`.
nikomatsakis (Aug 06 2018 at 18:19, on Zulip):

it might want its own error code, too

davidtwco (Aug 06 2018 at 18:20, on Zulip):

:thumbs_up:

davidtwco (Aug 06 2018 at 20:55, on Zulip):

Submitted #53131.

davidtwco (Aug 06 2018 at 21:15, on Zulip):

Updated the PR again after tidy complained about duplicate error codes - couldn't quite reproduce that locally but incremented the number anyway.

davidtwco (Aug 06 2018 at 21:15, on Zulip):

(locally it said 710 before a rebase that I did prior to making the PR, after the rebase it still said 710 but Travis complained that 711 was taken, so I've gone to 712)

davidtwco (Aug 06 2018 at 22:27, on Zulip):

Had to update the PR again after forgetting to update a test when the error code changed.

davidtwco (Aug 07 2018 at 08:27, on Zulip):

Updated again because updating tests clearly isn't my forte.

nikomatsakis (Aug 07 2018 at 09:55, on Zulip):

@davidtwco

[01:19:06] ---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0712 (line 11385) stdout ----
[01:19:06] error[E0597]: borrowed value does not live long enough
[01:19:06]   --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:11392:14
[01:19:06]    |
[01:19:06] 8  |     let a = &FOO; // error: thread-local variable borrowed past end of function
[01:19:06]    |              ^^^ temporary value does not live long enough
[01:19:06] 13 | }
[01:19:06] 13 | }
[01:19:06]    | - temporary value only lives until here
[01:19:06]    |
[01:19:06]    = note: borrowed value must be valid for the static lifetime...
nikomatsakis (Aug 07 2018 at 09:55, on Zulip):

I hate those error-index failures...

davidtwco (Aug 07 2018 at 09:56, on Zulip):

@nikomatsakis Already on it. The error code it expects is right, but only if #![feature(nll)] is provided.

nikomatsakis (Aug 07 2018 at 09:56, on Zulip):

argh

davidtwco (Aug 07 2018 at 09:56, on Zulip):

I wasn't sure we wanted that in the error index.

nikomatsakis (Aug 07 2018 at 09:56, on Zulip):

I think you can add a #![feature(nll)] to the example...

nikomatsakis (Aug 07 2018 at 09:56, on Zulip):

I feel like we've done that elsewhere

davidtwco (Aug 07 2018 at 09:56, on Zulip):

Wasn't sure if that was alright to do.

nikomatsakis (Aug 07 2018 at 09:56, on Zulip):

e.g.

E0198: r##"
A negative implementation is one that excludes a type from implementing a
particular trait. Not being able to use a trait is always a safe operation,
so negative implementations are always safe and never need to be marked as
unsafe.

```compile_fail
#![feature(optin_builtin_traits)]

struct Foo;

// unsafe is unnecessary
unsafe impl !Clone for Foo { }
davidtwco (Aug 07 2018 at 10:00, on Zulip):

Do you know what the command for testing that locally is?

davidtwco (Aug 07 2018 at 10:00, on Zulip):

I am trying src/tool/rustdoc just now.

davidtwco (Aug 07 2018 at 10:00, on Zulip):

But not sure that tests the error index.

davidtwco (Aug 07 2018 at 10:00, on Zulip):

src/tool/error-index-generator doesn't.

nikomatsakis (Aug 07 2018 at 10:03, on Zulip):

I...don't know

nikomatsakis (Aug 07 2018 at 10:03, on Zulip):

I think at this point I usually give up and run ./x.py test

davidtwco (Aug 07 2018 at 10:04, on Zulip):

I hope that I'll find the correct path quick enough that I'll have run the error index tests in less time than it would take to just run everything.

davidtwco (Aug 07 2018 at 10:06, on Zulip):

Huh, seems like it is src/tools/error-index-generator but locally it isn't doing anything.

davidtwco (Aug 07 2018 at 10:08, on Zulip):

Ah, I just had to set verbose-tests in config.toml to see it doing it.

davidtwco (Aug 07 2018 at 10:09, on Zulip):

@nikomatsakis Anyway, updated that PR again now - it _should_ be fine, but I've said that three times already..

nikomatsakis (Aug 07 2018 at 10:09, on Zulip):

the "long tail" :)

nikomatsakis (Aug 07 2018 at 10:09, on Zulip):

/me been there

Last update: Nov 21 2019 at 14:50UTC