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

Add std::error::Report type #91938

Merged
merged 13 commits into from
Jan 14, 2022
Merged

Add std::error::Report type #91938

merged 13 commits into from
Jan 14, 2022

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Dec 14, 2021

This is a continuation of #90174, split into a separate PR since I cannot push to @seanchen1991 's fork

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Dec 14, 2021
@yaahc
Copy link
Member Author

yaahc commented Dec 14, 2021

r? @m-ou-se

@rust-highfive rust-highfive assigned m-ou-se and unassigned kennytm Dec 14, 2021
library/std/src/error/tests.rs Outdated Show resolved Hide resolved
@yaahc
Copy link
Member Author

yaahc commented Dec 14, 2021

Vendoring my most recent comment from the previous PR


Almost done resolving the recent round of comments but I have a couple of style questions that came up during the implementation that I could use some input on.

First, I want to make sure we have the right level of indentation setup for our multiline errors so they match up nicely with the output of Backtrace, and I'm wondering what you think would be best @m-ou-se.

Here's the current version with the changes I made and one source error:

Error: The source of the error

Caused by:
    Error with backtrace

Stack backtrace:
   0: std::error::tests::error_with_backtrace_outputs_correctly
   1: test::__rust_begin_short_backtrace
   2: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   3: std::panicking::try
   4: test::run_test_in_process
   5: std::sys_common::backtrace::__rust_begin_short_backtrace
   6: std::panicking::try
   7: core::ops::function::FnOnce::call_once{{vtable.shim}}
   8: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   9: std::sys::unix::thread::Thread::new::thread_start
  10: start_thread
             at /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8
  11: clone

And with 2 or more sources

Error: Error with two sources

Caused by:
    0: The source of the error
    1: Error with backtrace

Stack backtrace:
   0: std::error::tests::error_with_backtrace_outputs_correctly_with_two_sources
   1: test::__rust_begin_short_backtrace
   2: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   3: std::panicking::try
   4: test::run_test_in_process
   5: std::sys_common::backtrace::__rust_begin_short_backtrace
   6: std::panicking::try
   7: core::ops::function::FnOnce::call_once{{vtable.shim}}
   8: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   9: std::sys::unix::thread::Thread::new::thread_start
  10: start_thread
             at /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8

This is identical to the formatting rules for anyhow, as far as I can tell. Right now it seems to want to line up the error messages so that each message starts on the same column, but my instinct is to line it up so that the causes start on the same column as the symbols in the backtrace. The case where the errors aren't numbered already doesn't line up with the outermost source error message, so either way I'd like it to be changed for consistency, whether or not that ends up being based on the first error message or the backtrace.

My preferred version would look like this

Error: Error with two sources

Caused by:
   0: The source of the error
   1: Error with backtrace

Stack backtrace:
   0: std::error::tests::error_with_backtrace_outputs_correctly_with_two_sources
   1: test::__rust_begin_short_backtrace
   2: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   3: std::panicking::try
   4: test::run_test_in_process
   5: std::sys_common::backtrace::__rust_begin_short_backtrace
   6: std::panicking::try
   7: core::ops::function::FnOnce::call_once{{vtable.shim}}
   8: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   9: std::sys::unix::thread::Thread::new::thread_start
  10: start_thread
             at /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8

and

Error: The source of the error

Caused by:
      Error with backtrace

Stack backtrace:
   0: std::error::tests::error_with_backtrace_outputs_correctly
   1: test::__rust_begin_short_backtrace
   2: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   3: std::panicking::try
   4: test::run_test_in_process
   5: std::sys_common::backtrace::__rust_begin_short_backtrace
   6: std::panicking::try
   7: core::ops::function::FnOnce::call_once{{vtable.shim}}
   8: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
   9: std::sys::unix::thread::Thread::new::thread_start
  10: start_thread
             at /build/glibc-eX1tMB/glibc-2.31/nptl/pthread_create.c:477:8
  11: clone

@rust-log-analyzer

This comment has been minimized.

@camelid camelid added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-error-handling Area: Error handling labels Dec 14, 2021
library/std/src/error.rs Outdated Show resolved Hide resolved
library/std/src/error.rs Outdated Show resolved Hide resolved
library/std/src/error.rs Outdated Show resolved Hide resolved
library/std/src/error/tests.rs Outdated Show resolved Hide resolved
@m-ou-se m-ou-se 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 Dec 15, 2021
@yaahc
Copy link
Member Author

yaahc commented Dec 16, 2021

The current issues I have with the proposed API, note that I don't think these should block it landing, but I will document them in the tracking issue and want to work towards resolving them.

  • show_backtrace currently has no effect if the pretty format isn't enabled, we won't display backtraces ever in the single line format. I dislike this aspect, and am wondering if setting show_backtrace should implicitly also enable pretty, which isn't necessarily much better, but at least it's less error prone.
  • I don't like the name of pretty, in the docs we refer to it as the multi-line format, should it just be multiline?
  • I dislike how requiring the boolean for pretty/show_backtrace means you can't write something like let val = my_result.map_err(Report::new).map_err(Report::pretty).unwrap();, though I'm leaning towards this being the lesser evil compared to the issues with lifetimes and if statements on alternative builder API styles. It's not that big of a deal to introduce a closure like .map_err(|r| r.pretty(should_be_pretty))
  • The current design of Report does not work well as a return value for main, since it needs to be something like fn main() -> Result<(), Report<MyError>>, and Box<dyn Error> wont work since it doesn't impl Error and it would require two simultaneous conversions via From
    • I think the best solution to this is to use similar type erasure tricks to those used in anyhow/extracterr so that we can first store the error in a monomorphized struct with a custom vtable for accessing it's error field, then erase that parameter and only access the error field via the lookup method which returns it as a &dyn Error.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@yaahc yaahc requested a review from m-ou-se December 16, 2021 23:43
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 8, 2022
Add `std::error::Report` type

This is a continuation of rust-lang#90174, split into a separate PR since I cannot push to `@seanchen1991` 's fork
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 9, 2022
Add `std::error::Report` type

This is a continuation of rust-lang#90174, split into a separate PR since I cannot push to ``@seanchen1991`` 's fork
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 10, 2022
Add `std::error::Report` type

This is a continuation of rust-lang#90174, split into a separate PR since I cannot push to ```@seanchen1991``` 's fork
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 10, 2022
Add `std::error::Report` type

This is a continuation of rust-lang#90174, split into a separate PR since I cannot push to ````@seanchen1991```` 's fork
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 12, 2022
Add `std::error::Report` type

This is a continuation of rust-lang#90174, split into a separate PR since I cannot push to `````@seanchen1991````` 's fork
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 13, 2022
Add `std::error::Report` type

This is a continuation of rust-lang#90174, split into a separate PR since I cannot push to ``````@seanchen1991`````` 's fork
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 14, 2022
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#90001 (Make rlib metadata strip works with MIPSr6 architecture)
 - rust-lang#91687 (rustdoc: do not emit tuple variant fields if none are documented)
 - rust-lang#91938 (Add `std::error::Report` type)
 - rust-lang#92006 (Welcome opaque types into the fold)
 - rust-lang#92142 ([code coverage] Fix missing dead code in modules that are never called)
 - rust-lang#92277 (rustc_metadata: Stop passing `CrateMetadataRef` by reference (step 1))
 - rust-lang#92334 (rustdoc: Preserve rendering of macro_rules matchers when possible)
 - rust-lang#92807 (Update cargo)
 - rust-lang#92832 (Update RELEASES for 1.58.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e045c79 into rust-lang:master Jan 14, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 14, 2022
@yaahc yaahc deleted the error-reporter branch January 14, 2022 18:25
@yaahc yaahc mentioned this pull request Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error-handling Area: Error handling S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants