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

panic_str only exists for the migration to 2021 panic macros #123050

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 25, 2024

The only caller is expect_failed, which is already a cold inline(never) function, so inlining into that function should be fine. (And indeed panic_str was #[inline] anyway.)

The existence of panic_str risks someone calling it when they should call panic instead, and I can't see a reason why this footgun should exist.

I also extended the comment in panic to explain why it needs a 'static string -- I know I've wondered about this in the past and it took me quite a while to understand.

@rustbot
Copy link
Collaborator

rustbot commented Mar 25, 2024

r? @scottmcm

rustbot has assigned @scottmcm.
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 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 Mar 25, 2024
@RalfJung
Copy link
Member Author

r? @m-ou-se

@rustbot rustbot assigned m-ou-se and unassigned scottmcm Mar 25, 2024
@m-ou-se
Copy link
Member

m-ou-se commented Mar 25, 2024

panic_str is also used here:

// Use `panic_str` instead of `panic_display::<&str>` for non_fmt_panic lint.
($msg:expr $(,)?) => ({
$crate::panicking::panic_str($msg);
}),

(See also the diagram in #116005)

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

RalfJung commented Mar 25, 2024 via email

@RalfJung
Copy link
Member Author

It seems like that lint does have type information available via cx.typeck_results().expr_ty. So instead of checking for panic_str it could check for panic_display with an &&str argument -- or would that not give the right results?

@bors
Copy link
Contributor

bors commented Mar 26, 2024

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

@RalfJung
Copy link
Member Author

It seems like that lint does have type information available via cx.typeck_results().expr_ty. So instead of checking for panic_str it could check for panic_display with an &&str argument -- or would that not give the right results?

I guess that would not be right since the lint should kick in for println!(mystrvar) but not println!("{}", mystrvar).

@RalfJung RalfJung changed the title remove panic_str, inline into the only caller panic_str only exists for the migration to 2021 panic macros Mar 26, 2024
@RalfJung
Copy link
Member Author

@rustbot ready

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

@m-ou-se I have updated the PR based on your comment. Do you want to take another look or should I try to find a different reviewer?

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

Lgtm! r=me when ready.

@RalfJung
Copy link
Member Author

This is ready, I just rebased to fix a typo in the commit message. :)

@bors r=m-ou-se rollup

@bors
Copy link
Contributor

bors commented Apr 23, 2024

📌 Commit 132921e has been approved by m-ou-se

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#123050 (panic_str only exists for the migration to 2021 panic macros)
 - rust-lang#124067 (weak lang items are not allowed to be #[track_caller])
 - rust-lang#124099 (Disallow ambiguous attributes on expressions)
 - rust-lang#124284 (parser: remove unused(no reads) max_angle_bracket_count field)
 - rust-lang#124288 (remove `push_trait_bound_inner`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 72e8fb4 into rust-lang:master Apr 23, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
Rollup merge of rust-lang#123050 - RalfJung:panic_str, r=m-ou-se

panic_str only exists for the migration to 2021 panic macros

The only caller is `expect_failed`, which is already a cold inline(never) function, so inlining into that function should be fine. (And indeed `panic_str` was `#[inline]` anyway.)

The existence of panic_str risks someone calling it when they should call `panic` instead, and I can't see a reason why this footgun should exist.

I also extended the comment in `panic` to explain why it needs a `'static` string -- I know I've wondered about this in the past and it took me quite a while to understand.
@RalfJung RalfJung deleted the panic_str branch April 23, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants