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

Tracking Issue for #[track_caller] on closures #87417

Open
1 of 3 tasks
Aaron1011 opened this issue Jul 23, 2021 · 12 comments
Open
1 of 3 tasks

Tracking Issue for #[track_caller] on closures #87417

Aaron1011 opened this issue Jul 23, 2021 · 12 comments
Labels
A-closures Area: closures (`|args| { .. }`) C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-track_caller `#![feature(track_caller)]` S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Jul 23, 2021

This is a tracking issue for the RFC "XXX" (rust-lang/rfcs#NNN).
The feature gate for the issue is #![feature(closure_track_caller)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

XXX --- list all the "unresolved questions" found in the RFC to ensure they are
not forgotten

  • Should async {}, etc support track caller on their lowered closures?

Implementation history

@Aaron1011 Aaron1011 added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jul 23, 2021
@jonas-schievink jonas-schievink added A-closures Area: closures (`|args| { .. }`) F-track_caller `#![feature(track_caller)]` T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 23, 2021
@joshtriplett joshtriplett added the S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR label Jul 27, 2022
@DesmondWillowbrook
Copy link
Contributor

What's the status of this issue? I'd be happy to try and pursue stabilization if it's ready.

@Aaron1011
Copy link
Member Author

Even if this is stabilized, you'll still need the unstable stmt_expr_attributes to actually write it on a closure expression: #15701

@mpfaff
Copy link

mpfaff commented Jan 22, 2023

Even if this is stabilized, you'll still need the unstable stmt_expr_attributes to actually write it on a closure expression: #15701

That seems to be the case only if the closure is not passed immediately as a function argument:

// This works
let x = std::convert::identity(#[track_caller] || {});
// But this does not
let x = #[track_caller] || {};

Maybe I'm misunderstanding the terminology, but in the case that does work, the closure is still an expression, no?

@Aaron1011
Copy link
Member Author

You're right - this seems like a bug in the stmt_expr_attributes feature gate (or maybe an intentional partial stabilization).

teskje added a commit to teskje/materialize that referenced this issue Jan 27, 2023
`#[track_caller]` has no effect on async functions currently, and the
compiler warns about that. However, fixing this is in progress ([0]).
Rather than removing the annotations from async functions, we instead
ignore the compiler warning, so we don't forget to add the annotations
again once [0] is implemented.

[0]: rust-lang/rust#87417
fredr added a commit to EmbarkStudios/octobors that referenced this issue Mar 7, 2023
fredr added a commit to EmbarkStudios/octobors that referenced this issue Mar 8, 2023
* Set log lever via RUST_LOG

* track_caller on async is a no-op see issue #87417 <rust-lang/rust#87417> for more information

* cargo update and fix deny

* ignore clippy warning for no-op track_caller
@tmandry
Copy link
Member

tmandry commented Mar 28, 2023

Nominating for lang team discussion. We may not be ready to approve attributes on closures generally. However, this being unstable led to the backing out of#[track_caller] support on async fn in #104741 (originally implemented in #104219) unless the closure_track_caller feature gate is enabled, because it uses that feature under the hood.

@rustbot label I-lang-nominated I-async-nominated

@rust-lang/lang, can we get consensus on stabilizing support for #[track_caller] on async fn only, and does that need an FCP?

The only possible question I see is whether caller tracking should occur during initial call or subsequent polling; this follows the approach adopted and FCP'd in #87064 and adds it to poll. One implication of this is that combinators would need to add #[track_caller] to their poll implementations to be able to "see through" them to user code. @rust-lang/wg-async in case there are any concerns about this.

Additionally, how close are we to consensus on stabilizing closure_track_caller if a stabilization PR were to come up?

@rustbot rustbot added I-async-nominated The issue / PR has been nominated for discussion during an async working group meeting. I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. labels Mar 28, 2023
@nikomatsakis
Copy link
Contributor

One clarification @tmandry -- why exactly did we back things out? Was it because we were afraid of committing to something that would require a feature gate, or because the compiler didn't accept the patch because of the feature gate being needed on stable? (If the latter, there's a way to deal with that.)

That said, I feel fairly comfortable with approving attributes on closures.

@tmandry
Copy link
Member

tmandry commented Mar 29, 2023

One clarification @tmandry -- why exactly did we back things out? Was it because we were afraid of committing to something that would require a feature gate, or because the compiler didn't accept the patch because of the feature gate being needed on stable? (If the latter, there's a way to deal with that.)

This wasn't entirely clear to me, but I think it was more the latter - or more specifically, because the original patch had a bug that caused an error to appear on stable when #[track_caller] was used (previously it was accepted as a no-op.) @bryangarza may be able to add more background.

@bryangarza
Copy link
Contributor

We reused the closure_track_caller because it was already in the code, but I think we could make a separate flag for async fn only if we want to stabilize just that part.

@tmandry
Copy link
Member

tmandry commented Apr 4, 2023

We discussed this in the t-lang meeting (notes).

We agreed that we should make a separate flag for #[track_caller] on async fn and then FCP that. The rest of this comment can be copied over to the new tracking issue once it is created to kick off the FCP.


It's not 100% clear that tracking the poller, rather than the original caller, is the ideal behavior. That said, we may still be able to resolve the question without an RFC.

Arguments in favor of tracking the poller (current behavior):

  • The user might expect to see the call that's happening now rather than one that happened in the past.
  • Prevents us from having to store the caller in the Future type.
  • Usually poller and caller are the same function, anyway, so we should do the simpler and more performant thing.

Arguments in favor of tracking the original caller instead of the poller:

  • User might expect to see the actual caller; especially given the name track_caller, showing the poller can confusing.
  • A &'static Location is only one pointer; storing it is not so bad.
  • Combinators won't need to add #[track_caller] to "thread through" the caller information.
  • If we tracked the poller, #[track_caller] on a spawned task would show the executor.
    • There's no way today for the executor to save off caller location information (e.g. in spawn) and re-provide it to the polled function, even if it wanted to. We could fix this with a set_caller API.
    • Tracking won't work for &dyn Future. Executors can easily work around this if they have an API with access to the underlying type before coercing to dyn, as most do (playground). But if you happen to already have a dyn Future, you'll be stuck.

@tmandry
Copy link
Member

tmandry commented Apr 5, 2023

@bryangarza Would you be interested in making a separate feature flag and (optionally) opening a new tracking issue?

@bryangarza
Copy link
Contributor

@tmandry sure, I can do both of those

@tmandry tmandry removed I-async-nominated The issue / PR has been nominated for discussion during an async working group meeting. I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. labels Apr 10, 2023
@RalfJung
Copy link
Member

This is a tracking issue for the RFC "XXX" (rust-lang/rfcs#NNN).

Is there an RFC? Or is this just noise that should be removed from the issue?
I was looking for a high-level description of the design here, where can I find that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: closures (`|args| { .. }`) C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-track_caller `#![feature(track_caller)]` S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants