PowerShell / PowerShell/DSC

Standardize and refactor error design

Open
#1,694 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dev-UX Needs Triage
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.

  1. Initially, we defined every error in the DscError enum directly with every error variant defined as a tuple, like:

    https://github.com/PowerShell/DSC/blob/861df432f224d606ba30f4a39f49e40c3ef2b8a2/lib/dsc-lib/src/dscerror.rs#L20-L21

    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.

  2. 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, SemanticVersionReqError is defined in types::semantic_version_req and is defined in DscError like so:

    https://github.com/PowerShell/DSC/blob/861df432f224d606ba30f4a39f49e40c3ef2b8a2/lib/dsc-lib/src/dscerror.rs#L161-L162

    This allows us to define context-specific errors in the context where those errors are raised and automatically convert them into instances of DscError as needed (and automatically when using the ? error-return semantics).

  3. As part of the process and in recognition of the eventual need for more readable errors with useful context, we added the miette crate to provide diagnostics. We're not currently emitting that data but the wiring for error definitions is in place.

  4. 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:

    https://github.com/PowerShell/DSC/blob/861df432f224d606ba30f4a39f49e40c3ef2b8a2/lib/dsc-lib/src/types/semantic_version_req.rs#L539-L556

    Additionally, this made it more convenient to use the thiserror and miette attributes on the errors.

Proposals

New errors

I propose the following conventions for new errors:

  1. Define the error as close to the context it's raised from as possible. For example, if you're defining a new type like DscSettings and need errors for that type, define a new enum that derives Error and Diagnostic named DscSettingsError.

  2. 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 String we 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).

  3. 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-circuit return statements) or error collection. Doing both is generally an anti-pattern, though early return for Result<Option<T>, E> when you want to return Ok(None) is okay.

  4. 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),
    
  5. 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,
    },
    
  6. 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 errors field 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 CollectibleError for more information about the to_collected_string method used in this snippet.

  7. 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:

  1. 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.

  2. Look for errors that are specific to a module or type. Extract the errors from DscError into a new type in the appropriate module. Replace the errors in DscError with a transparent passthrough variant.

    For example, many of the Command* error variants are primarily or exclusively raised from dscresource::command_resource - these are a strong candidate for extraction.

  3. 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 DscError variant to either a transparent passthrough or a wrapped passthrough.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.