Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support #[track_caller] on closures. #74492

Closed
wants to merge 1 commit into from
Closed

Conversation

anp
Copy link
Member

@anp anp commented Jul 18, 2020

Closes #74042.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2020
@anp
Copy link
Member Author

anp commented Jul 18, 2020

Fails locally with:

thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `2`,
 right: `3`: #[track_caller] fn's must have 1 more argument in their ABI than in their MIR'

@lcnr
Copy link
Contributor

lcnr commented Jul 19, 2020

afaik closures use FnOnce::call_once, which has the "rust-call" calling convention.

This calling convention takes the argument list and wraps them in a tuple to support generics.
Not sure what has to be done about that here though 🤔

@anp anp force-pushed the track-closures branch from bd80081 to 3d39b9c Compare July 19, 2020 16:55
@anp
Copy link
Member Author

anp commented Jul 19, 2020

Thanks! That jogged my memory for the portion of codegen to look at. Since the assertion is there to prevent miscompilation I tried commenting it out and the argument passing appeared to "just work". Will still need to figure out the right number to assert on in the presence of untupled args but that can be done later.

The next issue is that the "definition span" we get for a closure-as-fn-ptr appears to be in libcore:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `"/home/adam/rust/src/libcore/ops/function.rs"`,
 right: `"/home/adam/rust/src/test/ui/rfc-2091-track-caller/track-caller-closure.rs"`', /home/adam/rust/src/test/ui/rfc-2091-track-caller/track-caller-closure.rs:52:5

I think this will need to be modified in

crate fn find_closest_untracked_caller_location(&self) -> Span {
but I might be wrong and it might also require special closure logic when resolving fn ptrs in
pub fn resolve_for_fn_ptr(
.

cc @eddyb btw

@eddyb
Copy link
Member

eddyb commented Jul 19, 2020

Sounds like you need to make some of the Fn-trait-related shims pretend to have #[track_caller] depending on whether the underlying function/closure does. I didn't think about that when I suggested closures "should just work" but:

You might be able to get away with testing a case where a closure doesn't implement more than FnOnce (e.g. capture something like a String that is entirely consumed by the closure body), because that shouldn't get any shims.

Then again, you mention coercing to a fn pointer being the only case that doesn't work, I'm not sure what the best solution for that is, maybe checking that we're in a reify shim and doing Instance::resolve on the DefId/Substs pair of the reify shim? That way, we may get back an InstanceDef::Item referring to the closure, or a shim that makes it clear what the closure DefId is?

// fn_abi.args.len(),
// args.len() + 1,
// "#[track_caller] fn's must have 1 more argument in their ABI than in their MIR",
// );
Copy link
Member

@bjorn3 bjorn3 Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the abi is "Rust-call" then the argument count is 1 (for the self argument) + the amount of elements in the type of the second argument (the operands bundle, which is a tuple).

@Muirrum Muirrum 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 Aug 6, 2020
@Dylan-DPC-zz
Copy link

@anp any updates?

@bors
Copy link
Contributor

bors commented Aug 8, 2020

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

@Dylan-DPC-zz
Copy link

@anp thanks for taking the time to contribute. Sadly this PR is inactive so I'm going to close it. If you wish and you have the time to complete this, you can open a new pull request and we'll take it from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[track_caller] on closures?
8 participants