Standardize and refactor error design
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 523
- Forks
- 75
- Avg merge
- 3d 16h
- Merged PRs (30d)
- 24
Description
Currently, the design for errors in dsc-lib represents an evolving understanding of how to define and handle errors.
-
Initially, we defined every error in the
DscErrorenum directly with every error variant defined as a tuple, like:The primary drawback to the tuple design is that it requires a contributor to know exactly what the field is meant to represent and carefully construct the error type as needed. This is made more difficult by the sparse documentation on the error variants.
-
As we worked through the type definition work, we colocated type-specific errors with those types and added a passthrough error to
DscError. For example,SemanticVersionReqErroris defined intypes::semantic_version_reqand is defined inDscErrorlike so:This allows us to define context-specific errors in the context where those errors are raised and automatically convert them into instances of
DscErroras needed (and automatically when using the?error-return semantics). -
As part of the process and in recognition of the eventual need for more readable errors with useful context, we added the
miettecrate to provide diagnostics. We're not currently emitting that data but the wiring for error definitions is in place. -
As we defined new errors for specific contexts, we also began defining the error variants as structs with named fields. This makes it easier to construct the errors and enables documenting the errors more effectively. For example:
Additionally, this made it more convenient to use the
thiserrorandmietteattributes on the errors.
Proposals
New errors
I propose the following conventions for new errors:
-
Define the error as close to the context it's raised from as possible. For example, if you're defining a new type like
DscSettingsand need errors for that type, define a new enum that derivesErrorandDiagnosticnamedDscSettingsError. -
Prefer defining new error variants instead of passing a translation string back to a higher-order variant. When we collapse what are effectively error variants into a single type where the inner value is a
Stringwe can't do useful per-variant handling or reporting.For example, in the current implementation we squash all kinds of parse errors into
DscError::Parser(String)where the inner value is the translated string. We lose all of the (programmatic) context, making it effectively impossible to distinguish between different parse errors or provide better diagnostics to users and integrating developers.Remember that we can wrap these more contextual errors in a higher-order error variant either transparently (passthrough exactly as defined) or with additional context (add more information or a message prefix).
-
When implementing code where it is coherent to collect errors, do so. Use the pattern:
fn might_have_multiple_errors(input: &str) -> Result<ReturnValue, ErrorType> { let mut errors: Vec<ErrorType> = vec![]; // If some problem arises, insert to error collection if some_problem { errors.push(ErrorType::SomeProblem{input: input.to_string()} } // continue processing, inserting errors as needed if errors.len() == 0 { Ok(return_value) } else { Err(ErrorType::CollectedErrorVariant{ input: input.to_string(), errors }) } }This pattern creates a mutable vector of errors that you add to when stepping through whatever the function needs to do. Return at the end with either the valid value or a wrapping error that keeps the raised errors for context.
You need to choose between either the early-return pattern (
?on calls that may fail or short-circuitreturnstatements) or error collection. Doing both is generally an anti-pattern, though early return forResult<Option<T>, E>when you want to returnOk(None)is okay. -
When defining a new error variant, define it as a struct with named fields unless it's just a passthrough for another error type and we're adding zero context to that error.
When defining a passthrough error variant, define it like:
#[error(transparent)] ErrorType(#[from]ErrorType), -
When defining an error variant that wraps another error type but provides our own error message, define the variant like:
#[error("{t}", t = t!("lookup.key.path", err = source))] ErrorType{ #[from] source: ErrorType, }, -
When defining an error variant that contains a collection of other errors - such as when defining a top-level parse error that contains every problem with the input string - define an
errorsfield with the#[related]miette attribute like:#[error("{t}", t = t!( "lookup.key.path", "text" => text, "errors" => errors.to_collected_string(", ") ))] ParseFooError { text: String, #[related] errors: Vec<FooError> },See the proposal for
CollectibleErrorfor more information about theto_collected_stringmethod used in this snippet. -
Document any newly defined error types, variants, and their fields. The documentation should be maintainer/library-user facing. We should explain when/why the error is raised and what each field represents.
Collectible error trait
I propose we define an implement a trait like CollectibleError to simplify converting a Vec<ErrorType> into a string we can pass to the display creator. Quick sketch:
pub trait CollectibleError {
fn to_collected_string(&self, separator: &str) -> String;
}
impl<T: std::error::Error> CollectibleError for Vec<T> {
fn to_collected_string(&self, separator: &str) -> String {
self.iter()
.map(|e| e.to_string())
.collect::<Vec<String>>()
.join(separator)
}
}
Which we can leverage for the display string for errors that collect other errors like so:
/// Indicates that the input string couldn't be parsed as an instance of [`Foo`]
/// for one or more reasons.
#[error("{t}", t = t!(
"lookup.key.path",
"text" => text,
"errors" => errors.to_collected_string(", ")
))]
ParseFooError {
/// The input text that failed to parse.
text: String,
/// Collected validation and parse failures that prevented the input text
/// from parsing into an instance of [`Foo`].
#[related]
errors: Vec<FooError>
},
Error refactoring
I propose that we iteratively update the error definitions in DscError by following these steps:
-
Refactor variants to define them as structs with named fields instead of tuples. This will require also updating the code where we construct these errors. We should also write the reference documentation for each error as we refactor from tuple to named fields.
-
Look for errors that are specific to a module or type. Extract the errors from
DscErrorinto a new type in the appropriate module. Replace the errors inDscErrorwith a transparent passthrough variant.For example, many of the
Command*error variants are primarily or exclusively raised fromdscresource::command_resource- these are a strong candidate for extraction. -
Look for errors where we can capture context we're currently squashing into a single variant by passing a translation string as the inner value instead of providing context and specific variants.
Define a new error type to capture the context with new variants and update the
DscErrorvariant to either a transparent passthrough or a wrapped passthrough.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reading lib/dsc-lib/src/dscerror.rs and the context-specific error example in lib/dsc-lib/src/types/semantic_version_req.rs. Then trace the Command* variants raised from dscresource::command_resource and their construction sites. Done requires a scoped refactor that introduces documented named-field or context-specific errors while preserving appropriate passthrough behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- tooling
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100