Fix FP in `threadlocal!` when falling back to `os_local` #12276

m-rph · 2024-02-11T17:07:28Z

Issue: The lint always triggers for some targets.
Why: The lint assumes an __init function is not present for const initializers.
This does not hold for all targets. Some targets always have an initializer function.
Fix: If an initializer function exists, we check that it is not a const fn.

Lint overview before the patch:

Outside threadlocal!, then exit early.
In threadlocal! and __init does not exist => is const already, exit early.
In threadlocal! and __init exists and __init body can be const => we lint.

Lint overview after the patch:

Outside threadlocal!, then exit early.
In threadlocal! and __init does not exist => is const already, exit early.
In threadlocal! and __init exists and is const fn => is const already, exit early.
In threadlocal! and __init exists and is not const fn and __init body can be const => we lint.

The patch adds step 3.

changelog: Fixed fp in [thread_local_initializer_can_be_made_const] where fallback implementation of threadlocal! was always linted.

Verifying the changes

For each of these platforms [ mac, x86_64-windows-gnu, x86_64-windows-msvc ]:

switched to master, ran bless. Found a failure for windows-gnu.
switched to patch, ran bless. Success for all three platforms.

How platform affects the lint:

The compiler performs a conditional compilation depending on platform features. The os_local fallback includes a call to __init, without step 3, we lint in all cases.

cfg_if::cfg_if! {
    if #[cfg(any(all(target_family = "wasm", not(target_feature = "atomics")), target_os = "uefi"))] {
        #[doc(hidden)]
        mod static_local;
        #[doc(hidden)]
        pub use static_local::{Key, thread_local_inner};
    } else if #[cfg(target_thread_local)] {
        #[doc(hidden)]
        mod fast_local;
        #[doc(hidden)]
        pub use fast_local::{Key, thread_local_inner};
    } else {
        #[doc(hidden)]
        mod os_local;
        #[doc(hidden)]
        pub use os_local::{Key, thread_local_inner};
    }
}

r? @llogiq

m-rph · 2024-02-12T19:58:29Z

@rustbot label +beta-nominated

I think this should be merged upstream as well.

blyxyas · 2024-02-13T19:50:38Z

Just tested this patch on an ARM 64-bit Linux, that's a target that the CI doesn't look for. Quinn has checked for Windows (which I'd think she'd be able to check for both MSVC and GNU), and nobody uses 32-bit Windows. I think that this patch is covered on the test department.

blyxyas

Just some mini-nits. Mainly regarding to consistency with the rest of the codebase. I'll benchmark this PR, I'm a little bit worried about the optimized-MIR call, but that may not have as big of an effect as we might think.

Sorry for being so thorough, you asked for high standards, and these are my highest reasonable standards 😅

clippy_lints/src/thread_local_initializer_can_be_made_const.rs

m-rph · 2024-02-13T20:38:50Z

Thank you Alejandra I appreciate your time and effort!

I found another case I hadn't considered initially, and will add that with the changes.

m-rph

Need more tests to check the case where we should peel.

clippy_lints/src/thread_local_initializer_can_be_made_const.rs

blyxyas · 2024-02-17T18:34:05Z

Okis, sorry for the delay. I finally have the benchmark results: There isn't really a big performance change

blyxyas

This should have a comment linking to this issue and some conditional compilation. If the issue only shows on a fallback, it's kinda silly to check this for non-fallback compilations.

Apart from that, LGTM

m-rph · 2024-02-29T23:18:48Z

If this is fine, maybe we can merge this to bundle it with the release?

blyxyas

LGTM, thanks! ❤️ Could you squash the commits?

`os_local` impl of `thread_local` — regardless of whether it is const and unlike other implementations — includes an `fn __init(): EXPR`. Existing implementation of the lint checked for the presence of said function and whether the expr can be made const. Because for `os_local` we always have an `__init()`, it triggers for const implementations. The solution is to check whether the `__init()` function is already const. If it is `const`, there is nothing to do. Otherwise, we verify that we can make it const. Co-authored-by: Alejandra González <blyxyas@gmail.com>

m-rph · 2024-02-29T23:55:44Z

Done, thanks! ☺️

blyxyas · 2024-03-01T00:02:45Z

@bors r+

bors · 2024-03-01T00:02:48Z

📌 Commit bb1ee87 has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-03-01T00:03:54Z

⌛ Testing commit bb1ee87 with merge 225d377...

bors · 2024-03-01T00:11:16Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 225d377 to master...

flip1995 · 2024-03-14T19:00:55Z

rust-lang/rust#122510

…ulacrum [beta] Clippy backports Backports included in this PR: - rust-lang/rust-clippy#12276 Fixing the lint on some platforms before hitting stable - rust-lang/rust-clippy#12405 Respect MSRV before hitting stable - rust-lang/rust-clippy#12266 Fixing an (unlikely) ICE - rust-lang/rust-clippy#12177 Fixing FPs before hitting stable Each backport on its own might not warrant a backport, but the collection of those are nice QoL fixes for Clippy users, before those bugs hit stable. All of those commits are already on `master`. r? `@Mark-Simulacrum`

rustbot assigned llogiq Feb 11, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 11, 2024

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 12, 2024

blyxyas requested changes Feb 13, 2024

View reviewed changes

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

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

m-rph commented Feb 13, 2024

View reviewed changes

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

m-rph requested a review from blyxyas February 19, 2024 20:48

blyxyas requested changes Feb 21, 2024

View reviewed changes

m-rph force-pushed the hide-thread-local branch from 6e8c237 to a6f170e Compare February 29, 2024 21:18

blyxyas approved these changes Feb 29, 2024

View reviewed changes

m-rph force-pushed the hide-thread-local branch from e8fb91f to bb1ee87 Compare February 29, 2024 23:42

bors merged commit 225d377 into rust-lang:master Mar 1, 2024
5 checks passed

flip1995 added beta-accepted Accepted for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Mar 14, 2024

flip1995 mentioned this pull request Mar 14, 2024

[beta] Clippy backports rust-lang/rust#122510

Merged

xFrednet removed the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix FP in `threadlocal!` when falling back to `os_local` #12276

Fix FP in `threadlocal!` when falling back to `os_local` #12276

m-rph commented Feb 11, 2024 •

edited

m-rph commented Feb 12, 2024 •

edited

blyxyas commented Feb 13, 2024

blyxyas left a comment

m-rph commented Feb 13, 2024

m-rph left a comment •

edited

blyxyas commented Feb 17, 2024

blyxyas left a comment

m-rph commented Feb 29, 2024 •

edited

blyxyas left a comment

m-rph commented Feb 29, 2024

blyxyas commented Mar 1, 2024

bors commented Mar 1, 2024

bors commented Mar 1, 2024

bors commented Mar 1, 2024

flip1995 commented Mar 14, 2024

Fix FP in threadlocal! when falling back to os_local #12276

Fix FP in threadlocal! when falling back to os_local #12276

Conversation

m-rph commented Feb 11, 2024 • edited

Verifying the changes

How platform affects the lint:

m-rph commented Feb 12, 2024 • edited

blyxyas commented Feb 13, 2024

blyxyas left a comment

Choose a reason for hiding this comment

m-rph commented Feb 13, 2024

m-rph left a comment • edited

Choose a reason for hiding this comment

blyxyas commented Feb 17, 2024

blyxyas left a comment

Choose a reason for hiding this comment

m-rph commented Feb 29, 2024 • edited

blyxyas left a comment

Choose a reason for hiding this comment

m-rph commented Feb 29, 2024

blyxyas commented Mar 1, 2024

bors commented Mar 1, 2024

bors commented Mar 1, 2024

bors commented Mar 1, 2024

flip1995 commented Mar 14, 2024

Fix FP in `threadlocal!` when falling back to `os_local` #12276

Fix FP in `threadlocal!` when falling back to `os_local` #12276

m-rph commented Feb 11, 2024 •

edited

m-rph commented Feb 12, 2024 •

edited

m-rph left a comment •

edited

m-rph commented Feb 29, 2024 •

edited