Implement `std::error::Error` for all error types #419

davidpdrsn · 2019-08-23T14:49:21Z

Fixes #415

I'm having some issues with FieldError. I end up hitting a conflicting implementation error when I add

impl<S> std::error::Error for FieldError<S> {}

impl<S> fmt::Display for FieldError<S> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        unimplemented!()
    }
}

I get this error:

error[E0119]: conflicting implementations of trait `std::convert::From<executor::FieldError<_>>` for type `executor::FieldError<_>`:
   --> juniper/src/executor/mod.rs:142:1
    |
142 | / impl<T: Display, S> From<T> for FieldError<S>
143 | | where
144 | |     S: crate::value::ScalarValue,
145 | | {
...   |
151 | |     }
152 | | }
    | |_^
    |
    = note: conflicting implementation in crate `core`:
            - impl<T> std::convert::From<T> for T;

I'm not quite sure what is causing that.

LegNeato

Thanks for the PR! Not sure why you are getting that error.

juniper/src/lib.rs

theduke · 2019-08-27T07:57:54Z

I'm not quite sure what is causing that.

Since FieldError has a From<T: Display> impl, if it implements Display itself the compiler can't resolve that cyclical relationship.

Not sure what's the best way around that, I'll have a look at the code.

LegNeato · 2019-08-28T05:24:20Z

From https://www.reddit.com/r/rust/comments/9amj6w/hey_rustaceans_got_an_easy_question_ask_here/e59n3n5/:

When T = U, you'd have two applicable implementations of the trait, and Rust won't allow that.
Best you can probably do is just provide a cast method or somesuch. Or, if you're really set on using From, you could use an intermediate type

theduke · 2019-08-28T06:45:17Z

I never was a big fan of the From T: Display impl to be honest.

We could think about removing it, and only having a From T: Error impl.
We can still have a manual constructor like FieldError::from_str.

davidpdrsn · 2019-08-28T07:13:29Z

@theduke I like that. Since thats a breaking change do we have to go through a round of deprecation or is it fine?

theduke · 2019-10-21T15:33:54Z

Nah let's batch it together with the other changes related to async/await and removing the old macros.

What's the status here, did you implement the FieldError?

davidpdrsn · 2019-10-21T21:02:01Z

I had totally forgotten about this. I'll give it another look soon.

davidpdrsn · 2019-10-31T20:19:58Z

Sorry for the delay but this should be ready for next review.

I had to make a few changes to get impl std::error::Error for FieldError working. They're outlined in the commit message.

This is required for implementing `Display` for `FieldError`

This required removing `impl From<T> for FieldError where T: Display` because it would otherwise cause a conflicting implementation. That is because `impl From<T> for T` already exists. Instead I added `impl From<String> for FieldError` and `impl From<&str> for FieldError` which should cover most use cases of the previous `impl`. I also added `FieldError::from_error` so users can convert from any error they may have.

codecov-io · 2019-10-31T20:30:12Z

Codecov Report

Merging #419 into master will decrease coverage by <.01%.
The diff coverage is 75.25%.

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
- Coverage   85.74%   85.74%   -0.01%     
==========================================
  Files         110      111       +1     
  Lines       15933    15982      +49     
==========================================
+ Hits        13661    13703      +42     
- Misses       2272     2279       +7

Impacted Files	Coverage Δ
juniper/src/parser/parser.rs	`78.26% <ø> (ø)`	⬆️
juniper/src/executor/mod.rs	`91.64% <ø> (+0.1%)`	⬆️
juniper/src/parser/lexer.rs	`90.09% <ø> (ø)`	⬆️
juniper/src/validation/context.rs	`84.78% <0%> (-5.92%)`	⬇️
juniper/src/lib.rs	`61.36% <0%> (-22.51%)`	⬇️
juniper/src/parser/utils.rs	`90% <0%> (-7.3%)`	⬇️
juniper/src/value/mod.rs	`91.35% <100%> (+5.76%)`	⬆️
juniper/src/integrations/serde.rs	`57.35% <0%> (-2.21%)`	⬇️
juniper/src/validation/input_value.rs	`83.75% <0%> (-0.83%)`	⬇️
juniper_warp/src/lib.rs	`93.62% <0%> (-0.5%)`	⬇️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91a9752...b7eb36a. Read the comment docs.

theduke · 2019-10-31T22:07:43Z

This seems to be missing a From<E: std::error::Error> impl right now?

We need that more than a the From<String/&str> / variants.

davidpdrsn · 2019-11-01T08:20:03Z

I did try adding From<E: std::error::Error> but unfortunately it gives the same error:

error[E0119]: conflicting implementations of trait `std::convert::From<executor::FieldError<_>>` for type `executor::FieldError<_>`:
   --> juniper/src/executor/mod.rs:182:1
    |
182 | / impl<S, E> From<E> for FieldError<S>
183 | | where
184 | |     S: crate::value::ScalarValue,
185 | |     E: std::error::Error,
...   |
189 | |     }
190 | | }
    | |_^
    |
    = note: conflicting implementation in crate `core`:
            - impl<T> std::convert::From<T> for T;

It seems that you cannot both implement Display and From<T> where T: Error.

use std::fmt::{Formatter, self, Display};

#[derive(Debug)]
struct MyError(String);

impl std::error::Error for MyError {}

impl Display for MyError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        unimplemented!()
    }
}

impl<T: std::error::Error> From<T> for MyError {
    fn from(s: T) -> Self {
        unimplemented!()
    }
}

That fails to compile with the same error.

That is why I added

pub fn from_error<E>(error: E) -> FieldError<S>
where
    E: std::error::Error,
{
    FieldError::from(error.to_string())
}

Was the only working solution I could think of.

davidpdrsn · 2019-11-01T17:31:26Z

Does it make sense to not implement std::error::Error for FieldError because of this? I’m thinking it’s pretty inconvenient 🤔

theduke · 2019-11-01T17:42:58Z

Ah yeah I forgot about the coherence rules. You cant both implement Error and From<Error> without specialization...

Yeah let's just drop the impl on FieldError. It doesn't really matter anyway since it is not a type users work with directly.

davidpdrsn · 2019-11-01T17:45:23Z

Alright cool. I’ll revert it tomorrow.

Does it make sense to keep the Display impl for Value?

We cannot have this and `impl<S> std::error::Error for FieldError<S>` so we agreed this is more valuable. More context graphql-rust#419

davidpdrsn · 2019-11-05T08:24:16Z

I have brought back impl<T: Display, S> From<T> for FieldError<S> now.

Guess this should be good now 😊

theduke · 2019-11-07T02:46:43Z

I have some additional changes. Will push tomorrow.

davidpdrsn · 2019-11-17T10:43:43Z

@theduke Anything I can help out with?

jkoudys · 2020-01-05T20:08:36Z

@davidpdrsn Is there anything more to go in on this PR? juniper is the last holdout in my app before I can simply Box<dyn Error> then use ? like mad everywhere. The open change request looks like a pretty trivial message change.

juniper/src/lib.rs

davidpdrsn · 2020-01-06T08:15:42Z

@jkoudys No it should be good to go. I wonder which error you're having to convert into Box<dyn Error>? I haven't run into such issues with the juniper version that is out now.

Also note that as per the discussions above you wont be able to convertFieldError into Box<dyn Error>. That shouldn't be an issue in practice however.

jkoudys · 2020-01-07T04:43:47Z

@jkoudys No it should be good to go. I wonder which error you're having to convert into Box<dyn Error>? I haven't run into such issues with the juniper version that is out now.

Trying to into a Box<dyn Error> using the ?, but erroring on ^ the trait std::error::Erroris not implemented forjuniper::GraphQLError<'_>`` because GraphQLError doesn't impl Error. I'm on the async-await branch, so that may need to be rebased.

davidpdrsn · 2020-01-10T08:18:50Z

@theduke @LegNeato This has been done for quite some time as you can see someone is asking for it 😊 Do you have any last minute things I should correct before merging? I might also try to get it merged into the async/await branch afterwards.

andy128k · 2020-01-30T19:45:27Z

I think proper implementation of std::error::Error should include implementation of source method too.

jkoudys · 2020-02-11T20:03:52Z

@andy128k you mean like on the GraphQLError itself, to give a simple mapping back to the original error type? e.g.

impl<'a> std::error::Error for GraphQLError<'a> {
    fn source(&self) -> Option<&(dyn Error + 'a)> {
        Some(&self)
    }
}

Can't think what else we'd need, unless you mean sources on e.g. ParserError could return a LexerError or something.
At any rate, does this really need to go in before accepting the PR? It's been done but sitting in limbo for such a long time. Juniper is the one holdout we have that's keeping us from having an easy time managing only Errors that impl std:error::Error.

It's been 4 months....

davidpdrsn · 2020-02-11T23:35:27Z

This PR has been blocked by all async-flux. That seems to have settled now so I do hope to get it merged soon.

davidpdrsn · 2020-02-19T10:11:16Z

@LegNeato master still doesn't build due to some async stuff it seems. Should I merge this into the async branch instead?

LegNeato · 2020-02-19T22:41:10Z

Everything on master except the book builds / passes?

davidpdrsn · 2020-02-19T23:00:13Z

If I clone a fresh copy of the repo and run cargo test on nightly I get a bunch of errors like these:

error[E0432]: unresolved import `futures`
  --> integration_tests/juniper_tests/src/codegen/derive_enum.rs:10:10
   |
10 | #[derive(juniper::GraphQLEnum, Debug, PartialEq)]
   |          ^^^^^^^^^^^^^^^^^^^^ use of undeclared type or module `futures`

error[E0432]: unresolved import `futures`
  --> integration_tests/juniper_tests/src/codegen/derive_enum.rs:19:10
   |
19 | #[derive(juniper::GraphQLEnum)]
   |          ^^^^^^^^^^^^^^^^^^^^ use of undeclared type or module `futures`

error[E0432]: unresolved import `futures`
  --> integration_tests/juniper_tests/src/codegen/derive_enum.rs:29:10
   |
29 | #[derive(juniper::GraphQLEnum, Debug, PartialEq)]
   |          ^^^^^^^^^^^^^^^^^^^^ use of undeclared type or module `futures`

error[E0432]: unresolved import `futures`
  --> integration_tests/juniper_tests/src/codegen/derive_enum.rs:37:10
   |
37 | #[derive(juniper::GraphQLEnum, Debug, PartialEq)]
   |          ^^^^^^^^^^^^^^^^^^^^ use of undeclared type or module `futures`

error[E0432]: unresolved import `futures`
  --> integration_tests/juniper_tests/src/codegen/derive_object.rs:13:10
   |
13 | #[derive(GraphQLObject, Debug, PartialEq)]
   |          ^^^^^^^^^^^^^ use of undeclared type or module `futures`

error[E0432]: unresolved import `futures`
  --> integration_tests/juniper_tests/src/codegen/derive_object.rs:29:10
   |
29 | #[derive(GraphQLObject, Debug, PartialEq)]
   |          ^^^^^^^^^^^^^ use of undeclared type or module `futures`

error[E0432]: unresolved import `futures`
  --> integration_tests/juniper_tests/src/codegen/derive_object.rs:38:10

LegNeato · 2020-02-21T05:34:02Z

@davidpdrsn yeah weirdly just doing cargo test doesn't work for integration tests. Haven't looked into it yet. In any case, you can see the non-Github CI is all passing (Github CI failures are #531).

davidpdrsn · 2020-02-21T15:11:39Z

Thanks for merging!

LegNeato requested changes Aug 27, 2019

View reviewed changes

juniper/src/lib.rs Outdated Show resolved Hide resolved

davidpdrsn added 2 commits October 31, 2019 21:13

Implement std::error::Error for all error types

0e49403

Fix copy-paste

5d6b87c

davidpdrsn force-pushed the master branch from c950d74 to 4385539 Compare October 31, 2019 20:18

davidpdrsn added 3 commits October 31, 2019 21:20

Implement Display for Value

bec1f99

This is required for implementing `Display` for `FieldError`

Update changelog

30e9c00

davidpdrsn force-pushed the master branch from 4385539 to 30e9c00 Compare October 31, 2019 20:20

Format

0bc0523

Bring back impl<T: Display, S> From<T> for FieldError<S>

5e3d146

We cannot have this and `impl<S> std::error::Error for FieldError<S>` so we agreed this is more valuable. More context graphql-rust#419

davidpdrsn mentioned this pull request Nov 30, 2019

Remove the From<Dispaly> trait for FieldError #469

Closed

MikailBag reviewed Jan 5, 2020

View reviewed changes

juniper/src/lib.rs Outdated Show resolved Hide resolved

Write errors without allocations

b7eb36a

davidpdrsn requested review from LegNeato and theduke January 9, 2020 22:21

LegNeato previously approved these changes Jan 21, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master'

8453620

davidpdrsn dismissed LegNeato’s stale review via 8453620 January 25, 2020 13:09

LegNeato merged commit ca28e90 into graphql-rust:master Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `std::error::Error` for all error types #419

Implement `std::error::Error` for all error types #419

davidpdrsn commented Aug 23, 2019 •

edited

Loading

LegNeato left a comment

theduke commented Aug 27, 2019

LegNeato commented Aug 28, 2019

theduke commented Aug 28, 2019

davidpdrsn commented Aug 28, 2019

theduke commented Oct 21, 2019

davidpdrsn commented Oct 21, 2019

davidpdrsn commented Oct 31, 2019 •

edited

Loading

codecov-io commented Oct 31, 2019 •

edited

Loading

theduke commented Oct 31, 2019

davidpdrsn commented Nov 1, 2019

davidpdrsn commented Nov 1, 2019

theduke commented Nov 1, 2019

davidpdrsn commented Nov 1, 2019

davidpdrsn commented Nov 5, 2019

theduke commented Nov 7, 2019

davidpdrsn commented Nov 17, 2019

jkoudys commented Jan 5, 2020

davidpdrsn commented Jan 6, 2020

jkoudys commented Jan 7, 2020

davidpdrsn commented Jan 10, 2020

andy128k commented Jan 30, 2020

jkoudys commented Feb 11, 2020

davidpdrsn commented Feb 11, 2020

davidpdrsn commented Feb 19, 2020

LegNeato commented Feb 19, 2020

davidpdrsn commented Feb 19, 2020

LegNeato commented Feb 21, 2020 •

edited

Loading

davidpdrsn commented Feb 21, 2020

Implement std::error::Error for all error types #419

Implement std::error::Error for all error types #419

Conversation

davidpdrsn commented Aug 23, 2019 • edited Loading

LegNeato left a comment

Choose a reason for hiding this comment

theduke commented Aug 27, 2019

LegNeato commented Aug 28, 2019

theduke commented Aug 28, 2019

davidpdrsn commented Aug 28, 2019

theduke commented Oct 21, 2019

davidpdrsn commented Oct 21, 2019

davidpdrsn commented Oct 31, 2019 • edited Loading

codecov-io commented Oct 31, 2019 • edited Loading

Codecov Report

theduke commented Oct 31, 2019

davidpdrsn commented Nov 1, 2019

davidpdrsn commented Nov 1, 2019

theduke commented Nov 1, 2019

davidpdrsn commented Nov 1, 2019

davidpdrsn commented Nov 5, 2019

theduke commented Nov 7, 2019

davidpdrsn commented Nov 17, 2019

jkoudys commented Jan 5, 2020

davidpdrsn commented Jan 6, 2020

jkoudys commented Jan 7, 2020

davidpdrsn commented Jan 10, 2020

andy128k commented Jan 30, 2020

jkoudys commented Feb 11, 2020

davidpdrsn commented Feb 11, 2020

davidpdrsn commented Feb 19, 2020

LegNeato commented Feb 19, 2020

davidpdrsn commented Feb 19, 2020

LegNeato commented Feb 21, 2020 • edited Loading

davidpdrsn commented Feb 21, 2020

Implement `std::error::Error` for all error types #419

Implement `std::error::Error` for all error types #419

davidpdrsn commented Aug 23, 2019 •

edited

Loading

davidpdrsn commented Oct 31, 2019 •

edited

Loading

codecov-io commented Oct 31, 2019 •

edited

Loading

LegNeato commented Feb 21, 2020 •

edited

Loading