New Lint: [`thread_local_initializer_can_be_made_const`] #12026

m-rph · 2023-12-27T14:58:48Z

Adds a new lint to suggest using const on thread_local! initializers that can be evaluated at compile time.

Impl details:

The lint relies on the expansion of thread_local!. For non const-labelled initializers, thread_local! produces a function called __init that lazily initializes the value. We check the function and decide whether the body can be const. If so, we lint that the initializer value can be made const.

changelog: new lint [thread_local_initializer_can_be_made_const]

fixes: #12015

rustbot · 2023-12-27T14:58:53Z

r? @llogiq

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

llogiq · 2023-12-27T15:09:55Z

I wonder whether we should take mutability into account – some thread locals might be there to be mutated & making those const leads to compile errors.

At least we should have a test against this, also if the lint catches those, we should move it to nursery until the false positive no longer appears.

m-rph · 2023-12-27T15:12:33Z

Edit: I will add some more tests and accommodate mutability 🤔

m-rph · 2023-12-27T16:13:50Z

@llogiq I checked, and it's impossible to have a mut initializer.

    |
33  |         static mut BUF_5: i32 = 0;
    |                    ^^^^^ no rules expected this token in macro call
    |

because the patterns are:

($(#[$attr:meta])* $vis:vis static $name:ident: $t:ty = const { $init:expr }; $($rest:tt)*) => 
($(#[$attr:meta])* $vis:vis static $name:ident: $t:ty = const { $init:expr }) =>
($(#[$attr:meta])* $vis:vis static $name:ident: $t:ty = $init:expr; $($rest:tt)*) =>
($(#[$attr:meta])* $vis:vis static $name:ident: $t:ty = $init:expr) =>

joboet · 2023-12-28T12:08:03Z

I love the idea, but is there any way that the implementation could avoid depending on internal std details? There is some work currently being done on thread locals (see rust-lang/rust#110897 and e.g. rust-lang/rust#116123), and I really want to avoid accidentally breaking clippy when changing the expansion.

m-rph · 2023-12-28T13:16:41Z

I am uncertain.

This has to be done as a late-lint so that we can check whether it can be const evaluated.
Then we have to find the initialiser to check it. As long as the initializer via the __init remains the same, it should be okay.

Perhaps if we find a way to go back to pre-expanded code, and then do a kind of pattern match?

The issue is that we must know what to lint, in a thread_local! with multiple non-const assignments, finding the right one to lint doesn't seem particularly obvious. The other thing is that thread_local_inner! doesn't have a symbol / isn't public, so we can't use the call-site of that to figure out which snippet to lint.

I am out of my element element I think. If anyone has any suggestions i'd be happy to give it a shot.

m-rph · 2023-12-28T17:13:50Z

clippy_lints/src/thread_local_initializer_can_be_made_const.rs

+            // this is the `__init` function emitted by the `thread_local!` macro
+            // when the `const` keyword is not used. We avoid checking the `__init` directly
+            // as that is not a public API.
+            // we know that the function is const-qualifiable, so now we need only to get the
+            // initializer expression to span-lint it.
+            && let ExprKind::Block(block, _) = body.value.kind
+            && let Some(ret_expr) = block.expr
+            && let initializer_snippet = snippet(cx, ret_expr.span, "thread_local! { ... }")
+            && initializer_snippet != "thread_local! { ... }"


@joboet I have reduced the dependency on internal details to "just" knowing that the last expression in the initializer is the original snippet and that an initializer function exists. Name doesn't matter anymore.

I have also reviewed the storage pr you listed, and it looks fine from the perspective of this, but having a comment there indicating that the initializer is checked would most likely avoid breaking it buuut I really don't want to tie this to particular impl.

bors · 2023-12-29T11:10:28Z

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

rustbot · 2023-12-29T12:32:42Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

a74fa7a

rustbot · 2023-12-29T12:43:22Z

Error: Label has-merge-conflicts can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

llogiq

There is a final tiny nit pick, otherwise this looks good to merge.

clippy_lints/src/thread_local_initializer_can_be_made_const.rs

Adds a new lint to suggest using `const` on `thread_local!` initializers that can be evaluated at compile time. Impl details: The lint relies on the expansion of `thread_local!`. For non const-labelled initializers, `thread_local!` produces a function called `__init` that lazily initializes the value. We check the function and decide whether the body can be const. The body of the function is exactly the initializer. If so, we lint the body. changelog: new lint [`thread_local_initializer_can_be_made_const`]

llogiq · 2024-01-02T00:26:17Z

Thank you. I think the dependency on the thread_local implementation is lean enough to accept for now, perhaps we'll revisit it (like we did with format_args a while ago) at some point.

@bors r+

bors · 2024-01-02T00:26:20Z

📌 Commit 70024e1 has been approved by llogiq

It is now in the queue for this repository.

bors · 2024-01-02T00:27:26Z

⌛ Testing commit 70024e1 with merge e73bb00...

bors · 2024-01-02T00:36:46Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing e73bb00 to master...

petrochenkov · 2024-01-21T22:14:25Z

@partiallytyped
This lint doesn't work correctly on all targets.
On some targets fn __init is emitted even if the const { ... } block is used, so this lint is always reported.
The affected targets are windows-gnu, and all others for which the implementation of thread_local! from mod os_local; is used.

petrochenkov · 2024-01-22T13:35:13Z

(I noticed this because full rust-lang/rust test suite, which includes clippy, fails on windows-gnu now.)

m-rph · 2024-01-22T13:36:52Z

I have opened a new PR for that #12186

rustbot assigned llogiq Dec 27, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 27, 2023

m-rph marked this pull request as ready for review December 27, 2023 16:14

m-rph commented Dec 28, 2023

View reviewed changes

m-rph mentioned this pull request Dec 28, 2023

iter_without_into_iter fires on code generated by macros from other crates #12037

Closed

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Dec 29, 2023

rustbot removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Dec 29, 2023

llogiq approved these changes Jan 1, 2024

View reviewed changes

clippy_lints/src/thread_local_initializer_can_be_made_const.rs Outdated Show resolved Hide resolved

bors merged commit e73bb00 into rust-lang:master Jan 2, 2024
8 checks passed

m-rph deleted the 12015 branch January 2, 2024 06:55

m-rph restored the 12015 branch January 21, 2024 22:34

taiki-e mentioned this pull request Mar 3, 2024

thread_local_initializer_can_be_made_const should respect MSRV #12404

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Lint: [`thread_local_initializer_can_be_made_const`] #12026

New Lint: [`thread_local_initializer_can_be_made_const`] #12026

m-rph commented Dec 27, 2023 •

edited

Loading

rustbot commented Dec 27, 2023

llogiq commented Dec 27, 2023 •

edited

Loading

m-rph commented Dec 27, 2023 •

edited

Loading

m-rph commented Dec 27, 2023 •

edited

Loading

joboet commented Dec 28, 2023

m-rph commented Dec 28, 2023 •

edited

Loading

m-rph Dec 28, 2023 •

edited

Loading

bors commented Dec 29, 2023

rustbot commented Dec 29, 2023

rustbot commented Dec 29, 2023

llogiq left a comment

llogiq commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

petrochenkov commented Jan 21, 2024

petrochenkov commented Jan 22, 2024

m-rph commented Jan 22, 2024

New Lint: [thread_local_initializer_can_be_made_const] #12026

New Lint: [thread_local_initializer_can_be_made_const] #12026

Conversation

m-rph commented Dec 27, 2023 • edited Loading

rustbot commented Dec 27, 2023

llogiq commented Dec 27, 2023 • edited Loading

m-rph commented Dec 27, 2023 • edited Loading

m-rph commented Dec 27, 2023 • edited Loading

joboet commented Dec 28, 2023

m-rph commented Dec 28, 2023 • edited Loading

m-rph Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

bors commented Dec 29, 2023

rustbot commented Dec 29, 2023

rustbot commented Dec 29, 2023

llogiq left a comment

Choose a reason for hiding this comment

llogiq commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

bors commented Jan 2, 2024

petrochenkov commented Jan 21, 2024

petrochenkov commented Jan 22, 2024

m-rph commented Jan 22, 2024

New Lint: [`thread_local_initializer_can_be_made_const`] #12026

New Lint: [`thread_local_initializer_can_be_made_const`] #12026

m-rph commented Dec 27, 2023 •

edited

Loading

llogiq commented Dec 27, 2023 •

edited

Loading

m-rph commented Dec 27, 2023 •

edited

Loading

m-rph commented Dec 27, 2023 •

edited

Loading

m-rph commented Dec 28, 2023 •

edited

Loading

m-rph Dec 28, 2023 •

edited

Loading