Skip to content

Rename begin_panic to panic_with_payload #144902

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rename begin_panic to panic_with_payload #144902

wants to merge 1 commit into from
+63 −55

Conversation

Kivooeo
Copy link
Contributor

@Kivooeo Kivooeo commented Aug 4, 2025

This was little bit discussed here #t-libs > `begin_panic` renaming @ 💬, so I guess this is fine name

Also as far as I can say it's safe to rename because of lang item, correct me here if I'm wrong

Part of #116005

@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

This PR modifies tests/ui/issues/. If this PR is adding new tests to tests/ui/issues/,
please refrain from doing so, and instead add it to more descriptive subdirectories.

Some changes occurred to constck

cc @fee1-dead

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Kivooeo Kivooeo force-pushed the more-creative-name-for-begin-panic branch from 1fc546b to 8d38e92 Compare August 4, 2025 14:16
@rust-log-analyzer

This comment has been minimized.

Sign in to view
bjorn3
bjorn3 reviewed Aug 4, 2025
View reviewed changes
compiler/rustc_const_eval/src/check_consts/check.rs
// types other than str. Check to enforce that only str can be used in
// const-eval.

// const-eval of the `begin_panic` fn assumes the argument is `&str`
// const-eval of the `panic_with_payload` fn assumes the argument is `&str`
if tcx.is_lang_item(callee, LangItem::BeginPanic) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the lang item should be renamed too to match the new function name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does it needs FCP?

Copy link
Member

Choose a reason for hiding this comment

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

No, that is not necessary. Lang items are purely internal implementation details. They change all the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Could you take a look and resolve it if so

@Kivooeo Kivooeo force-pushed the more-creative-name-for-begin-panic branch from 8d38e92 to 5d10eb5 Compare August 4, 2025 16:39
@rustbot rustbot added the A-rustc-dev-guide Area: rustc-dev-guide label Aug 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2025

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol, @tshepang

@Kivooeo
Copy link
Contributor Author

Kivooeo commented Aug 4, 2025

According to message above would it be reasonable to move rust-analyzer changes into it's repo? or it's fine like this

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Kivooeo Kivooeo force-pushed the more-creative-name-for-begin-panic branch from 5d10eb5 to 98d2ad4 Compare August 4, 2025 16:46
@Kivooeo Kivooeo force-pushed the more-creative-name-for-begin-panic branch from 98d2ad4 to 22e6d2b Compare August 4, 2025 16:49
@rustbot rustbot added the T-release Relevant to the release subteam, which will review and decide on the PR/issue. label Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-rustc-dev-guide Area: rustc-dev-guide S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Kivooeo @rustbot @rust-log-analyzer @bjorn3 @Mark-Simulacrum