Support forwarding caller location through trait object method call #81360

Aaron1011 · 2021-01-24T23:03:54Z

Since PR #69251, the #[track_caller] attribute has been supported on
traits. However, it only has an effect on direct (monomorphized) method
calls. Calling a #[track_caller] method on a trait object will not
propagate caller location information - instead, Location::caller() will
return the location of the method definition.

This PR forwards caller location information when #[track_caller] is
present on the method definition in the trait. This is possible because
#[track_caller] in this position is 'inherited' by any impls of that
trait, so all implementations will have the same ABI.

This PR does not change the behavior in the case where
#[track_caller] is present only on the impl of a trait.
While all implementations of the method might have an explicit
#[track_caller], we cannot know this at codegen time, since other
crates may have impls of the trait. Therefore, we keep the current
behavior of not forwarding the caller location, ensuring that all
implementations of the trait will have the correct ABI.

See the modified test for examples of how this works

rust-highfive · 2021-01-24T23:03:57Z

r? @lcnr

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

Aaron1011 · 2021-01-24T23:04:09Z

cc @rust-lang/lang @anp

lcnr · 2021-01-25T00:52:35Z

compiler/rustc_middle/src/ty/instance.rs

+                            // (e.g. `trait Foo { #[track_caller] my_fn() { /* impl */ } }`),
+                            // then we don't need to generate a shim. This check is needed because
+                            // `should_inherit_track_caller` returns `false` if our method
+                            // implementation comes from the trait block, and not an impl block


Suggested change

// implementation comes from the trait block, and not an impl block

// implementation comes from the trait block, and not an impl block.

anp

LGTM, cc @eddyb who reviewed most of the original query code.

If this lands, make sure to update the reference's language too?

bors · 2021-02-02T14:58:36Z

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

cramertj · 2021-02-16T17:58:57Z

We discussed this in the most recent language team meeting and everyone was generally favorable to offering this behavior. However, we would like a write-up explaining the feature, its interaction with user-written ABI annotations, and the changes to the Rust reference. Additionally, several of us felt that the annotation should come with a warning in the case that the annotation is present on the trait but not on a specific implementation. Personally, I might prefer this to be an error, but I understand that there may be traits which would want to add this attribute after-the-fact in a backwards compatible way.

nikomatsakis · 2021-02-23T17:59:23Z

Removing nominated label. Please re-nominate when there's a write-up! Thanks.

crlf0710 · 2021-03-12T18:14:36Z

@Aaron1011 Ping from triage! would you want to write such a writeup, or do you want some help on this?

Aaron1011 · 2021-03-13T23:56:03Z

Personally, I might prefer this to be an error, but I understand that there may be traits which would want to add this attribute after-the-fact in a backwards compatible way.

I think being able to add this backwards-compatibly is very important - otherwise, adding this annotation to a trait would require updating every downstream crate.

eddyb · 2021-03-18T20:48:11Z

Personally, I might prefer this to be an error, but I understand that there may be traits which would want to add this attribute after-the-fact in a backwards compatible way.

We've already done this to e.g. Index::index, so at this point I believe the ship has sailed.

There's no reason to require the impls to opt into this as the trait forces #[track_caller] onto them.

eddyb · 2021-03-18T20:51:05Z

compiler/rustc_middle/src/ty/instance.rs

-            InstanceDef::Item(def) => {
-                tcx.codegen_fn_attrs(def.did).flags.contains(CodegenFnAttrFlags::TRACK_CALLER)
+            InstanceDef::Item(ty::WithOptConstParam { did: def_id, .. })
+            | InstanceDef::Virtual(def_id, _) => {
+                tcx.codegen_fn_attrs(def_id).flags.contains(CodegenFnAttrFlags::TRACK_CALLER)


Heh, nice, I like that this is all it takes for virtual calls to start taking panic::Location.
I assume the rest of the changes is there "just" to handle edge cases.

JohnCSimon · 2021-05-09T18:58:08Z

Ping from triage:
@Aaron1011 you have a merge conflict.

lcnr · 2021-05-24T18:19:41Z

I am not able to review any PRs in the near future.

r? @nagisa

nagisa · 2021-05-26T22:45:40Z

src/test/ui/rfc-2091-track-caller/tracked-trait-obj.rs

+    // We have `#[track_caller]` on the implementation of the method,
+    // but not on the definition of the method in the trait. Therefore,
+    // caller location information will *not* work through a method call
+    // on a trait object. Instead, we will get the location of this method
+    #[track_caller]
+    fn track_caller_not_on_trait_method(&self) {
+        let location = std::panic::Location::caller();
+        assert_eq!(location.file(), file!());
+        assert_eq!(location.line(), line!() - 3);
+        assert_eq!(location.column(), 5);
+    }


Is there any reason why the file!/line! don't fall-back to their usual behaviour of providing precisely the line/column they are on, rather than that of where the method definition item begins? As implemented right now, this seems like a 3rd new behaviour for these macros.

(Though with the behaviour as it is, I'm sure that the bikeshed could go on ad infinitum here)

This is the current behavior whenever we need to generate a shim:

trait Foo { #[track_caller] fn bar(&self); } impl Foo for () { fn bar(&self) { panic!("Where does this end up?") } } fn foo(val: &dyn Foo) { val.bar(); } fn main() { foo(&()); }

On the latest nightly, this prints thread 'main' panicked at 'Where does this end up?', src/main.rs:6:5

I agree that it would probably be good to change this, but I think that could be done separately.

rfcbot · 2021-06-25T17:10:46Z

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.

The RFC will be merged soon.

nagisa · 2021-07-10T12:42:42Z

We are okay with merging this, correct? I gave my consent to r=me this previously on the implementation side of things.

RalfJung · 2021-07-10T12:50:09Z

Yes, FCP passed.
@bors r=nagisa

Aaron1011 · 2021-07-10T13:41:40Z

@bors r=nagisa

Aaron1011 · 2021-07-10T13:42:47Z

@bors r=nagisa

bors · 2021-07-10T13:42:48Z

📌 Commit 6fd6624 has been approved by nagisa

bors · 2021-07-10T14:11:43Z

⌛ Testing commit 6fd6624 with merge 3982eb3...

bors · 2021-07-10T16:41:20Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 3982eb3 to master...

rust-highfive · 2021-07-10T16:41:43Z

📣 Toolstate changed by #81360!

Tested on commit 3982eb3.
Direct link to PR: #81360

💔 miri on windows: test-pass → test-fail (cc @RalfJung @eddyb @oli-obk).
💔 miri on linux: test-pass → test-fail (cc @RalfJung @eddyb @oli-obk).

@RalfJung

Tested on commit rust-lang/rust@3982eb3. Direct link to PR: <rust-lang/rust#81360> 💔 miri on windows: test-pass → test-fail (cc @RalfJung @eddyb @oli-obk). 💔 miri on linux: test-pass → test-fail (cc @RalfJung @eddyb @oli-obk).

Mark-Simulacrum · 2021-07-13T14:41:48Z

This change led to moderate performance regressions. It would probably be good to run perf on ~all changes adding a query -- it looks like that may have been the primary cause of the regressions in this PR (though is hard to tell when the querification is combined with other changes).

As part of the performance triage process, I'm marking this as a performance regression, and as "triaged". I don't think this merits further investigation; performance regression is focused on smaller crates and is not particularly large.

@rustbot label +perf-regression perf-regression-triaged

Aaron1011 added T-lang Relevant to the language team, which will review and decide on the PR/issue. F-track_caller `#![feature(track_caller)]` labels Jan 24, 2021

rust-highfive assigned lcnr Jan 24, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2021

This comment has been minimized.

Sign in to view

lcnr reviewed Jan 25, 2021

View reviewed changes

lcnr approved these changes Jan 25, 2021

View reviewed changes

lcnr added the I-nominated label Jan 25, 2021

anp approved these changes Jan 25, 2021

View reviewed changes

nikomatsakis removed the I-nominated label Feb 23, 2021

crlf0710 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 12, 2021

eddyb reviewed Mar 18, 2021

View reviewed changes

JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 4, 2021

camelid 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2021

rust-highfive assigned nagisa and unassigned lcnr May 24, 2021

nagisa reviewed May 26, 2021

View reviewed changes

rfcbot added 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 Jun 15, 2021

Mark-Simulacrum removed the I-nominated label Jun 22, 2021

rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Jun 25, 2021

rfcbot added 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 Jun 25, 2021

apiraino removed the to-announce Announce this issue on triage meeting label Jul 1, 2021

Aaron1011 removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jul 2, 2021

Aaron1011 closed this Jul 10, 2021

Aaron1011 reopened this Jul 10, 2021

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 Jul 10, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 10, 2021

bors merged commit 3982eb3 into rust-lang:master Jul 10, 2021

rustbot added this to the 1.55.0 milestone Jul 10, 2021

rust-highfive mentioned this pull request Jul 10, 2021

miri no longer builds after rust-lang/rust#81360 #87030

Closed

rustbot added perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. labels Jul 13, 2021

Mark-Simulacrum mentioned this pull request Oct 10, 2021

1.55.0 segfault compiling stage1 rustc_codegen_llvm on powerpc64le #89744

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support forwarding caller location through trait object method call #81360

Support forwarding caller location through trait object method call #81360

Aaron1011 commented Jan 24, 2021

rust-highfive commented Jan 24, 2021

Aaron1011 commented Jan 24, 2021

This comment has been minimized.

lcnr Jan 25, 2021

anp left a comment

bors commented Feb 2, 2021

cramertj commented Feb 16, 2021

nikomatsakis commented Feb 23, 2021

crlf0710 commented Mar 12, 2021

Aaron1011 commented Mar 13, 2021

eddyb commented Mar 18, 2021

eddyb Mar 18, 2021

JohnCSimon commented May 9, 2021

lcnr commented May 24, 2021

nagisa May 26, 2021

Aaron1011 Jun 4, 2021

rfcbot commented Jun 25, 2021

nagisa commented Jul 10, 2021

RalfJung commented Jul 10, 2021

Aaron1011 commented Jul 10, 2021

Aaron1011 commented Jul 10, 2021

bors commented Jul 10, 2021

bors commented Jul 10, 2021

bors commented Jul 10, 2021

rust-highfive commented Jul 10, 2021

Mark-Simulacrum commented Jul 13, 2021

	// implementation comes from the trait block, and not an impl block
	// implementation comes from the trait block, and not an impl block.

Support forwarding caller location through trait object method call #81360

Support forwarding caller location through trait object method call #81360

Conversation

Aaron1011 commented Jan 24, 2021

rust-highfive commented Jan 24, 2021

Aaron1011 commented Jan 24, 2021

This comment has been minimized.

lcnr Jan 25, 2021

Choose a reason for hiding this comment

anp left a comment

Choose a reason for hiding this comment

bors commented Feb 2, 2021

cramertj commented Feb 16, 2021

nikomatsakis commented Feb 23, 2021

crlf0710 commented Mar 12, 2021

Aaron1011 commented Mar 13, 2021

eddyb commented Mar 18, 2021

eddyb Mar 18, 2021

Choose a reason for hiding this comment

JohnCSimon commented May 9, 2021

lcnr commented May 24, 2021

nagisa May 26, 2021

Choose a reason for hiding this comment

Aaron1011 Jun 4, 2021

Choose a reason for hiding this comment

rfcbot commented Jun 25, 2021

nagisa commented Jul 10, 2021

RalfJung commented Jul 10, 2021

Aaron1011 commented Jul 10, 2021

Aaron1011 commented Jul 10, 2021

bors commented Jul 10, 2021

bors commented Jul 10, 2021

bors commented Jul 10, 2021

rust-highfive commented Jul 10, 2021

Mark-Simulacrum commented Jul 13, 2021