support pub(restricted) in thread_local! #40984

durka · 2017-04-01T03:09:50Z

pub(restricted) was stabilized in #40556 so let's go!

I changed the interface of __thread_local_inner!, which is supposedly unstable but this is not checked for macros (#34097 cc @petrochenkov @jseyfried), so this may be an issue.

rust-highfive · 2017-04-01T03:10:01Z

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

durka · 2017-04-01T03:10:51Z

src/libstd/thread/local.rs

+    // rule 4: handle multiple restricted public declarations
+    ($(#[$attr:meta])* pub $vis:tt static $name:ident: $t:ty = $init:expr) => (
+        __thread_local_inner!($(#[$attr])* [pub $vis] $name, $t, $init);
+        thread_local!($($rest)*);


Please note these six rules can be reduced to two if we revive the $x:vis RFC.

durka · 2017-04-01T19:21:44Z

Github ate my comment, so just to reiterate: this PR is real, but also meant to demonstrate why we need a :vis macro matcher.

alexcrichton · 2017-04-12T22:11:25Z

Discussed at the libs triage meeting the other day, our conclusion was that we should probably wait for #41012 to get merged, which looks like it's going to get merged.

carols10cents · 2017-04-14T17:42:52Z

Labeling waiting on review since rereview might be needed once #41012 is in.

arielb1 · 2017-04-18T08:41:44Z

#41012 has landed. @alexcrichton can you look at this?

durka · 2017-04-18T16:12:26Z

I was planning to update or resubmit after updating the macro to use :vis.

…

On Apr 18, 2017 4:42 AM, "Ariel Ben-Yehuda" ***@***.***> wrote: #41012 <#41012> has landed. @alexcrichton <https://github.com/alexcrichton> can you look at this? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#40984 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n4YJClLg37qyNqr0KCeBy6RaDEk0ks5rxHdZgaJpZM4MwWIZ> .

alexcrichton · 2017-04-19T01:19:25Z

Ok thanks @durka!

durka · 2017-04-19T02:34:31Z

Actually we can't use :vis until #41012 hits beta, right?

sfackler · 2017-04-19T06:58:04Z

Seems like you can do it before then with #[cfg(stage0)] and #[cfg(not(stage0))] versions of the macro, right?

durka · 2017-04-21T17:55:20Z

Hmm, this is odd:

[00:17:04] Building stage1 compiler artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
... snip ...
[00:17:20]    Compiling syntax_pos v0.0.0 (file:///checkout/src/libsyntax_pos)
[00:17:20] error: :vis fragment specifier is experimental and subject to change (see issue #41022)
[00:17:20]  --> <thread_local macros>:2:29
[00:17:20]   |
[00:17:20] 2 | $ ( # [ $ attr : meta ] ) * $ vis : vis static $ name : ident : $ t : ty = $
[00:17:20]   |                             ^^^^^^^^^^^
[00:17:20]   |
[00:17:20]   = help: add #![feature(macro_vis_matcher)] to the crate attributes to enable
[00:17:20] 
[00:17:20] error: :vis fragment specifier is experimental and subject to change (see issue #41022)
[00:17:20]  --> <thread_local macros>:6:29
[00:17:20]   |
[00:17:20] 6 | $ ( # [ $ attr : meta ] ) * $ vis : vis static $ name : ident : $ t : ty = $
[00:17:20]   |                             ^^^^^^^^^^^
[00:17:20]   |
[00:17:20]   = help: add #![feature(macro_vis_matcher)] to the crate attributes to enable

But I did add that attribute. Does it have to be added to crates that invoke the macro as well? That would be a problem. And it should be fine because thread_local! and __thread_local_inner! are marked #[allow_internal_unstable]. Perhaps fragment specifiers are not covered by that?

alexcrichton · 2017-04-21T20:29:09Z

As the author of #41012, you're probably in the best position to answer that question :)

durka · 2017-04-21T22:54:46Z

Good call :)

Can a commit fixing that go in this PR or should I separate it?

jseyfried · 2017-04-24T19:22:37Z

I've reached out to @jseyfried to see if this is fixable or not

I believe this is fixable, but I'm not sure exactly how -- I'll look into it when I get the time (probably a week or two).

durka · 2017-04-25T20:09:54Z

OK, I did come up with a way to make it shorter, by having the macro munch attributes one at a time until it gets to the visibility specifier:

macro_rules! thread_local {
    // terminate recursion
    (@ [] -> $_x:tt) => {};
    
    // munch one attribute
    (@ [#[$attr:meta] $($rest:tt)*] -> [$(#[$attrs:meta])*]) => {
        thread_local!(@ [$($rest)*] -> [$(#[$attrs])* #[$attr]]);
    };
    
    // finish a static with no trailing semicolon (therefore there are no more)
    (@ [$vis:vis static $name:ident: $typ:ty = $init:expr] -> [$(#[$attrs:meta])*]) => {
        __thread_local_inner!(($(#[$attrs])*) $vis, $name, $typ, $init);
    };
    
    // finish a static with trailing semicolon and continue to parse more
    (@ [$vis:vis static $name:ident: $typ:ty = $init:expr; $($rest:tt)*] -> [$(#[$attrs:meta])*]) => {
        __thread_local_inner!(($(#[$attrs])*) $vis, $name, $typ, $init);
        thread_local!(@ [$($rest)*] -> []);
    };
    
    // public entry point
    ($($t:tt)*) => {
        thread_local!(@ [$($t)*] -> []);
    }
}

This business with the @ "private" rules could be pulled into a separate macro as well (e.g. __thread_local_collector!). If only that trailing semicolon was required, we could remove one more rule, oh well.

alexcrichton · 2017-04-26T14:32:15Z

Yeah if we do add a macro implementation like that we'd want it to be an internal macro hidden away, but it seems reasonableto have such an implementation.

alexcrichton · 2017-05-04T18:25:02Z

@durka to clarify, did you intend on landing that instantiation in this PR? Or waiting for a fix to the macros from @jseyfried?

durka · 2017-05-04T19:09:27Z

Ah sorry this dropped off my radar. I'm happy to land either the version with extra arms (so not actually using :vis in thread_local!, though it would be used in thread_local_inner!) or the muncher version in this PR. The muncher version wouldn't look great in rustdoc (since it would just be $($t:tt)*) but we can solve that with docs. Do you care which way we go?

…

On Thu, May 4, 2017 at 2:25 PM, Alex Crichton ***@***.***> wrote: @durka <https://github.com/durka> to clarify, did you intend on landing that instantiation in this PR? Or waiting for a fix to the macros from @jseyfried <https://github.com/jseyfried>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#40984 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n9sPs29up4xTFcSk_a0qxNDldhYLks5r2hgUgaJpZM4MwWIZ> .

alexcrichton · 2017-05-04T21:31:20Z

I'd ideally prefer to wait for :vis bugs to get fixed (or at least see if they're not fixable) but I also don't have a burning need for this change just yet. If there's some strong motivation I could see us landing this sooner rather than later.

carols10cents · 2017-05-08T13:51:39Z

I'd ideally prefer to wait for :vis bugs to get fixed (or at least see if they're not fixable)

Are there issues/PRs for the :vis bugs that are blocking this PR that we could link to for status tracking purposes?

durka · 2017-05-08T19:27:00Z

I think it is just #26444 (closed WONTFIX) unfortunately. But I could be wrong.

alexcrichton · 2017-05-08T21:22:31Z

This was to a large degree the motivation for #41012, right? If so, it seems a shame if it's actually still difficult to leverage :(

Mark-Simulacrum · 2017-05-14T15:38:00Z

@durka Could you give an update here? Looks like Travis is still failing -- @jseyfried, did you have a chance to take a look?

durka · 2017-05-14T20:49:08Z

Status quo.

bors · 2017-05-16T23:29:00Z

☔ The latest upstream changes (presumably #42038) made this pull request unmergeable. Please resolve the merge conflicts.

durka · 2017-05-17T00:08:39Z

I'm going to close this for now and post in the :vis tracking issue. I'll resubmit once we decide on an approach, or if someone complains about thread_local! specifically.

Fixes rust-lang#24189. Fixes rust-lang#26444. Fixes rust-lang#27832. Fixes rust-lang#34030. Fixes rust-lang#35650. Fixes rust-lang#39964. Fixes the 4th comment in rust-lang#40569. Fixes the issue blocking rust-lang#40984.

…seyfried Only match a fragment specifier the if it starts with certain tokens. When trying to match a fragment specifier, we first predict whether the current token can be matched at all. If it cannot be matched, don't bother to push the Earley item to `bb_eis`. This can fix a lot of issues which otherwise requires full backtracking (#42838). In this PR the prediction treatment is not done for `:item`, `:stmt` and `:tt`, but it could be expanded in the future. Fixes #24189. Fixes #26444. Fixes #27832. Fixes #34030. Fixes #35650. Fixes #39964. Fixes the 4th comment in #40569. Fixes the issue blocking #40984.

durka · 2017-07-12T16:11:22Z

Rebased and reopened now that #42913 got merged.

Mark-Simulacrum · 2017-07-12T16:14:44Z

Looks like you'll need to open a new pull request -- GH is being annoying and not letting this one be reopened.

durka · 2017-07-12T16:15:13Z

Resubmitted as #43185.

@petrochenkov

support pub(restricted) in thread_local! (round 2) Resurrected #40984 now that the issue blocking it was fixed. Original description: `pub(restricted)` was stabilized in #40556 so let's go! Here is a [playground](https://play.rust-lang.org/?gist=f55f32f164a6ed18c219fec8f8293b98&version=nightly&backtrace=1). I changed the interface of `__thread_local_inner!`, which is supposedly unstable but this is not checked for macros (#34097 cc @petrochenkov @jseyfried), so this may be an issue.

rust-highfive assigned brson Apr 1, 2017

durka commented Apr 1, 2017

View reviewed changes

durka force-pushed the thread-local-pub-restricted branch 5 times, most recently from 6622a28 to 20a8799 Compare April 1, 2017 15:42

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 11, 2017

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2017

alexcrichton added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 19, 2017

durka and others added 2 commits April 19, 2017 02:29

support pub(restricted) in thread_local!

7b349d2

use :vis in thread_local!

5fde1e2

durka force-pushed the thread-local-pub-restricted branch from 20a8799 to 8fffee0 Compare April 21, 2017 04:30

stage0 fallback

62e69cd

durka force-pushed the thread-local-pub-restricted branch from 8fffee0 to 62e69cd Compare April 21, 2017 05:30

durka force-pushed the thread-local-pub-restricted branch 2 times, most recently from 068b960 to 6174d01 Compare April 22, 2017 02:46

durka closed this May 17, 2017

durka mentioned this pull request May 17, 2017

Tracking issue for :vis macro matcher #41022

Closed

kennytm mentioned this pull request Jun 26, 2017

Only match a fragment specifier if it starts with certain tokens. #42913

Merged

durka mentioned this pull request Jul 12, 2017

support pub(restricted) in thread_local! (round 2) #43185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support pub(restricted) in thread_local! #40984

support pub(restricted) in thread_local! #40984

durka commented Apr 1, 2017 •

edited

Loading

rust-highfive commented Apr 1, 2017

durka Apr 1, 2017 •

edited

Loading

durka commented Apr 1, 2017

alexcrichton commented Apr 12, 2017

carols10cents commented Apr 14, 2017

arielb1 commented Apr 18, 2017

durka commented Apr 18, 2017 via email

alexcrichton commented Apr 19, 2017

durka commented Apr 19, 2017

sfackler commented Apr 19, 2017

durka commented Apr 21, 2017

alexcrichton commented Apr 21, 2017

durka commented Apr 21, 2017

jseyfried commented Apr 24, 2017 •

edited

Loading

durka commented Apr 25, 2017 •

edited

Loading

alexcrichton commented Apr 26, 2017

alexcrichton commented May 4, 2017

durka commented May 4, 2017 via email

alexcrichton commented May 4, 2017

carols10cents commented May 8, 2017

durka commented May 8, 2017

alexcrichton commented May 8, 2017

Mark-Simulacrum commented May 14, 2017

durka commented May 14, 2017

bors commented May 16, 2017

durka commented May 17, 2017

durka commented Jul 12, 2017

Mark-Simulacrum commented Jul 12, 2017

durka commented Jul 12, 2017

support pub(restricted) in thread_local! #40984

support pub(restricted) in thread_local! #40984

Conversation

durka commented Apr 1, 2017 • edited Loading

rust-highfive commented Apr 1, 2017

durka Apr 1, 2017 • edited Loading

Choose a reason for hiding this comment

durka commented Apr 1, 2017

alexcrichton commented Apr 12, 2017

carols10cents commented Apr 14, 2017

arielb1 commented Apr 18, 2017

durka commented Apr 18, 2017 via email

alexcrichton commented Apr 19, 2017

durka commented Apr 19, 2017

sfackler commented Apr 19, 2017

durka commented Apr 21, 2017

alexcrichton commented Apr 21, 2017

durka commented Apr 21, 2017

jseyfried commented Apr 24, 2017 • edited Loading

durka commented Apr 25, 2017 • edited Loading

alexcrichton commented Apr 26, 2017

alexcrichton commented May 4, 2017

durka commented May 4, 2017 via email

alexcrichton commented May 4, 2017

carols10cents commented May 8, 2017

durka commented May 8, 2017

alexcrichton commented May 8, 2017

Mark-Simulacrum commented May 14, 2017

durka commented May 14, 2017

bors commented May 16, 2017

durka commented May 17, 2017

durka commented Jul 12, 2017

Mark-Simulacrum commented Jul 12, 2017

durka commented Jul 12, 2017

durka commented Apr 1, 2017 •

edited

Loading

durka Apr 1, 2017 •

edited

Loading

jseyfried commented Apr 24, 2017 •

edited

Loading

durka commented Apr 25, 2017 •

edited

Loading