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

Multiple fixes to get rustup building #75

Merged
merged 8 commits into from Nov 30, 2016

Conversation

Projects
None yet
4 participants
@brson
Contributor

brson commented Nov 30, 2016

In attempting to upgrade rustup to error-chain 0.6.2 I found multiple irreconcilable incompatibilities. A number of previous changes had unintended impacts. This restores error-chain to a state that rustup can build again.

The three changes this rolls back are:

  • Defining the chain_err extension trait inside the error_chain crate. This extension must be defined on a per-crate basis for inference to work correctly.
  • Making ErrorKind not required in links { } sections. This resulted in coherence errors in cross crate scenarios due to conflicting From<ErrorKind> impls.
  • Making Error a struct. This type is a tuple so that pattern matching on it is nice.

I've added test cases for these problems.

Fixes #68, probably others.

r? @Yamakaky

cc @alexcrichton @nikomatsakis

brson added some commits Nov 30, 2016

Make Error a tuple again
This is an ergonomic concern. When matching on an error-chain error
it is more convenient to write

```
match download_with_backend(backend, url, callback) {
    Err(Error(ErrorKind::BackendUnavailable(_), _)) => (),
    Err(e) => return Err(e),
    Ok(()) => return Ok(()),
}
```

than

```
match download_with_backend(backend, url, callback) {
    Err(Error { kind: ErrorKind::BackendUnavailable(_), .. }) => (),
    Err(e) => return Err(e),
    Ok(()) => return Ok(()),
}
```
Add ErrorKind back to the links { } section to fix coherence issues
Without this one cannot actually create links across crates.
Add more docs
Especially about pattern matching.

@brson brson changed the title from Multiple fixes for regressions to Multiple fixes to get rustup building Nov 30, 2016

@brson brson referenced this pull request Nov 30, 2016

Merged

Set up error-chain #1

@colin-kiegel

This comment has been minimized.

Show comment
Hide comment
@colin-kiegel

colin-kiegel Nov 30, 2016

I want to comment on the tuple vs. named struct issue:

It's probably a matter of taste, but personally I strongly prefer named fields on a struct where the fields have different types. I like tuple structs only in cases where all types are the same (like 3d coordinates).

That was the one and only thing I didn't like about error_chain very much in the past and I was pleased to see the switch to named fields.

colin-kiegel commented Nov 30, 2016

I want to comment on the tuple vs. named struct issue:

It's probably a matter of taste, but personally I strongly prefer named fields on a struct where the fields have different types. I like tuple structs only in cases where all types are the same (like 3d coordinates).

That was the one and only thing I didn't like about error_chain very much in the past and I was pleased to see the switch to named fields.

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 30, 2016

Collaborator

Ok for ResultExt, by why removing ChainedError and the struct?
BTW, #73 should solve this, but it misses trait specialization.

Collaborator

Yamakaky commented Nov 30, 2016

Ok for ResultExt, by why removing ChainedError and the struct?
BTW, #73 should solve this, but it misses trait specialization.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Nov 30, 2016

Contributor

Thanks for the review @Yamakaky!

That was the one and only thing I didn't like about error_chain very much in the past and I was pleased to see the switch to named fields.

How does Error being defined as a struct impact your experience? Do you prefer matching on struct fields?

Ok for ResultExt, by why removing ChainedError and the struct?

I don't understand the question. ChainedError still exists after this PR. What struct are you referring to?

BTW, #73 should solve this, but it misses trait specialization.

What is it that #73 solves, and how?

I feel strongly about everything in this patch and am going to merge it to get the fixes out.

Contributor

brson commented Nov 30, 2016

Thanks for the review @Yamakaky!

That was the one and only thing I didn't like about error_chain very much in the past and I was pleased to see the switch to named fields.

How does Error being defined as a struct impact your experience? Do you prefer matching on struct fields?

Ok for ResultExt, by why removing ChainedError and the struct?

I don't understand the question. ChainedError still exists after this PR. What struct are you referring to?

BTW, #73 should solve this, but it misses trait specialization.

What is it that #73 solves, and how?

I feel strongly about everything in this patch and am going to merge it to get the fixes out.

@brson brson merged commit 5b37b30 into master Nov 30, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Nov 30, 2016

Contributor

I'll publish a new build this afternoon.

Contributor

brson commented Nov 30, 2016

I'll publish a new build this afternoon.

@@ -224,8 +224,8 @@ macro_rules! error_chain_processed {
$(
$(#[$meta_links])*
impl From<<$link_error_path as $crate::ChainedError>::ErrorKind> for $error_kind_name {
fn from(e: <$link_error_path as $crate::ChainedError>::ErrorKind) -> Self {
impl From<$link_kind_path> for $error_kind_name {

This comment has been minimized.

@Yamakaky

Yamakaky Nov 30, 2016

Collaborator

Well, you keep ChainedError, but you also remove it main advantage, so what the point? Now you have to specify ErrorKind in links.

@Yamakaky

Yamakaky Nov 30, 2016

Collaborator

Well, you keep ChainedError, but you also remove it main advantage, so what the point? Now you have to specify ErrorKind in links.

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 30, 2016

Collaborator

What's the advantage of reverting to Error as a tuple? I only see the disadvantage that the macro code is less readable... Also, you talk about matching, but I can't see a use-case where you couldn't use the defined methods.
#73 uses a crate-level defined Error instead of one for each invocation, so it should remove the inference problem. The trouble is that I get conflicting implementations for From since the code is too generic, I think they would go away with trait specialization.

Collaborator

Yamakaky commented Nov 30, 2016

What's the advantage of reverting to Error as a tuple? I only see the disadvantage that the macro code is less readable... Also, you talk about matching, but I can't see a use-case where you couldn't use the defined methods.
#73 uses a crate-level defined Error instead of one for each invocation, so it should remove the inference problem. The trouble is that I get conflicting implementations for From since the code is too generic, I think they would go away with trait specialization.

@colin-kiegel

This comment has been minimized.

Show comment
Hide comment
@colin-kiegel

colin-kiegel Nov 30, 2016

@brson Yes indeed I prefer matching on named fields, because it is agnostic of field ordering. Of course I have to remember field names instead. But I rather remember something which has additional meaning instead of something arbitrary like randomly fixed ordering. And generally speaking, when I match pattern against named fields I feel more comfortable because I know it will break if the meaning of the fields changes, even if the type is still the same.

Another use case is this: If I am writing a library and have to tell everyone "these are the errors you will get" I would like them to be in a nicely documented shape. And I also like explicit / self-documenting code. IDE-like tools like racer will be able to make meaningful auto-completions if someone puts a . after a struct instance with named fields. For me personally it would therefore definitely not be an option to export errors in a library, whose fields are unnamed. But I am certainly not representative of everyone. :-)

PS: I only make exceptions for tuple structs, if the struct has (a) one field or is (b) multi-dimensional over exactly the same type with the same meaning (like coordinates).

colin-kiegel commented Nov 30, 2016

@brson Yes indeed I prefer matching on named fields, because it is agnostic of field ordering. Of course I have to remember field names instead. But I rather remember something which has additional meaning instead of something arbitrary like randomly fixed ordering. And generally speaking, when I match pattern against named fields I feel more comfortable because I know it will break if the meaning of the fields changes, even if the type is still the same.

Another use case is this: If I am writing a library and have to tell everyone "these are the errors you will get" I would like them to be in a nicely documented shape. And I also like explicit / self-documenting code. IDE-like tools like racer will be able to make meaningful auto-completions if someone puts a . after a struct instance with named fields. For me personally it would therefore definitely not be an option to export errors in a library, whose fields are unnamed. But I am certainly not representative of everyone. :-)

PS: I only make exceptions for tuple structs, if the struct has (a) one field or is (b) multi-dimensional over exactly the same type with the same meaning (like coordinates).

@colin-kiegel

This comment has been minimized.

Show comment
Hide comment
@colin-kiegel

colin-kiegel Nov 30, 2016

PS: I also don't see the big disadvantage of matching on named fields.

    match foo {
        Foo { ref name, ref age } => {
            println!("age={}, name={}", age, name);
        }
    }

https://play.rust-lang.org/?gist=91d74900f2e15304f5b754ad99a73609&version=stable&backtrace=0

colin-kiegel commented Nov 30, 2016

PS: I also don't see the big disadvantage of matching on named fields.

    match foo {
        Foo { ref name, ref age } => {
            println!("age={}, name={}", age, name);
        }
    }

https://play.rust-lang.org/?gist=91d74900f2e15304f5b754ad99a73609&version=stable&backtrace=0

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 30, 2016

Collaborator

OK for ErrorKind, but you do you have an example of matching directly on Error?

Collaborator

Yamakaky commented Nov 30, 2016

OK for ErrorKind, but you do you have an example of matching directly on Error?

@colin-kiegel

This comment has been minimized.

Show comment
Hide comment
@colin-kiegel

colin-kiegel Nov 30, 2016

My example above was of course pretty silly. In case of only one matching branch it can be simplified to let or if let:

    let Foo { ref name, ref age } = foo;
    println!("age={}, name={}", age, name);

And this should work for errors from this macro crate accordingly:

    let MyError { ref kind, ref state } = my_error;
    // ...

colin-kiegel commented Nov 30, 2016

My example above was of course pretty silly. In case of only one matching branch it can be simplified to let or if let:

    let Foo { ref name, ref age } = foo;
    println!("age={}, name={}", age, name);

And this should work for errors from this macro crate accordingly:

    let MyError { ref kind, ref state } = my_error;
    // ...
@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 30, 2016

Collaborator

I know how to do a match, I just want to know why you would do that. Aren't kind() and backtrace() sufficient?

Collaborator

Yamakaky commented Nov 30, 2016

I know how to do a match, I just want to know why you would do that. Aren't kind() and backtrace() sufficient?

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Nov 30, 2016

Contributor

What's the advantage of reverting to Error as a tuple? I only see the disadvantage that the macro code is less readable... Also, you talk about matching, but I can't see a use-case where you couldn't use the defined methods.

I added some docs on this subject, containing examples.

Here's one:

match Error::from("error!") {
    Error(ErrorKind::InvalidToolchainName(_), _) => { }
    Error(ErrorKind::Msg(_), _) => { }
}

This is a compact syntax. With fields it would look more like:

match Error::from("error!") {
    Error { kind: ErrorKind::InvalidToolchainName(_), _ } => { }
    Error { kind: ErrorKind::Msg(_), _ } => { }
}

Which, while not significantly worse, is more characters and I find to be uglier. I admit that the tuple vs. struct change is one of personal preference, and not necessary for correctness.

rustup does do matches like this.

Contributor

brson commented Nov 30, 2016

What's the advantage of reverting to Error as a tuple? I only see the disadvantage that the macro code is less readable... Also, you talk about matching, but I can't see a use-case where you couldn't use the defined methods.

I added some docs on this subject, containing examples.

Here's one:

match Error::from("error!") {
    Error(ErrorKind::InvalidToolchainName(_), _) => { }
    Error(ErrorKind::Msg(_), _) => { }
}

This is a compact syntax. With fields it would look more like:

match Error::from("error!") {
    Error { kind: ErrorKind::InvalidToolchainName(_), _ } => { }
    Error { kind: ErrorKind::Msg(_), _ } => { }
}

Which, while not significantly worse, is more characters and I find to be uglier. I admit that the tuple vs. struct change is one of personal preference, and not necessary for correctness.

rustup does do matches like this.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Nov 30, 2016

Contributor

One other thing to consider with the structure layout is that the chaining protocol depends on the layout of the chained error structures - they pull chained error types apart during the linking. What this should mean is that, as long as the Error type layout doesn't chain, multiple revisions of error-chain should be compatible with each other. (hm, didn't explain this well. in a mtg, sorry).

Contributor

brson commented Nov 30, 2016

One other thing to consider with the structure layout is that the chaining protocol depends on the layout of the chained error structures - they pull chained error types apart during the linking. What this should mean is that, as long as the Error type layout doesn't chain, multiple revisions of error-chain should be compatible with each other. (hm, didn't explain this well. in a mtg, sorry).

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Nov 30, 2016

Collaborator
match Error::from("error!").kind() {
    ErrorKind::InvalidToolchainName(_) => {}
}

Even shorter.

Collaborator

Yamakaky commented Nov 30, 2016

match Error::from("error!").kind() {
    ErrorKind::InvalidToolchainName(_) => {}
}

Even shorter.

@colin-kiegel

This comment has been minimized.

Show comment
Hide comment
@colin-kiegel

colin-kiegel Nov 30, 2016

I was just about to post that, too. :-)

Another argument would be that I've encountered error structs in the standard library which also have named fields, like here: https://doc.rust-lang.org/src/std/up/src/libstd/io/error.rs.html#69-72

And I always try to mimic the style I find there.

colin-kiegel commented Nov 30, 2016

I was just about to post that, too. :-)

Another argument would be that I've encountered error structs in the standard library which also have named fields, like here: https://doc.rust-lang.org/src/std/up/src/libstd/io/error.rs.html#69-72

And I always try to mimic the style I find there.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Nov 30, 2016

Contributor

Even shorter.

Yeah, in this case it is. This only works when you know you have an error. If you have to match through the Result though then you have to match on the kind field.

Contributor

brson commented Nov 30, 2016

Even shorter.

Yeah, in this case it is. This only works when you know you have an error. If you have to match through the Result though then you have to match on the kind field.

@Arnavion

This comment has been minimized.

Show comment
Hide comment
@Arnavion

Arnavion Nov 30, 2016

Yes, agree with @brson. Match on the error instead of the .kind() is also needed for things like this:

match some_result {
    Ok(file) => file, // Found a matching file
    Err(Error(ErrorKind::FileNotFound, _)) => continue, // Try next file
    Err(err) => panic!(err), // Unexpected error
}

That is, cases when you want to deal with some error kinds specially while still referencing the whole err. With struct-like kind this becomes longer Err(Error { kind: ErrorKind::SomeKind, .. }).

The errorkind is the first member of the tuple struct, so field ordering shouldn't be a problem. And we will soon get .. for tuple patterns so number of members after the kind will also become ignorable.

Arnavion commented Nov 30, 2016

Yes, agree with @brson. Match on the error instead of the .kind() is also needed for things like this:

match some_result {
    Ok(file) => file, // Found a matching file
    Err(Error(ErrorKind::FileNotFound, _)) => continue, // Try next file
    Err(err) => panic!(err), // Unexpected error
}

That is, cases when you want to deal with some error kinds specially while still referencing the whole err. With struct-like kind this becomes longer Err(Error { kind: ErrorKind::SomeKind, .. }).

The errorkind is the first member of the tuple struct, so field ordering shouldn't be a problem. And we will soon get .. for tuple patterns so number of members after the kind will also become ignorable.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Nov 30, 2016

Contributor

Anyway, I'm not closed to debate on tuples vs. structs here, but rustup relies on matching like this, as of now I still like the tuple matching better, and today I just wanted to get error-chain back in a state I'm happy with. I'm happy to revisit. Sorry for opening and pushing this through so quickly.

Contributor

brson commented Nov 30, 2016

Anyway, I'm not closed to debate on tuples vs. structs here, but rustup relies on matching like this, as of now I still like the tuple matching better, and today I just wanted to get error-chain back in a state I'm happy with. I'm happy to revisit. Sorry for opening and pushing this through so quickly.

@Yamakaky

This comment has been minimized.

Show comment
Hide comment
@Yamakaky

Yamakaky Dec 1, 2016

Collaborator

And for the specification of ErrorKind in links?

Collaborator

Yamakaky commented Dec 1, 2016

And for the specification of ErrorKind in links?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment