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

impl iter() for dyn Error #58289

Merged
merged 1 commit into from
Feb 13, 2019
Merged

impl iter() for dyn Error #58289

merged 1 commit into from
Feb 13, 2019

Conversation

haraldh
Copy link
Contributor

@haraldh haraldh commented Feb 8, 2019

Examples:

let next_error_type_a = err
    .iter()
    .filter_map(Error::downcast_ref::<ErrorTypeA>)
    .next();
let source_root_error = err.iter().last();

Credit for the ErrorIter goes to reddit user /u/tdiekmann (Tim Diekmann)
https://www.reddit.com/r/rust/comments/aj3lpg/is_an_iterator_impl_over_errorsource_possible/

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2019
@haraldh haraldh force-pushed the master branch 2 times, most recently from 1831ef6 to 5a9c457 Compare February 8, 2019 07:13
@sfackler
Copy link
Member

sfackler commented Feb 8, 2019

Thanks for the PR! I think this kind of functionality is definitely useful, but I think we'll want to tweak the API a bit. In particular, I think a method name like sources() or iter_sources() is more descriptive than simply iter(). The failure crate also provides iterators off of both the causes of the error as well as the error itself followed by the causes. Both of those are useful based on the specific context of what you're doing, so it seems like a good idea to do the same here: https://docs.rs/failure/0.1.5/failure/trait.Fail.html#method.iter_causes

cc @withoutboats

@haraldh
Copy link
Contributor Author

haraldh commented Feb 8, 2019

agreed with iter_sources() .. changed the PR to iter_sources() and iter_chain()

@TimDiekmann
Copy link
Member

I'd return ErrorIter directly instead of impl Iterator. This way it's possible to nest the type in other structs.

@haraldh
Copy link
Contributor Author

haraldh commented Feb 8, 2019

Which would make ErrorIter pub.

@TimDiekmann
Copy link
Member

Yes, this is intended. Otherwise you have to box it to store it.

@sfackler
Copy link
Member

sfackler commented Feb 9, 2019

Ah yeah, we should return a concrete iterator type.

@haraldh
Copy link
Contributor Author

haraldh commented Feb 9, 2019

Ok, ErrorIter is returned and pub

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:225af542:start=1549722635420175979,finish=1549722708982251417,duration=73562075438
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:04:37]    Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[00:04:43] error: This node does not have a stability attribute
[00:04:43]    --> src/libstd/error.rs:807:1
[00:04:43]     |
[00:04:43] 807 | / pub struct ErrorIter<'a> {
[00:04:43] 808 | |     current: Option<&'a (dyn Error + 'static)>,
[00:04:43]     | |_^
[00:04:43] 
[00:04:43] error: This node does not have a stability attribute
[00:04:43]    --> src/libstd/error.rs:811:1
[00:04:43]    --> src/libstd/error.rs:811:1
[00:04:43]     |
[00:04:43] 811 | / impl<'a> Iterator for ErrorIter<'a> {
[00:04:43] 812 | |     type Item = &'a (dyn Error + 'static);
[00:04:43] 813 | |
[00:04:43] 814 | |     fn next(&mut self) -> Option<Self::Item> {
[00:04:43] 818 | |     }
[00:04:43] 819 | | }
[00:04:43]     | |_^
[00:04:43] 
[00:04:43] 
[00:04:43] error: type does not implement `fmt::Debug`; consider adding #[derive(Debug)] or a manual implementation
[00:04:43]     |
[00:04:43] 807 | / pub struct ErrorIter<'a> {
[00:04:43] 807 | / pub struct ErrorIter<'a> {
[00:04:43] 808 | |     current: Option<&'a (dyn Error + 'static)>,
[00:04:43]     | |_^
[00:04:43]     |
[00:04:43] note: lint level defined here
[00:04:43]    --> src/libstd/lib.rs:210:9
---
[00:04:44] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:04:44] expected success, got: exit code: 101
[00:04:44] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:04:44] Build completed unsuccessfully in 0:00:47
[00:04:44] make: *** [all] Error 1
[00:04:44] Makefile:18: recipe for target 'all' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:09d9d2e3
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sat Feb  9 14:36:41 UTC 2019
---
travis_time:end:064404e8:start=1549723002574247264,finish=1549723002580168139,duration=5920875
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0f1e3944
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0ace9852
travis_time:start:0ace9852
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:028f4634
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0331c84e:start=1549724228611119600,finish=1549724301514411354,duration=72903291754
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:03:49]    Compiling alloc v0.0.0 (/checkout/src/liballoc)
[00:03:49]    Compiling rustc-demangle v0.1.10
[00:03:49]    Compiling panic_abort v0.0.0 (/checkout/src/libpanic_abort)
[00:03:54]    Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[00:04:00] error: type does not implement `fmt::Debug`; consider adding #[derive(Debug)] or a manual implementation
[00:04:00]     |
[00:04:00] 808 | / pub struct ErrorIter<'a> {
[00:04:00] 808 | / pub struct ErrorIter<'a> {
[00:04:00] 809 | |     current: Option<&'a (dyn Error + 'static)>,
[00:04:00]     | |_^
[00:04:00]     |
[00:04:00] note: lint level defined here
[00:04:00]    --> src/libstd/lib.rs:210:9
---
[00:04:00] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:04:00] expected success, got: exit code: 101
[00:04:00] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:04:00] Build completed unsuccessfully in 0:00:47
[00:04:00] make: *** [all] Error 1
[00:04:00] Makefile:18: recipe for target 'all' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1779ed36
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sat Feb  9 15:02:31 UTC 2019
---
travis_time:end:06db1de0:start=1549724552186209773,finish=1549724552191759236,duration=5549463
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0001b30f
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:180c11b3
travis_time:start:180c11b3
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_fa

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Examples:

```rust
let next_error_type_a = err
    .iter_chain()
    .filter_map(Error::downcast_ref::<ErrorTypeA>)
    .next();
```

```rust
let source_root_error = err.iter_chain().last();
```

Credit for the ErrorIter goes to Tim Diekmann
https://www.reddit.com/r/rust/comments/aj3lpg/is_an_iterator_impl_over_errorsource_possible/
@haraldh
Copy link
Contributor Author

haraldh commented Feb 11, 2019

More comments?

@sfackler
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 12, 2019

📌 Commit f06af1f has been approved by sfackler

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 12, 2019
Centril added a commit to Centril/rust that referenced this pull request Feb 13, 2019
impl iter() for dyn Error

Examples:

```rust
let next_error_type_a = err
    .iter()
    .filter_map(Error::downcast_ref::<ErrorTypeA>)
    .next();
```

```rust
let source_root_error = err.iter().last();
```

Credit for the ErrorIter goes to reddit user /u/tdiekmann (Tim Diekmann)
https://www.reddit.com/r/rust/comments/aj3lpg/is_an_iterator_impl_over_errorsource_possible/
Centril added a commit to Centril/rust that referenced this pull request Feb 13, 2019
impl iter() for dyn Error

Examples:

```rust
let next_error_type_a = err
    .iter()
    .filter_map(Error::downcast_ref::<ErrorTypeA>)
    .next();
```

```rust
let source_root_error = err.iter().last();
```

Credit for the ErrorIter goes to reddit user /u/tdiekmann (Tim Diekmann)
https://www.reddit.com/r/rust/comments/aj3lpg/is_an_iterator_impl_over_errorsource_possible/
bors added a commit that referenced this pull request Feb 13, 2019
Rollup of 13 pull requests

Successful merges:

 - #57693 (Doc rewording)
 - #57815 (Speed up the fast path for assert_eq! and assert_ne!)
 - #58034 (Stabilize the time_checked_add feature)
 - #58057 (Stabilize linker-plugin based LTO (aka cross-language LTO))
 - #58137 (Cleanup: rename node_id_to_type(_opt))
 - #58166 (allow shorthand syntax for deprecation reason)
 - #58196 (Add specific feature gate error for const-unstable features)
 - #58200 (fix str mutating through a ptr derived from &self)
 - #58273 (Rename rustc_errors dependency in rust 2018 crates)
 - #58289 (impl iter() for dyn Error)
 - #58387 (Disallow `auto` trait alias syntax)
 - #58404 (use Ubuntu keyserver for CloudABI ports)
 - #58405 (Remove some dead code from libcore)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Feb 13, 2019
impl iter() for dyn Error

Examples:

```rust
let next_error_type_a = err
    .iter()
    .filter_map(Error::downcast_ref::<ErrorTypeA>)
    .next();
```

```rust
let source_root_error = err.iter().last();
```

Credit for the ErrorIter goes to reddit user /u/tdiekmann (Tim Diekmann)
https://www.reddit.com/r/rust/comments/aj3lpg/is_an_iterator_impl_over_errorsource_possible/
bors added a commit that referenced this pull request Feb 13, 2019
Rollup of 12 pull requests

Successful merges:

 - #57693 (Doc rewording)
 - #57815 (Speed up the fast path for assert_eq! and assert_ne!)
 - #58034 (Stabilize the time_checked_add feature)
 - #58057 (Stabilize linker-plugin based LTO (aka cross-language LTO))
 - #58137 (Cleanup: rename node_id_to_type(_opt))
 - #58166 (allow shorthand syntax for deprecation reason)
 - #58200 (fix str mutating through a ptr derived from &self)
 - #58273 (Rename rustc_errors dependency in rust 2018 crates)
 - #58289 (impl iter() for dyn Error)
 - #58387 (Disallow `auto` trait alias syntax)
 - #58404 (use Ubuntu keyserver for CloudABI ports)
 - #58405 (Remove some dead code from libcore)

Failed merges:

r? @ghost
@bors bors merged commit f06af1f into rust-lang:master Feb 13, 2019
@haraldh
Copy link
Contributor Author

haraldh commented Feb 13, 2019

@sfackler : What's the procedure for this to make it into stable?

@haraldh
Copy link
Contributor Author

haraldh commented Feb 14, 2019

The usability is not very ergonomic though:

    let mut iter = (&b as &(dyn Error)).iter_chain();
    // or
    let b = Box::<Error>::from(b); let mut iter = b.iter_chain();
    // or
    let mut iter = <dyn Error>::iter_chain(&b);
    // or
    let mut iter = Error::iter_chain(&b);
    // but not
    let mut iter = b.iter_chain();

Where we already have a Box<Error> it's not a problem, but for other cases it feels a little bit clunky.

@TimDiekmann
Copy link
Member

It may be better to implement it as a provided trait method.

@haraldh
Copy link
Contributor Author

haraldh commented Feb 14, 2019

pub trait Error: Debug + Display {fn iter_chain(&self) -> ErrorIter
    where
        Self: Sized + 'static,
    {
        <dyn Error>::iter_chain(self)
    }}

will give for:

    let mut iter = b.iter_chain();
247 |         <dyn Error>::iter_chain(self)
    |         ^^^^^^^^^^^^^^^^^^^^^^^ multiple `iter_chain` found

How can I call the impl dyn Error method explicitly?

@TimDiekmann
Copy link
Member

I meant, don't implement the method in impl dyn Error at all, but only in trait Error. dyn Error is only needed for downcasting.

@haraldh
Copy link
Contributor Author

haraldh commented Feb 14, 2019

if the impl for the type (dyn Error) is removed

pub trait Error: Debug + Display {fn iter_chain(&self) -> ErrorIter
    where
        Self: Sized + 'static,
    {
        ErrorIter {
            current: Some(self),
        }
    }}

will give for:

    let err: Box<Error> = b.into();
    let mut iter = err.iter_chain();
error: the `iter_chain` method cannot be invoked on a trait object
  --> src/lib.rs:592:20
   |
37 | let mut iter = err.iter_chain();
   |                    ^^^^^^^^^^

@TimDiekmann
Copy link
Member

Ah, I see ...

@haraldh
Copy link
Contributor Author

haraldh commented Feb 14, 2019

And with both in place returning ErrorIter:

    let err: Box<Error> = b.into();
    let mut iter = err.iter_chain();
37 | let mut iter = err.iter_chain();
   |                    ^^^^^^^^^^ multiple `iter_chain` found
   |
   = note: candidate #1 is defined in an impl for the type `(dyn Error + 'static)`
note: candidate #2 is defined in the trait `Error`

Where #2 cannot even be used 👎

@haraldh
Copy link
Contributor Author

haraldh commented Feb 14, 2019

Anyway, I could probably live with:

 let mut iter = Error::iter_chain(&b);

which works as of now

@haraldh
Copy link
Contributor Author

haraldh commented Feb 14, 2019

@withoutboats
Copy link
Contributor

withoutboats commented Feb 16, 2019

This needs a proper tracking issue to track stabilization and let people report feedback (the issue provided is to this pull request). I also think that iter_chain is out of the norm for std iterator methods and we should change it (IMO the deprecated failure API is more consistent with the std norm than the undeprecated one, in which this would probably be called sources).

@sfackler
Copy link
Member

Oh right, whoops. Tracking issue fix: #58521

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants