Skip to content

Fix overly restrictive lifetime in core::panic::Location::file return type #132087

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ijchen
Copy link
Contributor

@ijchen ijchen commented Oct 24, 2024

Fixes #131770 by relaxing the lifetime to match what's stored in the struct. See that issue for more details and discussion.

Since this is a breaking change, I think a crater run is in order. Since this change should only have an effect at compile-time, I think just a check run is sufficient.

@rustbot
Copy link
Collaborator

rustbot commented Oct 24, 2024

r? @cuviper

rustbot has assigned @cuviper.
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 Oct 24, 2024
Copy link
Contributor

@SpriteOvO SpriteOvO left a comment

Choose a reason for hiding this comment

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

Maybe adding a regression test in library/core/tests/panic/location.rs would be nice.

@ijchen
Copy link
Contributor Author

ijchen commented Oct 24, 2024

Maybe adding a regression test in library/core/tests/panic/location.rs would be nice.

That's a great idea! I'll implement that.

UPDATE: Implemented. It's a bit of a strange test, since it fails through a compiler error instead of a runtime panic, but I think that's necessary considering lifetimes and the borrow checker only exists at compile time.

@cuviper
Copy link
Member

cuviper commented Oct 24, 2024

Hmm, this is "overly restrictive" in the current implementation, but sometimes that's on purpose to give implementation freedom. For instance, we couldn't use Cow<'a, str> internally after your change. (ignoring core/alloc for a moment)

The breaking change of the function type is unlikely to be a problem IMO, but the API team should review it overall.

@rustbot label -T-libs +T-libs-api
r? libs-api

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 24, 2024
@rustbot rustbot assigned m-ou-se and unassigned cuviper Oct 24, 2024
@bors
Copy link
Collaborator

bors commented Jan 27, 2025

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

@Dylan-DPC Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2025
@Dylan-DPC
Copy link
Member

@ijchen any updates on this? this requires resolving conflicts in order to move ahead and if you are ready for a review you can click on »Ready for review« so it changes it from a draft.

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-CI Area: Our Github Actions CI A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-meta Area: Issues & PRs about the rust-lang/rust repository itself F-autodiff `#![feature(autodiff)]` T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Aug 2, 2025
@ijchen ijchen force-pushed the issue-131770-fix branch from 0ff917b to 8a0438f Compare August 2, 2025 21:04
@ijchen
Copy link
Contributor Author

ijchen commented Aug 2, 2025

Thanks for the poke! I've resolved merge conflicts, although it looks like the source code has changed a bit since I left off. There's some unsafe that I want to investigate more, so I'm going to hold off marking this ready for a bit.

@rust-log-analyzer

This comment has been minimized.

@ijchen ijchen marked this pull request as ready for review August 2, 2025 22:07
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 2, 2025
@m-ou-se m-ou-se added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-attributes Area: Attributes (`#[…]`, `#![…]`) T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-CI Area: Our Github Actions CI F-autodiff `#![feature(autodiff)]` labels Aug 5, 2025
@Amanieu
Copy link
Member

Amanieu commented Aug 12, 2025

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 12, 2025

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 12, 2025
@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Aug 12, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Aug 12, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overly restrictive lifetime in std::panic::Location::file
10 participants