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

[ijchen]

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

[rust-rfcbot]

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

[rust-rfcbot]

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.

[workingjubilee]

So, I realize this is a bit late (sorry), but if we ever get the ability to remove the Location data to another file (debuginfo or similar, assuming a common separate-file-debuginfo paradigm), and then reconstruct the Location data on-the-spot, wouldn't acquiring a &'static Location become difficult and require additional allocations?

[ijchen]

if we ever get the ability to remove the Location data to another file [...] wouldn't acquiring a &'static Location become difficult and require additional allocations?

That's a great point, thanks for raising it. I do agree that promising a &'static Location<'static>s makes that significantly harder - but I also think we've already made this commitment with Location::caller. It's been stable in core since 1.46, and returns a &'static Location<'static>. My opinion is that this change doesn't make the situation any worse, besides perhaps increasing the usefulness (and therefore likely the usage) of a potentially inflexible API. Interested to hear what people think about this.

[the8472]

AIUI the idea is that panics might switch to not use Location::caller or similar and instead rely on the stacktrace machinery to compute the location at runtime, with varying quality depending on available debug info.
And panic sites are a lot more common than explicit uses of Location::caller, so that doesn't really make a good precedent.

[ijchen]

Ahh I see - I think this is worth further discussion, what's the process for that? Is it too late to "cancel" the merge so we have more time to consider?

[Amanieu]

I think that even in the case where location data is generated at runtime it can still be "leaked" to provide a static lifetime. This isn't a memory leak if we only do this once for each potential location.

[workingjubilee]

@Amanieu That is true in the sense that it will be at-worst linear in the number of Location calls (and bounded by the number of distinct Locations actually used), but people who run valgrind will report it as memory leaks and I expect it will be more difficult to add an exclusion for them, I expect, as compared to e.g. things like stdio buffers which will have consistent sources and consistent overhead. That is admittedly consistent with our policy but someone will have to answer for that policy. Can I refer those issues to you then?

[workingjubilee]

A better question, perhaps: would providing a (way to get a) Cow (or similar type) so we have the freedom to moove in either direction be an unacceptable solution?

[ijchen]

I like the idea of Cow as a solution, although these tradeoffs/cons come to mind:

• It's more restrictive (as an example, my library would not be able to make a struct Copy that otherwise could be, since it needs to be 'static and would now need to (possibly) hold a String)
• Cow isn't in core, so this would no longer help PanicInfo
• Adding it as an alternative to PanicHookInfo::location feels overlapping in an unfortunate way, but (AFAIK) we can't add it as an (inherent) method on Location itself because Location is in core and Cow is not.

That all said, I do think I'd rather force users to deal with a Cow than find ourselves in a future where significant amounts of Rust code is bending over backwards or doing the "wrong" thing just to support this relatively niche use case. To me, it's a question of whether or not we're actually going to want runtime-determined panic locations, and if so, how problematic it would actually be to leak Locations for binaries which don't embed them at compile time. I don't know the answer to either of those.