Readd track_caller to Result::from_residual

[yaahc]

This is a followup on #87401 in and an attempt to move the issue towards resolution.

As part of the overhaul of the Try trait we removed the ability for errors to grab location information during propagation via ? with the builtin std::result::Result. The previously linked issue has a fair bit of discussion into the reasons for and against the usage of #[track_caller] on the FromResidual impl on Result that I will do my best to summarize.

---

For

• try_trait_v2 changed the location of the From::from caller so the stack trace is different. #87401 (comment): Difficulties with using non std::result::Result like types
• try_trait_v2 changed the location of the From::from caller so the stack trace is different. #87401 (comment): Inconsistency with functionality provided for recoverable (Result) and non-recoverable errors (panic), where panic provides a location and Result does not, pushing some users towards using panic

Against

• Tracking Issue for try_trait_v2, A new design for the ? desugaring (RFC#3058) #84277 (comment): concern that this will bloat callers that never use this data

---

Personally, I want to quantify the performance / bloat impact of re-adding this attribute, and fully evaluate the pros and cons before deciding if I need to switch eyre to have a custom Result type, which would also mean I need try_trait_v2 to be stabilized, cc @scottmcm. If the performance impact is minor enough in the general case I think I would prefer that the default Result type has the ability to track location information for consistency with panic error reporting, and leave it to applications that need particularly high performance to handle the micro optimizations of introducing their own efficient custom Result type or matching manually.

Alternatively, I wonder if the performance penalty on code that doesn't use the location information on FromResidual could be mitigated via new optimizations.

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[cuviper]

Alternatively, I wonder if the performance penalty on code that doesn't use the location information on FromResidual could be mitigated via new optimizations.

If the calls are inlined, it's likely that the unused location information will already be optimized away.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 44756d8 with merge 8c48303ad50f06e7dd1cc5ba110053ede6d55199...

[cuviper]

Looks like it might need some UI output review / blessing.

[scottmcm]

My big question here is whether it'd result in a de-facto stability guarantee, despite there not de-jure being one per #88302 (comment).

For example, without this we could merge the "re-yeet" MIR blocks from multiple ?s, but if they need to preserve all the line numbers that becomes very different.

[yaahc]

My big question here is whether it'd result in a de-facto stability guarantee, despite there not de-jure being one per #88302 (comment).

For example, without this we could merge the "re-yeet" MIR blocks from multiple ?s, but if they need to preserve all the line numbers that becomes very different.

No matter what I think we've already made the explicit decision that we do not consider #[track_caller] location data to be part of our stable API, so if some future consideration comes along that pushes us in the other direction we can always revert this change.

[bors]

☀️ Try build successful - checks-actions
Build commit: 8c48303ad50f06e7dd1cc5ba110053ede6d55199 (8c48303ad50f06e7dd1cc5ba110053ede6d55199)

[rust-timer]

Queued 8c48303ad50f06e7dd1cc5ba110053ede6d55199 with parent 0b42dea, future comparison URL.

[rust-timer]

Finished benchmarking commit (8c48303ad50f06e7dd1cc5ba110053ede6d55199): comparison url.

Summary: This change led to moderate relevant regressions 😿 in compiler performance.

• Moderate regression in instruction counts (up to 1.4% on incr-unchanged builds of wg-grammar)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[m-ou-se]

r? @cuviper

[cuviper]

The only negative perf results are in incremental builds, and even those are not too bad. I think it's acceptable.

@bors r+

[bors]

📌 Commit 94307e2 has been approved by cuviper

[bors]

⌛ Testing commit 94307e2 with merge df89fd2...

[bors]

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing df89fd2 to master...

[rust-timer]

Finished benchmarking commit (df89fd2): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression