Standardize and refactor error design

[michaeltlombardi]

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:

   DSC/lib/dsc-lib/src/dscerror.rs

   Lines 20 to 21 in 861df43

   	#[error("{t} '{0}' [{t2} {1}] {2}", t = t!("dscerror.commandResource"), t2 = t!("dscerror.exitCode"))]
   	Command(String, i32, String),

   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:

   DSC/lib/dsc-lib/src/dscerror.rs

   Lines 161 to 162 in 861df43

   	#[error(transparent)]
   	SemverReq(#[from] crate::types::SemanticVersionReqError),

   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:

   DSC/lib/dsc-lib/src/types/semantic_version_req.rs

   Lines 539 to 556 in 861df43

   	/// Indicates that a comparator was defined with a wildcard for the major version segment,
   	/// which DSC forbids.
   	///
   	/// [`semver`] supports defining the version for a comparator with the major version segment
   	/// as a wildcard. DSC forbids this construction, since it maps to "match any version," which
   	/// is the default behavior when no version requirement is defined.
   	#[error("{t}", t = t!(
   	"types.semantic_version_req.wildcardMajorVersion",
   	"comparator" => comparator,
   	"wildcard" => wildcard
   	))]
   	ComparatorWithWildcardMajorVersion{
   	/// The input string for the comparator that failed validation during parsing.
   	comparator: String,
   	/// The wildcard used for the major version segment.
   	wildcard: String,
   	},
   	}

   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.