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 Provider API for context information on errors (and possibly other things) #133

Merged
merged 9 commits into from
Jan 3, 2022

Conversation

TimDiekmann
Copy link
Member

@TimDiekmann TimDiekmann commented Jan 3, 2022

🌟 What is the purpose of this PR?

Contains the Provider trait and accompanying API, which enable trait objects to provide data based on typed requests, an alternate form of runtime reflection.

Currently, we use thiserror in hEngine, which just wraps every error in another error. Errors may (or may not) contain additional information on the cause or the context. It's very hard to track down the exact cause with all it's information (without huge, nested match error statements). It's even harder to add more general information like the Location, where the error/cause/context happened. We want to replace our error handling with a more generic approach like shown in #134.

The provider API was proposed by the official error handling project of Rust (originally to move Error into core). This PR effectively reimplements the proposed Provider, so we can use it until/if it's merged into the language, I only added a few tests (which should have a code coverage of 100%).

🔗 Related links

🔍 What does this change?

Provider and the associated APIs support generic, type-driven access to data, and a mechanism
for implementers to provide such data. The key parts of the interface are the Provider trait
for objects which can provide data, and the request_by_type_tag function for data from an
object which implements Provider. Note that end users should not call requesting
request_by_type_tag directly, it is a helper function for intermediate implementers to use to
implement a user-facing interface.

Typically, a data provider is a trait object of a trait which extends Provider. A user will
request data from the trait object by specifying the type or a type tag (a type tag is a type
used only as a type parameter to identify the type which the user wants to receive).

We pointed out (at least) the following points for our error handling:

  1. provide context information
  2. play well with async
  3. has parsable errors to be used for logging
  4. needs to be human readable

Looking at the dataflow of the provider API:

  • A user requests an object, which is delegated to request_by_type_tag
  • request_by_type_tag creates a Requisition object and passes it to Provider::provide
  • The object provider's implementation of Provider::provide tries providing values of
    different types using Requisition::provide_*. If the type tag matches the type requested by
    the user, it will be stored in the Requisition object.
  • request_by_type_tag unpacks the Requisition object and returns any stored value to the
    user.

All these steps happen in Error handling, not in Error generation, i.e. everything is independent of threads/async etc. (2. ✅). Obviously context information is passed. This can be everything from an error message, over backtraces or spantraces up to file/line/column, where the error occurred (1. ✅). Since all information can be encoded, the error handler can do what it wants with it. E.g. put it into a nice logging entry (3. ✅) or output it nicely to the console (4. ✅).

🛡 Tests

  • ✅ Unit Tests (100% code coverage)
  • ✅ Miri
  • ✅ Manual Tests

@github-actions github-actions bot added A-engine area/deps Relates to third-party dependencies (area) labels Jan 3, 2022
@TimDiekmann TimDiekmann removed the area/deps Relates to third-party dependencies (area) label Jan 3, 2022
@github-actions github-actions bot added the area/deps Relates to third-party dependencies (area) label Jan 3, 2022
@TimDiekmann TimDiekmann marked this pull request as ready for review January 3, 2022 14:43
@TimDiekmann TimDiekmann force-pushed the td/provider-api branch 2 times, most recently from fbeab64 to 5b10430 Compare January 3, 2022 14:44
…her things)

Add tests to provider


Fix newline on Cargo.toml
@@ -110,6 +110,8 @@
//
// TODO: replace library with https://github.com/rust-lang/project-error-handling/issues/3.

#![warn(clippy::pedantic, clippy::nursery)]
Copy link
Member

Choose a reason for hiding this comment

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

+1 for these as warnings. I don't think we should ever block on them, but it's good to get the visibility (provided folks are able to make sensible decisions about what to address/when, and can avoid the distraction of their likely omnipresence).

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually we block on any warning currently which is strictly not backwards compatible:

cargo clippy --manifest-path ${{ matrix.directory }}/Cargo.toml --all --all-features -- -D warnings

I have written this crate taking into account those warnings. Adding those to our current engine would be just noise. We even disabled some of the most annoying clippy lints, but as we proceed, we want to enable them again:

#![allow(
clippy::borrowed_box,
clippy::wrong_self_convention,
clippy::diverging_sub_expression,
clippy::expect_fun_call,
clippy::large_enum_variant,
clippy::type_complexity,
clippy::module_inception,
clippy::new_ret_no_self,
clippy::unnecessary_cast,
clippy::enum_variant_names,
clippy::should_implement_trait,
clippy::mutex_atomic
)]

packages/engine/lib/provider/src/internal.rs Show resolved Hide resolved
packages/engine/lib/provider/src/lib.rs Outdated Show resolved Hide resolved
packages/engine/lib/provider/src/internal.rs Outdated Show resolved Hide resolved
packages/engine/lib/provider/src/requisition.rs Outdated Show resolved Hide resolved
packages/engine/lib/provider/src/tags.rs Outdated Show resolved Hide resolved
@TimDiekmann
Copy link
Member Author

Thanks for the very deep review!

@TimDiekmann TimDiekmann merged commit b4b9520 into main Jan 3, 2022
@TimDiekmann TimDiekmann deleted the td/provider-api branch January 3, 2022 17:37
@vilkinsons vilkinsons added the category/enhancement New feature or request label Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deps Relates to third-party dependencies (area) category/enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants