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

Merged
merged 5 commits into from
Aug 23, 2025

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.

@m-ou-se m-ou-se removed the F-autodiff `#![feature(autodiff)]` label 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. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 22, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Aug 22, 2025

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

@dtolnay
Copy link
Member

dtolnay commented Aug 22, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 22, 2025

📌 Commit 792ec3b has been approved by dtolnay

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 22, 2025
@dtolnay dtolnay assigned dtolnay and unassigned m-ou-se Aug 22, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Aug 22, 2025
Fix overly restrictive lifetime in `core::panic::Location::file` return type

Fixes rust-lang#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.
bors added a commit that referenced this pull request Aug 23, 2025
Rollup of 20 pull requests

Successful merges:

 - #132087 (Fix overly restrictive lifetime in `core::panic::Location::file` return type)
 - #137396 (Recover `param: Ty = EXPR`)
 - #142185 (Convert moves of references to copies in ReferencePropagation)
 - #144443 (Make target pointer width in target json an integer)
 - #144648 (Implementation: `#[feature(nonpoison_rwlock)]`)
 - #144897 (print raw lifetime idents with r#)
 - #145218 ([Debuginfo] improve enum value formatting in LLDB for better readability)
 - #145380 (Add codegen-llvm regression tests)
 - #145573 (Add an experimental unsafe(force_target_feature) attribute.)
 - #145597 (resolve: Remove `ScopeSet::Late`)
 - #145641 (On E0277, point at type that doesn't implement bound)
 - #145669 (rustdoc-search: GUI tests check for `//` in URL)
 - #145695 (Introduce ProjectionElem::try_map.)
 - #145710 (Fix the ABI parameter inconsistency issue in debug.rs for LoongArch64)
 - #145726 (Experiment: Reborrow trait)
 - #145731 (Make raw pointers work in type-based search)
 - #145736 (triagebot: Update style team reviewers)
 - #145738 (Uplift rustc_mir_transform::coverage::counters::union_find to rustc_data_structures.)
 - #145743 (doc: fix some typos in comment)
 - #145745 (tests: Ignore basic-stepping.rs on LoongArch)

Failed merges:

 - #145670 (port `sanitize` attribute to the new parsing infrastructure)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Aug 23, 2025
Rollup of 28 pull requests

Successful merges:

 - #132087 (Fix overly restrictive lifetime in `core::panic::Location::file` return type)
 - #137396 (Recover `param: Ty = EXPR`)
 - #137457 (Fix host code appearing in Wasm binaries)
 - #142185 (Convert moves of references to copies in ReferencePropagation)
 - #144648 (Implementation: `#[feature(nonpoison_rwlock)]`)
 - #144897 (print raw lifetime idents with r#)
 - #145218 ([Debuginfo] improve enum value formatting in LLDB for better readability)
 - #145380 (Add codegen-llvm regression tests)
 - #145573 (Add an experimental unsafe(force_target_feature) attribute.)
 - #145597 (resolve: Remove `ScopeSet::Late`)
 - #145633 (Fix some typos in LocalKey documentation)
 - #145641 (On E0277, point at type that doesn't implement bound)
 - #145669 (rustdoc-search: GUI tests check for `//` in URL)
 - #145695 (Introduce ProjectionElem::try_map.)
 - #145710 (Fix the ABI parameter inconsistency issue in debug.rs for LoongArch64)
 - #145726 (Experiment: Reborrow trait)
 - #145731 (Make raw pointers work in type-based search)
 - #145736 (triagebot: Update style team reviewers)
 - #145738 (Uplift rustc_mir_transform::coverage::counters::union_find to rustc_data_structures.)
 - #145742 (rustdoc js: Even more typechecking improvments )
 - #145743 (doc: fix some typos in comment)
 - #145745 (tests: Ignore basic-stepping.rs on LoongArch)
 - #145747 (Refactor lint buffering to avoid requiring a giant enum)
 - #145751 (fix(lexer): Allow '-' in the frontmatter infostring continue set)
 - #145761 (Add aarch64_be-unknown-hermit target)
 - #145762 (convert strings to symbols in attr diagnostics)
 - #145763 (Ship LLVM tools for the correct target when cross-compiling)
 - #145765 (Revert suggestions for missing methods in tuples)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit dbc38ee into rust-lang:master Aug 23, 2025
10 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Aug 23, 2025
rust-timer added a commit that referenced this pull request Aug 23, 2025
Rollup merge of #132087 - ijchen:issue-131770-fix, r=dtolnay

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

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.
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. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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