Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow any const expression blocks in thread_local! #116392

Closed
wants to merge 1 commit into from

Conversation

nvzqz
Copy link
Contributor

@nvzqz nvzqz commented Oct 3, 2023

Previously only a single expression could be allowed within the const block. This change allows for statements to precede the final expression, similarly to real inline const expressions.

This works by taking $init as a block in thread_local! and passing it to thread_local_inner!, where it is accepted as an expr.

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2023

r? @Mark-Simulacrum

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 3, 2023
@nvzqz
Copy link
Contributor Author

nvzqz commented Oct 3, 2023

@rustbot label +T-libs-api +A-thread-locals +A-macros -T-libs

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-thread-locals Area: Thread local storage (TLS) and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 3, 2023
@nvzqz nvzqz changed the title Allow any expression blocks in thread_local! Allow any const expression blocks in thread_local! Oct 3, 2023
@Mark-Simulacrum
Copy link
Member

r? @m-ou-se for T-libs-api FCP since this is a user-facing stable API change.

@rustbot rustbot assigned m-ou-se and unassigned Mark-Simulacrum Oct 7, 2023
@Mark-Simulacrum Mark-Simulacrum added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Oct 7, 2023
Comment on lines +155 to +158
/// pub static FOO: Cell<u32> = const {
/// let value = 1;
/// Cell::new(value)
/// };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to have a test that checks this works, but I don't think we should make the documentation example more verbose/complicated than necessary.

@m-ou-se
Copy link
Member

m-ou-se commented Nov 28, 2023

@rfcbot merge

Summary:

// Allowed today:
thread_local! {
    pub static A: Cell<u32> = const { Cell::new(value) };
}

// Disallowed today, but allowed after this PR:
thread_local! {
    pub static B: Cell<u32> = const {
        let value = 1;
        Cell::new(value)
    };
}

@rfcbot
Copy link

rfcbot commented Nov 28, 2023

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 28, 2023
@rfcbot
Copy link

rfcbot commented Nov 28, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 28, 2023
@dtolnay
Copy link
Member

dtolnay commented Nov 29, 2023

LGTM.

I hope this won't have an unexpected interaction with const_closures (#106003). But this is clearly worth doing anyway.

#![allow(incomplete_features)]
#![feature(const_closures)]
#![feature(type_alias_impl_trait)]

type Closure = impl Fn();

thread_local! {
    pub static X: Closure = const || {};
}

@rfcbot reviewed

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 8, 2023
@rfcbot
Copy link

rfcbot commented Dec 8, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@dtolnay
Copy link
Member

dtolnay commented Dec 8, 2023

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 8, 2023
@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 8, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 14, 2023
@dtolnay dtolnay assigned dtolnay and unassigned m-ou-se Jan 12, 2024
@dtolnay
Copy link
Member

dtolnay commented Jan 12, 2024

@nvzqz this is waiting on #116392 (review) to be addressed.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
Allow any `const` expression blocks in `thread_local!`

This PR contains a rebase of the macro change from rust-lang#116392, together with adding a test under library/std/tests.

Testing this feature by making the documentation's example code needlessly more complicated was not appropriate as pointed out in rust-lang#116392 (review).

Without the macro change, this new test would fail to build as follows:

```console
error: no rules expected the token `let`
   --> library/std/tests/thread.rs:26:13
    |
26  |             let value = 1;
    |             ^^^ no rules expected this token in macro call
    |
note: while trying to match meta-variable `$init:expr`
   --> library/std/src/thread/local.rs:189:69
    |
189 |     ($(#[$attr:meta])* $vis:vis static $name:ident: $t:ty = const { $init:expr }; $($rest:tt)*) => (
    |                                                                     ^^^^^^^^^^
```

Closes rust-lang#116392.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
Allow any `const` expression blocks in `thread_local!`

This PR contains a rebase of the macro change from rust-lang#116392, together with adding a test under library/std/tests.

Testing this feature by making the documentation's example code needlessly more complicated was not appropriate as pointed out in rust-lang#116392 (review).

Without the macro change, this new test would fail to build as follows:

```console
error: no rules expected the token `let`
   --> library/std/tests/thread.rs:26:13
    |
26  |             let value = 1;
    |             ^^^ no rules expected this token in macro call
    |
note: while trying to match meta-variable `$init:expr`
   --> library/std/src/thread/local.rs:189:69
    |
189 |     ($(#[$attr:meta])* $vis:vis static $name:ident: $t:ty = const { $init:expr }; $($rest:tt)*) => (
    |                                                                     ^^^^^^^^^^
```

Closes rust-lang#116392.
@bors bors closed this in d3761de Jan 22, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
Rollup merge of rust-lang#120181 - dtolnay:tlconst, r=thomcc

Allow any `const` expression blocks in `thread_local!`

This PR contains a rebase of the macro change from rust-lang#116392, together with adding a test under library/std/tests.

Testing this feature by making the documentation's example code needlessly more complicated was not appropriate as pointed out in rust-lang#116392 (review).

Without the macro change, this new test would fail to build as follows:

```console
error: no rules expected the token `let`
   --> library/std/tests/thread.rs:26:13
    |
26  |             let value = 1;
    |             ^^^ no rules expected this token in macro call
    |
note: while trying to match meta-variable `$init:expr`
   --> library/std/src/thread/local.rs:189:69
    |
189 |     ($(#[$attr:meta])* $vis:vis static $name:ident: $t:ty = const { $init:expr }; $($rest:tt)*) => (
    |                                                                     ^^^^^^^^^^
```

Closes rust-lang#116392.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-thread-locals Area: Thread local storage (TLS) disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants