Support #[track_caller] on closures and generators

[Aaron1011]

Lang team summary

This PR adds support for placing the #[track_caller] attribute on closure and generator expressions. This attribute's addition behaves identically (from a users perspective) to the attribute being placed on the method in impl Fn/FnOnce/FnMut for ... generated by compiler.

The attribute is currently "double" feature gated -- both stmt_expr_attributes (preexisting) and closure_track_caller (newly added) must be enabled in order to place these attributes on closures.

As the Fn* traits lack a #[track_caller] attribute in their definition, caller information does not propagate when invoking closures through dyn Fn*. There is no limitation that this PR adds in supporting this; it can be added in the future.

Implementation details

This is implemented in the same way as for functions - an extra
location argument is appended to the end of the ABI. For closures,
this argument is not part of the 'tupled' argument storing the
parameters - the final closure argument for #[track_caller] closures
is no longer a tuple.

For direct (monomorphized) calls, the necessary support was already
implemented - we just needeed to adjust some assertions around checking
the ABI and argument count to take closures into account.

For calls through a trait object, more work was needed.
When creating a ReifyShim, we need to create a shim
for the trait method (e.g. FnOnce::call_mut) - unlike normal
functions, closures are never invoked directly, and always go through a
trait method.

Additional handling was needed for InstanceDef::ClosureOnceShim. In
order to pass location information throgh a direct (monomorphized) call
to FnOnce::call_once on an FnMut closure, we need to make
ClosureOnceShim aware of #[tracked_caller]. A new field
track_caller is added to ClosureOnceShim - this is used by
InstanceDef::requires_caller location, allowing codegen to
pass through the extra location argument.

Since ClosureOnceShim.track_caller is only used by codegen,
we end up generating two identical MIR shims - one for
track_caller == true, and one for track_caller == false. However,
these two shims are used by the entire crate (i.e. it's two shims total,
not two shims per unique closure), so this shouldn't a big deal.

[rust-highfive]

r? @estebank

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

[Aaron1011]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit 9ef105d74208a3bd339678a75595d9b9b4144aee with merge 0dc3a7eeb0512b0600339922ece622801ae932f0...

[bors]

☀️ Try build successful - checks-actions
Build commit: 0dc3a7eeb0512b0600339922ece622801ae932f0 (0dc3a7eeb0512b0600339922ece622801ae932f0)

[rust-timer]

Queued 0dc3a7eeb0512b0600339922ece622801ae932f0 with parent ca99e3e, future comparison URL.

[rust-timer]

Finished benchmarking try commit (0dc3a7eeb0512b0600339922ece622801ae932f0): comparison url.

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

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.

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

[estebank]

The code changes look reasonable.

[Mark-Simulacrum]

The language team discussed this in the last meeting, and had several thoughts:

A separate feature gate to avoid coupling this would be good -- in particular, while stmt_expr_attributes is unlikely to stabilize, coupling these two seems like a footgun for ourselves in the future.

There was some discussion about the implications this has for future stabilization of the Fn traits (Fn, FnMut, FnOnce). I think the primary concern is that making sure that the behavior for closures matches placing #[track_caller] on the (compiler-generated) trait implementation for the compiler-generated type of the closure. If that is the case here (at least from a language perspective) then the feeling was that this could likely be accepted.

@Aaron1011 Can you adjust or clarify for these two points? I think it could be useful to provide a lang-team oriented summary of the PR, rather than one oriented towards compiler team reviewers -- currently there are some details in the PR description that reference (hopefully) internal constructs to the compiler.

(Please re-nominate once these are clarified).

[Aaron1011]

A separate feature gate to avoid coupling this would be good -- in particular, while stmt_expr_attributes is unlikely to stabilize, coupling these two seems like a footgun for ourselves in the future.

I've added a feature gate closure_track_caller

I think the primary concern is that making sure that the behavior for closures matches placing #[track_caller] on the (compiler-generated) trait implementation for the compiler-generated type of the closure.

That's correct. The Fn/FnOnce/FnMut traits do not have #[track_caller, so calls through a trait object will not propagate caller location information. However, direct (monomorphized) calls behave as if you had written impl FnOnce for MyClosure { #[track_caller] fn call_once() }

[bors]

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

[Mark-Simulacrum]

Thanks @Aaron1011!

This was discussed in the last lang team meeting and there was general sentiment of moving ahead, but we wanted to see the PR description updated (e.g., it looks like the feature gate being added is not there yet). It might also be helpful to provide a concise description for T-lang FCP. I've tried to do so in this comment -- maybe you can take that as a base and uplift it into the PR description. The main bit is that I've tried to stay out of compiler internal impl details with this summary, which (AFAICT) do not affect the language surface area.

Once the description is updated it's likely we can kickoff fcp here.

---

This PR adds support for placing the #[track_caller] attribute on closure and generator expressions. This attribute's addition behaves identically (from a users perspective) to the attribute being placed on the method in impl Fn/FnOnce/FnMut for ... generated by compiler.

The attribute is currently "double" feature gated -- both stmt_expr_attributes (preexisting) and closure_track_caller (newly added) must be enabled in order to place these attributes on closures.

As the Fn* traits lack a track_caller attribute in their definition, caller information does not propagate when invoking closures through dyn Fn*. There is no limitation that this PR adds in supporting this; it can be added in the future.

[Aaron1011]

@estebank Are there any other changes that you'd like me to make?

[bors]

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

[estebank]

@bors r+

[bors]

📌 Commit 94b19fa has been approved by estebank

[bors]

⌛ Testing commit 94b19fa with merge 0132f82...

[bors]

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 0132f82 to master...

[rust-highfive]

📣 Toolstate changed by #87064!

Tested on commit 0132f82.
Direct link to PR: #87064

💔 rls on linux: test-pass → test-fail (cc @Xanewok).

[rust-timer]

Finished benchmarking commit (0132f82): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

• Large improvement in instruction counts (up to -1.8% on incr-unchanged builds of webrender-wrench)
• Small regression in instruction counts (up to 0.5% on incr-unchanged builds of helloworld)

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

[rylev]

The large majority of performance regressions were quite small and many were historically noisy benchmarks. Looking at the non-noisy benchmarks nothing seemed to stand out as a clear culprit. There were some test cases that indicated large jumps in incr_comp_persist_dep_graph but this did not hold true across all incremental test cases, and nothing in the changes seemed to jump out at me as potentially causing this issue.

@estebank @Aaron1011 anything to add here or should we mark as triaged?

[estebank]

From my end, nothing stands out as interesting.