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

Add RFC for Error Context and Compatibility #1819

Merged
merged 8 commits into from
Oct 12, 2022
Merged

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Oct 6, 2022

This RFC proposes a pattern for writing Rust errors to provide consistent error context and forwards/backwards compatibility.

Rendered

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions
Copy link

github-actions bot commented Oct 6, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Comment on lines +73 to +74
3. Use of enum tuples, even with `#[non_exhaustive]`, adds friction to evolving the API since the
tuple members cannot be named.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is suuuuuuuch a good callout and one that didn't even occur to me

}
}

pub struct VariantC { some_private_field: u32 }
Copy link
Contributor

@Velfi Velfi Oct 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know how you can define associated types and consts in traits? It'd be cool if you could define a struct inside an enum without using a struct enum variant, just to show that they're intentionally linked together forever.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub enum MyError {
    struct A {
        some_private_field: String,
    }
    
    VariantA(A)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be nice.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@DavidSouther DavidSouther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like the philosophical distinction in actionable vs informative error types. From a consumer standpoint, this is a pattern that will allow users of the SDK to marshal to their own types (possibly thiserror).

Copy link
Contributor

@LukeMathWalker LukeMathWalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I love it.
I left a small comment on error chain reporting.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti enabled auto-merge (squash) October 12, 2022 17:46
@jdisanti jdisanti merged commit 7866477 into main Oct 12, 2022
@jdisanti jdisanti deleted the jdisanti-error-rfc branch October 12, 2022 17:53
Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like the direction here. I feel like there are two big pieces missing from the RFC:

  1. This RFC doesn't ever talk about the experience that customers have actually dealing with these errors or ways we can make the errors easy (or hard) for customers to interact with. I'd love to see code examples that showed what it's like to interact with these errors.

My hope is that by digging into this more directly, we'll be able to settle on a key set of utilities (is_foo() maybe?) that will make errors easy for customers to interact with while maintaining all of our backwards compatibility
2. Other than careful code review, I think we need a mechanism to drive consistency here, otherwise I suspect even after a large effort to remove divergence, things will diverge again.

Comment on lines +353 to +355
In code where errors are logged rather than returned to the customer, the full error source chain
must be displayed. This will be made easy by placing a `DisplayErrorContext` struct in `aws-smithy-types` that
is used as a wrapper to get the better error formatting:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing that I wish we were able to better support was retroactively adding context to an existing error—I feel like that's a very common pattern that would improve the errors we're able to give to customers & reduce the need to get debug logs when resolving issues

@jdisanti
Copy link
Collaborator Author

jdisanti commented Dec 8, 2022

This RFC doesn't ever talk about the experience that customers have actually dealing with these errors or ways we can make the errors easy (or hard) for customers to interact with. I'd love to see code examples that showed what it's like to interact with these errors.

Yeah, that wasn't the intent of this RFC, although the into_service_error() function this RFC introduced at least improved ergonomics a bit.

The crate organization RFC was originally tackling some of the ergonomics issues, but I've since moved this into a separate RFC.

Other than careful code review, I think we need a mechanism to drive consistency here, otherwise I suspect even after a large effort to remove divergence, things will diverge again.

Yeah, we could consider implementing a special linter for this. It's hard to say if that additional effort would be worth it or not though, unless another tool makes this a lot easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants