add `thiserror`, split `error::Error` into different modules by gerau · Pull Request #324 · BlockstreamResearch/SimplicityHL

gerau · 2026-05-14T11:48:28Z

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.

stringhandler · 2026-05-15T06:59:39Z

-    ArgumentMissing(WitnessName),
-    ArgumentTypeMismatch(WitnessName, ResolvedType, ResolvedType),
+    LexerError(String),
+    ParsingError(crate::parse::Error),


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

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.

stringhandler · 2026-05-15T07:01:31Z

 use crate::types::{AliasedType, BuiltinAlias, TypeConstructible, UIntType};

+#[derive(Debug, Clone, Eq, PartialEq, Hash)]
+pub struct SyntaxError {


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

Yep, would rename it

stringhandler · 2026-05-15T07:03:44Z

+    SyntaxError(#[from] SyntaxError),
+
+    #[error("Incompatible match arms: {0}, {1}")]
+    IncompatibleMatchArms(MatchPattern, MatchPattern),


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

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

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.

apoelstra · 2026-05-17T18:53:36Z

+
+impl From<simplicity::elements::hex::Error> for Error {
+    fn from(error: simplicity::elements::hex::Error) -> Self {
+        Self::CannotParse(error.to_string())


In e656be0:

In general I don't like these From impls -- it is better to make the user write .map_err(some_explicit_conversion)? rather than just ?, because if you have the single-character ? available, you're really discouraged from adding more error context at call sides.

Then in specific, I really don't like these From impls which convert to strings and then shove the strings into a generic enum variant.

apoelstra · 2026-05-17T18:56:48Z

+    FunctionUndefined(FunctionName),
+
+    #[error("Failed to compile to Simplicity: {0}")]
+    CannotCompile(String),


In 39ff893:

This is used in only one place - to indicate that the use keyword is not yet supported. We should have a dedicated error variant for this.

apoelstra · 2026-05-17T18:57:34Z

+    BitStringPow2(usize),
+
+    #[error("{0}")]
+    Generic(String),


In 39ff893:

What is this for? It is unused in this commit. Can it hold a Box<dyn std::error::Error> rather than a string?

apoelstra · 2026-05-17T18:57:58Z

+}
+
+impl From<crate::num::ParseIntError> for Error {
+    fn from(error: crate::num::ParseIntError) -> Self {


In 39ff893:

My same complaint about the From impls in the last commit apply here.

apoelstra · 2026-05-17T19:04:55Z

Rather than reviewing all these commits, let me try to provide some general guidance:

don't represent errors as Strings unless you really have to (the two cases I know of are: if you are wrapping an error which is already a String; if you are holding a generic error where you have alloc but not std and a MSRV of 1.80 or less)
don't have error messages like cannot parse: {0} where {0} is a wrapped error. Just use cannot parse. The user can call source if they want to see {0}
don't use anonymous fields like FileNotFound(String). Use FileNotFound { filename: String }. Also, in that case, consider whether the filename should actually be a PathBuf or a OsString.
don't implement From<WrappedError> for Error which just stuffs the error in a wrapper, unless you are sure that there is no additional context that will ever be available

apoelstra · 2026-05-17T19:06:13Z

I understand that you're violating many of these guidelines to keep the changes minimal so that each PR is reviewable.

Would it be possible to change only some errors at a time, but to do so completely, to create minimal PRs? For example, I think any error type which is currently a String could be replaced by a strongly-typed error in its own PR.

gerau · 2026-05-18T08:37:33Z

Thanks for the guidance!

Would it be possible to change only some errors at a time, but to do so completely, to create minimal PRs? For example, I think any error type which is currently a String could be replaced by a strongly-typed error in its own PR.

Okay, I will try this first. Basically, I wanted to do most of this after this PR, but after your comments, I think doing it before makes more sense.

add thiserror dependency

01418fb

gerau requested a review from delta1 as a code owner May 14, 2026 11:48

gerau force-pushed the simc/refactor-errors-split branch from ddf1800 to d1bb37b Compare May 14, 2026 11:59

stringhandler reviewed May 15, 2026

View reviewed changes

gerau added 7 commits May 15, 2026 11:53

parse: add error type for parsing stage

e656be0

We moved SyntaxError as a separate struct so we can make custom Display implementation

error: change chumsky traits implementation to use new error

2b90d67

also add inside test reexport for parse::Error so there wouldn't be conflicts after deleteting Error variants

ast: add error type for AST

39ff893

This also made change inside some modules to use ast::Error instead of error::Error.

driver: add error for driver

98ca750

compile: add compiling error

8271060

lexer: add LexerError in error::Error

fd43ffc

remove old Error variants

c30d9ca

gerau force-pushed the simc/refactor-errors-split branch from d1bb37b to c30d9ca Compare May 15, 2026 08:57

apoelstra reviewed May 17, 2026

View reviewed changes

gerau marked this pull request as draft May 18, 2026 08:37

gerau mentioned this pull request May 18, 2026

simc: error: refactor generic String to error types #325

Open

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

Conversation

gerau commented May 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented May 17, 2026

Uh oh!

apoelstra commented May 17, 2026

Uh oh!

gerau commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants