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

Use Error::source not Error::cause in Rust versions that have it #255

Open
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@palfrey
Copy link

palfrey commented Feb 12, 2019

This should fix #254...

@chachi

This comment has been minimized.

Copy link

chachi commented Feb 19, 2019

@Yamakaky Any chance this could be merged and the version updated? (Apologies if you're not the one to ping here)

palfrey added some commits Feb 19, 2019

@palfrey

This comment has been minimized.

Copy link
Author

palfrey commented Mar 2, 2019

@alexcrichton Any thoughts on this one?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Mar 4, 2019

Sorry but I don't maintain this crate

#[cfg(has_error_source)]
fn finds_source() {
let chained_error = try_foreign_error().err().unwrap();
assert!(::std::error::Error::source(&chained_error).is_none());

This comment has been minimized.

@hcpl

hcpl Mar 5, 2019

Isn't Error::source supposed to work exactly like Error::cause except with downcasting? Which means the assert here should be the same as the assert for finds_cause modulo the method names.

This comment has been minimized.

@palfrey

palfrey Mar 9, 2019

Author

That would be what I thought as well, but it doesn't. I've tried experimenting around with this, and can't think what's missing/wrong here (guessed ForeignError was missing something, but adding a source method to it didn't help)

This comment has been minimized.

@hcpl

hcpl Mar 9, 2019

So adding a source implementation here didn't work?

impl ::std::error::Error for $error_name {
fn description(&self) -> &str {
self.description()
}
#[allow(unknown_lints, renamed_and_removed_lints, unused_doc_comment, unused_doc_comments)]
fn cause(&self) -> Option<&::std::error::Error> {
match self.1.next_error {
Some(ref c) => Some(&**c),
None => {
match self.0 {
$(
$(#[$meta_foreign_links])*
$error_kind_name::$foreign_link_variant(ref foreign_err) => {
foreign_err.cause()
}
) *
_ => None
}
}
}
}
}

I think it should be exactly the same as the body of cause method.

This comment has been minimized.

@palfrey

palfrey Mar 10, 2019

Author

Ah, nice! I'd only added in the tests, but adding it both here and in the tests works and I've been able to get rid of a bunch of the special cases. Thanks! Good catch!

() => assert_eq!(format!("{}", ForeignErrorCause {}),
format!("{}", error_iter.next().unwrap())),
#[cfg(has_error_source)]
() => assert!(error_iter.next().is_none()),

This comment has been minimized.

@hcpl

hcpl Mar 5, 2019

Same as above with finds_source test.

This comment has been minimized.

@palfrey

palfrey Mar 9, 2019

Author

Fixed

This comment has been minimized.

@palfrey

palfrey Mar 9, 2019

Author

Doing this change broke 1.10 support, so reverting back to my original changes.

This comment has been minimized.

@hcpl

hcpl Mar 9, 2019

My bad, attributes on statements is a 1.13.0 feature, should have checked minimal supported Rust first.

And just in case I wasn't clear, here I was referring to the assert for #[cfg(has_error_source)] being functionally equal to its #[cfg(not(has_error_source))] counterpart above.

Cargo.toml Outdated
@@ -21,6 +21,10 @@ travis-ci = { repository = "rust-lang-nursery/error-chain" }
[features]
default = ["backtrace", "example_generated"]
example_generated = []
has_error_source = [] # Determined by build.rs

This comment has been minimized.

@hcpl

hcpl Mar 5, 2019

You don't need a Cargo feature for #[cfg(has_error_source)] to work. It's only needed when used as #[cfg(feature = "has_error_source")].

This comment has been minimized.

@palfrey

palfrey Mar 9, 2019

Author

Fixed

build.rs Outdated
if (is_nightly().unwrap_or(false)
&& is_min_date("2018-09-02")
.unwrap_or((false, "".to_string())).0)
|| is_min_version("1.30").unwrap_or((false, "".to_string())).0

This comment has been minimized.

@hcpl

hcpl Mar 5, 2019

I'd omit the check for old nightlies. Not because your code wouldn't work on nightlies since 2018-09-02 (I'm pretty sure it will, with it being stable), but because explicit support for both old and new nightlies is not feasible in general:

  • new nightlies often break old code, so supporting both means supporting mutually incompatible API and that's a massive pain;
  • even if the feature in question is stable since then, builds might still fail for other reasons, if the library decides to use nightly features at some point (this is for future-proofing);

So I'd rather drop support for old nightlies than give false hope to users that expect error-chain to work on their outdated nightly. Nightlies are supposed to be updated "every night", after all.

That said I'm fine if you still want to keep supporting old nightlies. I'll only rely on stable and latest nightly to build error-chain which is a subset of what is planned to support here.

Also i'd rewrite

is_min_version("1.30").unwrap_or((false, "".to_string())).0

as

is_min_version("1.30").map(|(b, )| b).unwrap_or(false)

because it conveys the "I only need the boolean" intent more clearly.

This comment has been minimized.

@palfrey

palfrey Mar 9, 2019

Author

Fixed

@@ -188,7 +188,12 @@ macro_rules! impl_error_chain_processed {
$(
$(#[$meta_foreign_links])*
$error_kind_name::$foreign_link_variant(ref foreign_err) => {
foreign_err.cause()
match () {

This comment has been minimized.

@hcpl

hcpl Mar 5, 2019

match here is unnecessary, can be replaced with

                                    #[cfg(not(has_error_source))]
                                    {
                                        foreign_err.cause()
                                    }
                                    #[cfg(has_error_source)]
                                    {
                                        foreign_err.source()
                                    }
@@ -570,7 +570,12 @@ impl<'a> Iterator for Iter<'a> {
fn next<'b>(&'b mut self) -> Option<&'a error::Error> {
match self.0.take() {
Some(e) => {
self.0 = e.cause();
self.0 = match () {

This comment has been minimized.

@hcpl

hcpl Mar 5, 2019

Same as above

                self.0 = {
                    #[cfg(not(has_error_source))]
                    {
                        e.cause()
                    }
                    #[cfg(has_error_source)]
                    {
                        e.source()
                    }
                };
@palfrey

This comment has been minimized.

Copy link
Author

palfrey commented Mar 12, 2019

@brson Can you help here?

hcpl added a commit to hcpl/serde_mtproto that referenced this pull request Mar 16, 2019

Fix builds on Rust 1.33
Fixes 2 errors:

* Integer patterns can now be exhaustive which makes
  `_ => unreachable!(...)` fail the `unreachable_patterns` warning.
  The builds fail because the warning is set as `deny` for this crate
  (we should drop this practice altogether and only deny warnings for
  CI).
* Temporarily allow the deprecated since 1.33 `Error::cause()` method
  until the issue is fixed in `error-chain` upstream in
  <rust-lang-nursery/error-chain#255>.
  This should also only warn, but we set it as deny.
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.