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

Stabilize core::panic::Location::caller #72448

Closed
dtolnay opened this issue May 22, 2020 · 13 comments
Closed

Stabilize core::panic::Location::caller #72448

dtolnay opened this issue May 22, 2020 · 13 comments
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-track_caller `#![feature(track_caller)]` finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented May 22, 2020

The lang FCP for #[track_caller] is happening in #72445. This is a libs FCP for https://doc.rust-lang.org/nightly/std/panic/struct.Location.html#method.caller which is under the same feature gate and required for making use of #[track_caller] in library code.

Refer to @anp's stabilization report and the method's example code.

In particular:

  • The return type is &'static Location<'static> which resembles the return type of std::panic::PanicInfo::location (Option<&'a Location<'a>>) but static and non-optional.

  • The location points to the outermost call site not inside of a #[track_caller] function.

#[track_caller]
fn f() -> &'static Location<'static> {
    Location::caller()
}

fn g() -> &'static Location<'static> {
    f() /*
    ^ calling g() gives a location whose file()/line()/column() point to here
        */
}

@rust-lang/libs

@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 22, 2020
@dtolnay
Copy link
Member Author

dtolnay commented May 22, 2020

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented May 22, 2020

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 22, 2020
@Aaron1011
Copy link
Member

Was a decision ever reached on #69977? It's currently impossible to determine which method call in a chain (e.g. foo.a().b().c() is the actual caller, since a call from any of them will cause Location::caller to point to the beginning of the call chain.

@Aaron1011
Copy link
Member

and required for making use of #[track_caller] in library code.

That's only true if a library wants to programatically inspect the caller, right? It's possible to use #[track_caller] only for its effect on panicking, which doesn't require libraries to use Location::caller

@jonas-schievink jonas-schievink added the F-track_caller `#![feature(track_caller)]` label May 22, 2020
@anp
Copy link
Member

anp commented May 22, 2020

Was a decision ever reached on #69977? It's currently impossible to determine which method call in a chain (e.g. foo.a().b().c() is the actual caller, since a call from any of them will cause Location::caller to point to the beginning of the call chain.

I don’t think any action was decided upon here. I think we can “enhance” the Location type with span end info separately from this feature. I can see an argument for gating stabilization on fixing the underlying issue with spans, but I’m obviously inclined to treat it as a follow up :).

@anp
Copy link
Member

anp commented May 22, 2020

and required for making use of #[track_caller] in library code.

That's only true if a library wants to programatically inspect the caller, right? It's possible to use #[track_caller] only for its effect on panicking, which doesn't require libraries to use Location::caller

This is correct, for custom error types to get the improved panic messages earlier, we could stabilize the attribute separately from the intrinsic wrapper. This seems like a good option to exercise if there are concerns about this FCP which wouldn’t block the attribute from stabilizing, as it would decouple schedule pressures.

@rfcbot 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 May 22, 2020
@rfcbot
Copy link

rfcbot commented May 22, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@Aaron1011
Copy link
Member

I think we can “enhance” the Location type with span end info separately from this feature.

Does this mean that 'span start' info (currently Location::{file, line, column}) will always point to the start of the method chain? If so, I think it should be mentioned in the stabilization report and docs.

@SimonSapin
Copy link
Contributor

Why shouldn’t each call in a chain have its own span?

@anp
Copy link
Member

anp commented May 22, 2020

AIUI there’s a limitation in how MIR propagates spans in method chains but @eddyb should correct me.

@anp
Copy link
Member

anp commented May 23, 2020

I think we can “enhance” the Location type with span end info separately from this feature.

Does this mean that 'span start' info (currently Location::{file, line, column}) will always point to the start of the method chain? If so, I think it should be mentioned in the stabilization report and docs.

I would expect that to change if the Location type became aware of span start/ends, or if MIR becomes able to more accurately provide a span start within the middle of the chain. This doesn't seem problematic to me in general but that's operating from the assumption that changes to span reporting wouldn't be breaking changes. Maybe that assumption is in error?

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Jun 1, 2020
@rfcbot
Copy link

rfcbot commented Jun 1, 2020

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.

@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 1, 2020
@Aaron1011
Copy link
Member

Can this issue be closed now that Location::caller() has been stabilized?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-track_caller `#![feature(track_caller)]` finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants