Skip to content

Threadlocal_initializer_can_be_made_const will not trigger for unreachable initializers #12685

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

Merged
merged 1 commit into from
Apr 20, 2024
Merged

Conversation

PartiallyUntyped
Copy link
Contributor

This commit introduces a check to ensure that the lint won't trigger when the initializer is unreachable, such as:

thread_local! {
    static STATE: Cell<usize> = panic!();
}

This is achieved by looking at the unpeeled initializer expression and ensuring that the parent macro is not panic!(), todo!(), unreachable!(), unimplemented!().

fixes #12637

changelog: [threadlocal_initializer_can_be_made_const] will no longer trigger on unreachable macros.

@rustbot
Copy link
Collaborator

rustbot commented Apr 17, 2024

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 17, 2024
@PartiallyUntyped PartiallyUntyped assigned blyxyas and unassigned llogiq Apr 17, 2024
@blyxyas
Copy link
Member

blyxyas commented Apr 19, 2024

I've made a new patch that does a few things:
Get the diagnostic item name as a variable and extrapolate that is_panic to be able to reuse the diag name for the todo, unreachable, unimplemented.

…hable initializers

This commit introduces a check to ensure that the lint won't trigger when the initializer is
unreachable, such as:

```
thread_local! {
    static STATE: Cell<usize> = panic!();
}
```

This is achieved by looking at the unpeeled initializer expression and ensuring that the parent
macro is not `panic!()`, `todo!()`, `unreachable!()`, `unimplemented!()`.

fixes #12637

changelog: [`threadlocal_initializer_can_be_made_const`] will no longer trigger on `unreachable` macros.
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! ❤️
I've done a little change (optimization + symbols directly)

@blyxyas
Copy link
Member

blyxyas commented Apr 19, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 19, 2024

📌 Commit 206b1a1 has been approved by blyxyas

It is now in the queue for this repository.

@PartiallyUntyped
Copy link
Contributor Author

PartiallyUntyped commented Apr 19, 2024

This pattern seems to happen pretty often.
Do you think we could move this to utils and perhaps return an Option<UnreachableKind>?

@bors
Copy link
Contributor

bors commented Apr 19, 2024

⌛ Testing commit 206b1a1 with merge 33edb19...

bors added a commit that referenced this pull request Apr 19, 2024
Threadlocal_initializer_can_be_made_const will not trigger for unreachable initializers

This commit introduces a check to ensure that the lint won't trigger when the initializer is unreachable, such as:

```
thread_local! {
    static STATE: Cell<usize> = panic!();
}
```

This is achieved by looking at the unpeeled initializer expression and ensuring that the parent macro is not `panic!()`, `todo!()`, `unreachable!()`, `unimplemented!()`.

fixes #12637

changelog: [`threadlocal_initializer_can_be_made_const`] will no longer trigger on `unreachable` macros.
@blyxyas
Copy link
Member

blyxyas commented Apr 19, 2024

@bors r-

@blyxyas
Copy link
Member

blyxyas commented Apr 19, 2024

Hmmmm... lemme see where else is this pattern used (meow)

@bors
Copy link
Contributor

bors commented Apr 19, 2024

☀️ Try build successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Build commit: 33edb19 (33edb1920b0684d418b8e35da9315f6ca870fbb4)

@blyxyas
Copy link
Member

blyxyas commented Apr 19, 2024

Where else is some kind of function like this used? I've tried to Ctrl+Shift+F but there doesn't seem to exist almost anywhere else (and be extrapolatable)

@PartiallyUntyped
Copy link
Contributor Author

PartiallyUntyped commented Apr 20, 2024

Off the top of my head; unused_io_amount and panic_unimplemented.

In panic_unimplemented knowledge of which macro it is helps. In unused_io_amount it isn't.

@blyxyas
Copy link
Member

blyxyas commented Apr 20, 2024

Hmmm... If there are at least two others and you're feeling extra-helpful today, yeah it would help! ❤️
But that should be another PR (PRs should be as small as possible). You can assign me to it when you open it 🐱 (if you're planning to do it)

@blyxyas
Copy link
Member

blyxyas commented Apr 20, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 20, 2024

📌 Commit 206b1a1 has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 20, 2024

⌛ Testing commit 206b1a1 with merge c2a98cb...

@bors
Copy link
Contributor

bors commented Apr 20, 2024

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

@bors bors merged commit c2a98cb into rust-lang:master Apr 20, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

thread_local_initializer_can_be_made_const false positive: panicking Cell and RefCell initialization
5 participants