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

Refactor errors to include position, span (offset + len), and optional miette integration #95

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chanced
Copy link
Owner

@chanced chanced commented Oct 22, 2024

This pull request:

  • Adds Span structure which contains len & offset
  • Adds Spanned error wrapper struct that contains a Span and a source error. It implements miette::Diagnostic by acting as a pass-through for all methods except labels (used to mark spans). For labels, it provides a blank (None) label with the span
  • Adds Positioned error wrapper struct that contains a Spanned error and a position. It also implements miette::Diagnostic by deferring to Spanned
  • Changes all errors to tuple variants, using either Positioned or Spanned, depending on context.

I'm just getting started with it so it will not compile right now. I should hopefully have it done today.

* Adds type alias `crate::resolve::ResolveError` for `crate::resolve::Error`
* Renames `crate::assign::AssignError` to `crate::assign::Error`
* Adds type alias `crate::assign::AssignError` for crate::assign::Error`
* Adds `position` (token index) to variants of `assign::Error` & `resolve::Error`
@chanced
Copy link
Owner Author

chanced commented Oct 22, 2024

@asmello incase you happen to have a minute - can you please spot this and give me a bit of feedback on how you feel about both the degree of breaking change this is going to introduce as well as the general design? I only spot checked one crate which uses miette and it was kinda convoluted. I think this approach is fairly clean but it does break pretty much all error handling for consumers.

Here's a short summary, told through types:

pub struct Span {
    pub offset: usize,
    pub len: usize,
}

pub struct Spanned<E> {
    span: Span,
    source: E,
}

#[cfg(feature = "miette")]
impl<E> miette::Diagnostic for Spanned<E>
where
    E: 'static + miette::Diagnostic,
{
    fn labels(&self) -> Option<Box<dyn Iterator<Item = miette::LabeledSpan> + '_>> {
        Some(Box::new(once(miette::LabeledSpan::new(
            None,      // no label - this would require an allocation (String) + doesnt seem like we can add much
            self.offset(), // self.span.offset
            self.len(),    // self.span.len
        ))))
    }
  // all other methods are passthrough to `self::source`
}

// implements miette::Diagnostic as passthrough to Spanned<E>
pub struct Positioned<E> {
    position: usize,
    source: Spanned<E>,
}

usage is:

pub enum Error {
    FailedToParseIndex(Positioned<ParseIndexError>),
    OutOfBounds(Positioned<OutOfBoundsError>),
}

@asmello
Copy link
Collaborator

asmello commented Oct 22, 2024

I haven't used miette like this (my uses have been through the dynamic macros or thiserror integration), but will give a look.

@chanced
Copy link
Owner Author

chanced commented Oct 22, 2024

I will need to use the miette's Diagnostic derive macro on the errors themselves (e.g. ParseIndexError)

@asmello
Copy link
Collaborator

asmello commented Oct 22, 2024

Oh I think I understand, each Error will implement Diagnostic on its own, but we'll leave the labels to a wrapper since that part is common to most (all?) of them. I think this makes sense. We may want some wrapper for the source code part too, for a similar benefit. After all the labels are pointless without the source code.

I think it'll be worth playing around with a few errors and seeing how it goes. Biggest risk I see is this ends up being equivalent work to just implementing these directly for each error type.

@chanced
Copy link
Owner Author

chanced commented Oct 22, 2024

Oh, right! I forgot about source!

So the reason I opted for the wrappers is that most of the errors themselves do not have knowledge of their surroundings. I can change that though with wrappers specific to each.

/// Indicates that an `Index` is not within the given bounds.
#[derive(Debug, PartialEq, Eq)]
pub struct OutOfBoundsError {
    pub length: usize,
    pub index: usize,
}

to

#[derive(Debug, PartialEq, Eq)]
pub struct OutOfBoundsErrorWrapper {
    pub source: OutOfBoundsError,
    pub position: usize,
    pub span: Span,
}

or something like it

@chanced
Copy link
Owner Author

chanced commented Oct 22, 2024

Or I guess I could go the other route and refactor the error into a struct:

// assign::Error
pub struct Error {
     kind: Kind,
     position: usize,
     span: Span,
}

pub enum Kind {
    FailedToParseIndex(ParseIndexError),
    OutOfBounds(OutOfBoundsError),
}

@chanced
Copy link
Owner Author

chanced commented Oct 22, 2024

I'm going to flush this design out to see how it works - I haven't figured out what to do about source yet (the convoluted nature of pest's errors now make a lot more sense) so it may be all for not.

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.

2 participants