Change Location<'_> lifetime to 'static in Panic[Hook]Info

[ijchen]

View all comments

I'm writing a library that would benefit from being able to store a &'static str instead of a String for the file location of a panic. Currently, the API of std::panic::PanicHookInfo::location and core::panic::PanicInfo::location return a Location<'_> (and by extension, a file name &str) whose lifetime is tied to the borrow of the Panic[Hook]Info.

It is my understanding that the Location/&str will always be Location<'static>/&'static str, since the file name is embedded directly into the compiled binary (and I don't see a likely reason/way for that to ever change in the future). Since it seems unlikely to ever need to change, and since making the guarantee that the returned lifetime is 'static has a real benefit (allows users to avoid unnecessary allocations), I think changing to the returned Location<'_>'s lifetime to 'static would be a worthwhile change.

see also the recent #1320870, which made a similar change to std::panic::Location

[rustbot]

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

[Amanieu]

Considering that the only way to construct a location is Location::caller which returns a &'static Location<'static> (this is used internally by the panic macros and compiler-generated panics), we could make these methods return a &'static Location<'static> as well. I don't see any value in a non-'static version of this unless we plan to allow constructing custom locations (which seems unlikely).

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[ijchen]

That's a great idea! I've updated accordingly.

[Amanieu]

@rfcbot merge

[rust-rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @the8472

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]

Restarting FCP to reflect updated team membership.

@rfcbot cancel
@rfcbot merge

[rust-rfcbot]

@Amanieu proposal cancelled.

[rust-rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @the8472

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.

[ijchen]

Friendly ping for checkboxes here, in case this slipped under the radar: @BurntSushi @joshtriplett @the8472

[hanna-kruppe]

I found this PR after also writing some code that could've made good use of getting a &'static Location in a panic hook. It would be great to get this un-stuck somehow. Some thoughts:

If there's appetite for having an accessor that returns a Cow or morally similar enum (e.g., None | &'a Location | &'static Location), this would have to be a new method. Changing the return type of the existing location() method would be very breaking. In that case, location() could be redefined to mean "the &'static one, if it exists" (it already reserves the right to return None). This isn't great since existing code that uses location() may sometimes miss out on location info that's available (just not 'static), and would have to be updated to a different API. But in principle, with some renames and deprecations, it's may not end up too different from e.g. payload() vs payload_as_str().

More fundamentally: it's not clear to me why "no &'static Location but std can create one dynamically" is something that's useful enough to block useful improvements to Panic[Hook]Info. I get the use case for opting out of the static Location data per panic location, that could be a measurable binary size benefit. Most projects probably won't want the trade-off w.r.t. ability to debug crashes remotely, but it's not bad to have the option. (Though I'll note that so far nobody was motivated to implement this opt-out.) That's no problem for this PR, as location() already reserves the right to return None. The question is just, are there use cases where you'd want to strip Locations from the binary and still can and want to get some panic location information?

If you want that binary size benefit so badly, you're not going to ship debug info with the binary, you'll use split debug info, not ship the debug info with the binary, and do symbolication of crashes out-of-process (often on a different machine entirely). You may not even want to pay the cost of embedding symbolication code in the binary (cc #139209). So it seems unlikely that a binary built with the "panic Location opt-out" flag will be able to actually recover any locations. I only see two cases where the debug info would nevertheless be available, but neither seems convincing to me:

• The developers are debugging a local build of the binary and still have the split debug info lying around on disk for std to find it. But in this scenario they can also (re-)build in a configuration that's better for debugging and includes static Locations.
• You got an optimized binary and then separately fetch debug info for it from a symbol server. This is useful for debuggers and profilers, but generally done out-of-process. It seems very unlikely to me that Rust's standard library would want to include all the complexity to fetch debug info this way, especially in the use cases where binary size is important enough that dropping Locations is interesting.

[Amanieu]

This was discussed in the @rust-lang/libs-api meeting. We stand by our previous decision to stabilize this change, essentially agreeing with @hanna-kruppe's assessment.

@bors r+

[rust-bors]

📋 This PR cannot be approved because it currently has the following label: S-waiting-on-fcp.

[ijchen]

I believe this is now just waiting on code review, correct?

[ijchen]

pinging @Mark-Simulacrum as the current assignee - let me know if this is waiting on me for something, I'm new to the process.

[Mark-Simulacrum]

@bors r+

[rust-bors]

📌 Commit e3b8df1 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[rust-bors]

💔 Test for d77ab4f failed: CI. Failed job:

• auto - dist-ohos-x86_64 (web logs, enhanced plaintext logs)

[JonathanBrouwer]

@bors retry

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)