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

Conversation

Projects
None yet
6 participants
@haraldh
Copy link
Contributor

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 8, 2019

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.

@haraldh haraldh force-pushed the haraldh:master branch 2 times, most recently from 1831ef6 to 5a9c457 Feb 8, 2019

@sfackler

This comment has been minimized.

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 haraldh force-pushed the haraldh:master branch from 5a9c457 to d90e60a Feb 8, 2019

@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Feb 8, 2019

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

@haraldh haraldh force-pushed the haraldh:master branch from d90e60a to 2cb190b Feb 8, 2019

@TimDiekmann

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 8, 2019

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

@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Feb 8, 2019

Which would make ErrorIter pub.

@TimDiekmann

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 8, 2019

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

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Feb 9, 2019

Ah yeah, we should return a concrete iterator type.

@haraldh haraldh force-pushed the haraldh:master branch from 2cb190b to 2659433 Feb 9, 2019

@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Feb 9, 2019

Ok, ErrorIter is returned and pub

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 9, 2019

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)

@haraldh haraldh force-pushed the haraldh:master branch from 2659433 to 7058961 Feb 9, 2019

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 9, 2019

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)

impl iter_sources() and iter_chain() for dyn Error
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 haraldh force-pushed the haraldh:master branch from 7058961 to f06af1f Feb 9, 2019

@haraldh

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Feb 11, 2019

More comments?

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Feb 12, 2019

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 12, 2019

📌 Commit f06af1f has been approved by sfackler

Centril added a commit to Centril/rust that referenced this pull request Feb 13, 2019

Rollup merge of rust-lang#58289 - haraldh:master, r=sfackler
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

Rollup merge of rust-lang#58289 - haraldh:master, r=sfackler
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

Auto merge of #58413 - Centril:rollup, r=Centril
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

Rollup merge of rust-lang#58289 - haraldh:master, r=sfackler
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

Auto merge of #58415 - Centril:rollup, r=Centril
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

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Feb 13, 2019

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

@haraldh

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 14, 2019

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

@haraldh

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 14, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 14, 2019

Ah, I see ...

@haraldh

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

haraldh commented Feb 14, 2019

@withoutboats

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

sfackler commented Feb 16, 2019

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