Skip to content

add thiserror, split error::Error into different modules#324

Open
gerau wants to merge 8 commits into
BlockstreamResearch:masterfrom
gerau:simc/refactor-errors-split
Open

add thiserror, split error::Error into different modules#324
gerau wants to merge 8 commits into
BlockstreamResearch:masterfrom
gerau:simc/refactor-errors-split

Conversation

@gerau
Copy link
Copy Markdown
Contributor

@gerau gerau commented May 14, 2026

Implementing the first step of the error refactor issue: #204 (comment).

I tried to keep the changes minimal, so I did not refactor RichError completely. Also, keep in mind that we will most likely remove error::Error and replace RichError with something more like a Diagnostic.

@gerau gerau requested a review from delta1 as a code owner May 14, 2026 11:48
@gerau gerau force-pushed the simc/refactor-errors-split branch from ddf1800 to d1bb37b Compare May 14, 2026 11:59
Comment thread src/error.rs
ArgumentMissing(WitnessName),
ArgumentTypeMismatch(WitnessName, ResolvedType, ResolvedType),
LexerError(String),
ParsingError(crate::parse::Error),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was expecting to see ParsingError(#[from] crate::parse::Error), is there a reason it's not done this way?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm leaving the old Display implementation and not migrating to thiserror to keep the changes minimal. Also, I hope that we remove this enum completely, so I think it's fine to leave it as is.

Comment thread src/parse.rs Outdated
use crate::types::{AliasedType, BuiltinAlias, TypeConstructible, UIntType};

#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct SyntaxError {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub struct SyntaxError {
pub struct SyntaxErrorInfo {

Perhaps better to use a name that doesn't end in Error so that it is not confused with an error that can be returned

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, would rename it

Comment thread src/parse.rs
SyntaxError(#[from] SyntaxError),

#[error("Incompatible match arms: {0}, {1}")]
IncompatibleMatchArms(MatchPattern, MatchPattern),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
IncompatibleMatchArms(MatchPattern, MatchPattern),
IncompatibleMatchArms{ left: MatchPattern, right: MatchPattern},

More of a personal preference, but I prefer using the named struct pattern when the inner members are not obvious

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would also like to implement this in future PRs to keep the changes minimal. The plan is to also refactor these errors to contain more information (additional spans, notes, etc.)

gerau added 7 commits May 15, 2026 11:53
We moved SyntaxError as a separate struct so we can make custom Display
implementation
also add inside test reexport for parse::Error so there wouldn't be
conflicts after deleteting Error variants
This also made change inside some modules to use ast::Error instead of
error::Error.
@gerau gerau force-pushed the simc/refactor-errors-split branch from d1bb37b to c30d9ca Compare May 15, 2026 08:57
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