Skip to content
GitHub no longer supports this web browser. Learn more about the browsers we support.
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

`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]` #67887

Merged
merged 4 commits into from Jan 8, 2020

Conversation

@anp
Copy link
Member

anp commented Jan 5, 2020

The annotated functions now produce panic messages pointing to the location where they were called, rather than core's internals.

anp added 2 commits Jan 5, 2020
Also includes a simple test with a custom panic hook to ensure we don't regress.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 5, 2020

r? @Mark-Simulacrum

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

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Jan 5, 2020

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 5, 2020

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 5, 2020

⌛️ Trying commit 7a6af7e with merge f7a5b63...

bors added a commit that referenced this pull request Jan 5, 2020
`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`

The annotated functions now produce panic messages pointing to the location where they were called, rather than `core`'s internals.
Fix typo
Co-Authored-By: lzutao <taolzu@gmail.com>
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Jan 5, 2020

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 5, 2020

⌛️ Trying commit b97ee0f with merge cbd3d1c...

bors added a commit that referenced this pull request Jan 5, 2020
`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`

The annotated functions now produce panic messages pointing to the location where they were called, rather than `core`'s internals.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 5, 2020

☀️ Try build successful - checks-azure
Build commit: cbd3d1c (cbd3d1c40687e33e53f2bded4ea7764314113ed4)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 5, 2020

Queued cbd3d1c with parent 093241d, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 5, 2020

Finished benchmarking try commit cbd3d1c, comparison URL.

@anp

This comment has been minimized.

Copy link
Member Author

anp commented Jan 5, 2020

I don't see any meaningful changes in the benchmark results, am I missing anything? If not, that's very exciting and a little surprising!

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Jan 5, 2020

@bors r+

yeah, the perf results appear to be within typical variance. Wouldn’t say the results are surprising at all, because we were already generating the location just a different one before the attribute.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 5, 2020

📌 Commit b97ee0f has been approved by nagisa

@anp

This comment has been minimized.

Copy link
Member Author

anp commented Jan 5, 2020

we were already generating the location just a different one before the attribute.

There was concern on the initial RFC about the impact of not being able to unify multiple unwraps' panicking branches because their arguments would be obviously different. Doesn't seem to have shown up in the compiler at least!

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Jan 6, 2020

Oh right, that would definitely affect code size and locality slightly.

@lzutao

This comment has been minimized.

Copy link
Contributor

lzutao commented Jan 6, 2020

Will this PR have effect on 1.42.0 stable?

@anp

This comment has been minimized.

Copy link
Member Author

anp commented Jan 6, 2020

If it lands in time, which I think it will, we have 2-3 weeks before the next beta cut per https://forge.rust-lang.org/#current-release-versions.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jan 6, 2020
`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`

The annotated functions now produce panic messages pointing to the location where they were called, rather than `core`'s internals.
@Aaron1011

This comment has been minimized.

Copy link
Contributor

Aaron1011 commented Jan 7, 2020

@anp: It looks like unwinding isn't supported on wasm32-unknown-unknown:

//! Unwinding for *wasm32* target.
//!
//! Right now we don't support this, so this is just stubs.

I suspect you'll just want to disable this test, like this one:

// ignore-wasm32-bare compiled with panic=abort by default

@anp

This comment has been minimized.

Copy link
Member Author

anp commented Jan 7, 2020

Thanks! Pushed a commit ignoring this test on the wasm32 target. r+ ?

@Dylan-DPC

This comment has been minimized.

Copy link
Member

Dylan-DPC commented Jan 7, 2020

@bors r=nagisa

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 7, 2020

📌 Commit 3acd346 has been approved by nagisa

@jamesmunns

This comment has been minimized.

Copy link
Member

jamesmunns commented Jan 8, 2020

I'd be interested in seeing what the impact of this is within the .text section for embedded applications - as this means that unwrap/expect error messages now have unique (and non de-duplicatable) strings associated with them. I'll plan on trying this out on embedded once the next nightly lands.

I don't think that this information should be a blocker to merging this (it's such a good QoL improvement!) but this does add to the "formatting/panicing is too expensive in embedded" list.

EDIT: error messages now have unique strings, not non-unique

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 8, 2020
`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`

The annotated functions now produce panic messages pointing to the location where they were called, rather than `core`'s internals.
bors added a commit that referenced this pull request Jan 8, 2020
Rollup of 10 pull requests

Successful merges:

 - #67774 (Try statx for all linux-gnu target.)
 - #67781 (Move `is_min_const_fn` query to librustc_mir.)
 - #67798 (Remove wrong advice about spin locks from `spin_loop_hint` docs)
 - #67849 (Add a check for swapped words when we can't find an identifier)
 - #67875 (Distinguish between private items and hidden items in rustdoc)
 - #67887 (`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`)
 - #67955 (rustdoc: Remove more `#[doc(cfg(..))]` duplicates)
 - #67977 (Updates for VxWorks)
 - #67985 (Remove insignificant notes from CStr documentation)
 - #68003 (ci: fix wrong shared.sh import for publish_toolstate)

Failed merges:

 - #67820 (Parse the syntax described in RFC 2632)
 - #67979 (Move `intravisit` => `rustc_hir` + misc cleanup)

r? @ghost
bors added a commit that referenced this pull request Jan 8, 2020
Rollup of 10 pull requests

Successful merges:

 - #67774 (Try statx for all linux-gnu target.)
 - #67781 (Move `is_min_const_fn` query to librustc_mir.)
 - #67798 (Remove wrong advice about spin locks from `spin_loop_hint` docs)
 - #67849 (Add a check for swapped words when we can't find an identifier)
 - #67875 (Distinguish between private items and hidden items in rustdoc)
 - #67887 (`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`)
 - #67955 (rustdoc: Remove more `#[doc(cfg(..))]` duplicates)
 - #67977 (Updates for VxWorks)
 - #67985 (Remove insignificant notes from CStr documentation)
 - #68003 (ci: fix wrong shared.sh import for publish_toolstate)

Failed merges:

 - #67820 (Parse the syntax described in RFC 2632)
 - #67979 (Move `intravisit` => `rustc_hir` + misc cleanup)

r? @ghost
@bors bors merged commit 3acd346 into rust-lang:master Jan 8, 2020
4 checks passed
4 checks passed
pr Build #20200107.35 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@Aaron1011

This comment has been minimized.

Copy link
Contributor

Aaron1011 commented Jan 8, 2020

@anp: Thanks for all of your work implementing this feature!

@anp anp deleted the anp:tracked-std-panics branch Jan 8, 2020
@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 9, 2020

And there was much rejoicing!

@jamesmunns That's a reasonable point. I wonder if we could add an optimization option that people could enable in [profile.release] to disable this. (I absolutely think this should remain enabled by default, in both debug and release.)

@anp

This comment has been minimized.

Copy link
Member Author

anp commented Jan 9, 2020

Something like this was included in the RFC: https://github.com/rust-lang/rfcs/blob/master/text/2091-inline-semantic.md#location-detail-control.

To be honest, I'm not sure how to implement it without having multiple flavors of core/std, similar to some of the GC proposals from a few years ago. Am I missing something?

@anp

This comment has been minimized.

Copy link
Member Author

anp commented Jan 9, 2020

(also may be good to continue this on the tracking issue)

@Pauan

This comment has been minimized.

Copy link
Member

Pauan commented Jan 9, 2020

@joshtriplett I think for heavily resource constrained environments you really want a way to completely disable all panic formatting.

Having more fine-grained control is nice-to-have, but doesn't seem particularly useful to me.

@lzutao

This comment has been minimized.

Copy link
Contributor

lzutao commented Jan 9, 2020

Can we tag this to relnotes tag for it to be included in 1.42 CHANGELOG?

@RalfJung RalfJung added the relnotes label Jan 9, 2020
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 9, 2020

Done.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Jan 9, 2020

@RalfJung For future reference, please also always add a team and a milestone when adding relnotes.

@Centril Centril added the T-compiler label Jan 9, 2020
@Centril Centril added this to the 1.42 milestone Jan 9, 2020
@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 10, 2020

@Pauan Fair.

@bradjc bradjc mentioned this pull request Jan 16, 2020
2 of 2 tasks complete
bors bot added a commit to tock/tock that referenced this pull request Jan 19, 2020
Merge #1546
1546: Update Rust nightly version r=ppannuto a=bradjc

### Pull Request Overview

It has been about 3 months, so a good time to stay current. Also motivated by rust-lang/rust#67887 which should make debugging easier.

Changes
- New warning about `()` not needed.
- Update to update script.


### Testing Strategy

travis


### TODO or Help Wanted

n/a


### Documentation Updated

- [x] Updated the relevant files in `/docs`, or no updates are required.

### Formatting

- [x] Ran `make formatall`.


Co-authored-by: Brad Campbell <bradjc5@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.