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

Error refactoring and thiserror 2.0 #1588

Open
wants to merge 11 commits into
base: next
Choose a base branch
from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
- [BREAKING] Updated Winterfell dependency to v0.11 (#1586).
- [BREAKING] resolved flag collision in `--verify` command and added functionality for optional input/output files (#1513).
- [BREAKING] Cleanup benchmarks and examples in the `miden-vm` crate (#1587)
- [BREAKING] Use `thiserror` 2.0 to derive errors and refactor them (#1588).

#### Enhancements
- Added `miden_core::mast::MastForest::advice_map` to load it into the advice provider before the `MastForest` execution (#1574).
Expand Down
38 changes: 10 additions & 28 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,6 @@ inherits = "release"
debug = true
debug-assertions = true
overflow-checks = true

[workspace.dependencies]
thiserror = { version = "2.0", default-features = false }
2 changes: 1 addition & 1 deletion air/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ std = ["vm-core/std", "winter-air/std", "thiserror/std"]
testing = []

[dependencies]
thiserror = { package = "miden-thiserror", version = "1.0", default-features = false }
thiserror = { workspace = true }
vm-core = { package = "miden-core", path = "../core", version = "0.11", default-features = false }
winter-air = { package = "winter-air", version = "0.11", default-features = false }
winter-prover = { package = "winter-prover", version = "0.11", default-features = false }
Expand Down
31 changes: 5 additions & 26 deletions air/src/errors.rs
Original file line number Diff line number Diff line change
@@ -1,33 +1,12 @@
use alloc::string::String;
use core::fmt::{Display, Formatter};

use crate::trace::MIN_TRACE_LEN;

// EXECUTION ERROR
// EXECUTION OPTIONS ERROR
// ================================================================================================

#[derive(Debug)]
#[derive(Debug, thiserror::Error)]
pub enum ExecutionOptionsError {
ExpectedCyclesTooBig(u32, u32),
#[error("expected number of cycles {expected_cycles} must be smaller than the maximum number of cycles {max_cycles}")]
ExpectedCyclesTooBig { max_cycles: u32, expected_cycles: u32 },
#[error("maximum number of cycles {0} must be greater than the minimum number of cycles {MIN_TRACE_LEN}")]
MaxCycleNumTooSmall(u32),
OtherErrors(String),
}

impl Display for ExecutionOptionsError {
fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), core::fmt::Error> {
use ExecutionOptionsError::*;

match self {
ExpectedCyclesTooBig(max, expected) => {
write!(f, "The expected number of cycles must be smaller than the maximum number of cycles: maximum is {max}, but expectd is {expected}")
},
MaxCycleNumTooSmall(max) => {
write!(f, "The maximum number of cycles must be greater than the minimum number of cycles: minimum is {MIN_TRACE_LEN}, but maximum is {max}")
},
OtherErrors(error) => write!(f, "{error}"),
}
}
}

#[cfg(feature = "std")]
impl std::error::Error for ExecutionOptionsError {}
5 changes: 4 additions & 1 deletion air/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,10 @@ impl ExecutionOptions {
return Err(ExecutionOptionsError::MaxCycleNumTooSmall(expected_cycles));
}
if max_cycles < expected_cycles {
return Err(ExecutionOptionsError::ExpectedCyclesTooBig(max_cycles, expected_cycles));
return Err(ExecutionOptionsError::ExpectedCyclesTooBig {
max_cycles,
expected_cycles,
});
}

// Round up the expected number of cycles to the next power of two. If it is smaller than
Expand Down
2 changes: 1 addition & 1 deletion air/src/trace/rows.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use vm_core::Felt;

/// Represents the types of errors that can occur when converting from and into [`RowIndex`] and
/// using its operations.
#[derive(Debug, thiserror::Error, PartialEq, Eq)]
#[derive(Debug, thiserror::Error)]
pub enum RowIndexError<T> {
#[error("value is too large to be converted into RowIndex: {0}")]
InvalidSize(T),
Expand Down
6 changes: 3 additions & 3 deletions assembly/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,19 @@ doctest = false

[features]
default = ["std"]
std = ["aho-corasick/std", "miette/fancy", "miette/std", "thiserror/std", "vm-core/std"]
std = ["aho-corasick/std", "miette/fancy", "miette/std", "vm-core/std", "thiserror/std"]
testing = ["dep:regex"]

[dependencies]
aho-corasick = { version = "1.1", default-features = false }
lalrpop-util = { version = "0.20", default-features = false }
miette = { package = "miden-miette", version = "7.1", default-features = false, features = [
miette = { package = "miden-miette", version = "8.0", default-features = false, features = [
"fancy-no-syscall",
"derive"
] }
regex = { version = "1.10", optional = true, default-features = false, features = ["unicode", "perf"] }
smallvec = { version = "1.13", features = ["union", "const_generics", "const_new"] }
thiserror = { package = "miden-thiserror", version = "1.0", default-features = false }
thiserror = { workspace = true }
tracing = { version = "0.1", default-features = false, features = ["attributes"] }
unicode-width = { version = "0.2", features = ["no_std"] }
vm-core = { package = "miden-core", path = "../core", version = "0.11", default-features = false, features = [
Expand Down
34 changes: 26 additions & 8 deletions assembly/src/assembler/mast_forest_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,9 @@ impl MastForestBuilder {
// decorator already exists in the forest; return previously assigned id
Ok(*decorator_id)
} else {
let new_decorator_id = self.mast_forest.add_decorator(decorator)?;
let new_decorator_id = self.mast_forest.add_decorator(decorator).map_err(|source| {
AssemblyError::forest_error("assembler failed to add new decorator", source)
})?;
self.decorator_id_by_fingerprint.insert(decorator_hash, new_decorator_id);

Ok(new_decorator_id)
Expand All @@ -366,7 +368,9 @@ impl MastForestBuilder {
// node already exists in the forest; return previously assigned id
Ok(*node_id)
} else {
let new_node_id = self.mast_forest.add_node(node)?;
let new_node_id = self.mast_forest.add_node(node).map_err(|source| {
AssemblyError::forest_error("assembler failed to add new node", source)
})?;
self.node_id_by_fingerprint.insert(node_fingerprint, new_node_id);
self.hash_by_node_id.insert(new_node_id, node_fingerprint);

Expand All @@ -380,7 +384,9 @@ impl MastForestBuilder {
operations: Vec<Operation>,
decorators: Option<DecoratorList>,
) -> Result<MastNodeId, AssemblyError> {
let block = MastNode::new_basic_block(operations, decorators)?;
let block = MastNode::new_basic_block(operations, decorators).map_err(|source| {
AssemblyError::forest_error("assembler failed to add new basic block node", source)
})?;
self.ensure_node(block)
}

Expand All @@ -390,7 +396,10 @@ impl MastForestBuilder {
left_child: MastNodeId,
right_child: MastNodeId,
) -> Result<MastNodeId, AssemblyError> {
let join = MastNode::new_join(left_child, right_child, &self.mast_forest)?;
let join =
MastNode::new_join(left_child, right_child, &self.mast_forest).map_err(|source| {
AssemblyError::forest_error("assembler failed to add new join node", source)
})?;
self.ensure_node(join)
}

Expand All @@ -400,25 +409,34 @@ impl MastForestBuilder {
if_branch: MastNodeId,
else_branch: MastNodeId,
) -> Result<MastNodeId, AssemblyError> {
let split = MastNode::new_split(if_branch, else_branch, &self.mast_forest)?;
let split =
MastNode::new_split(if_branch, else_branch, &self.mast_forest).map_err(|source| {
AssemblyError::forest_error("assembler failed to add new split node", source)
})?;
self.ensure_node(split)
}

/// Adds a loop node to the forest, and returns the [`MastNodeId`] associated with it.
pub fn ensure_loop(&mut self, body: MastNodeId) -> Result<MastNodeId, AssemblyError> {
let loop_node = MastNode::new_loop(body, &self.mast_forest)?;
let loop_node = MastNode::new_loop(body, &self.mast_forest).map_err(|source| {
AssemblyError::forest_error("assembler failed to add new loop node", source)
})?;
self.ensure_node(loop_node)
}

/// Adds a call node to the forest, and returns the [`MastNodeId`] associated with it.
pub fn ensure_call(&mut self, callee: MastNodeId) -> Result<MastNodeId, AssemblyError> {
let call = MastNode::new_call(callee, &self.mast_forest)?;
let call = MastNode::new_call(callee, &self.mast_forest).map_err(|source| {
AssemblyError::forest_error("assembler failed to add new call node", source)
})?;
self.ensure_node(call)
}

/// Adds a syscall node to the forest, and returns the [`MastNodeId`] associated with it.
pub fn ensure_syscall(&mut self, callee: MastNodeId) -> Result<MastNodeId, AssemblyError> {
let syscall = MastNode::new_syscall(callee, &self.mast_forest)?;
let syscall = MastNode::new_syscall(callee, &self.mast_forest).map_err(|source| {
AssemblyError::forest_error("assembler failed to add new syscall node", source)
})?;
self.ensure_node(syscall)
}

Expand Down
4 changes: 2 additions & 2 deletions assembly/src/ast/ident.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use core::{
use crate::{SourceSpan, Span, Spanned};

/// Represents the types of errors that can occur when parsing/validating an [Ident]
#[derive(Debug, thiserror::Error, PartialEq, Eq)]
#[derive(Debug, thiserror::Error)]
pub enum IdentError {
#[error("invalid identifier: cannot be empty")]
Empty,
Expand All @@ -24,7 +24,7 @@ pub enum IdentError {

/// Represents the various types of casing errors that can occur, e.g. using an identifier
/// with `SCREAMING_CASE` where one with `snake_case` is expected.
#[derive(Debug, thiserror::Error, PartialEq, Eq)]
#[derive(Debug, thiserror::Error)]
pub enum CaseKindError {
#[error("only uppercase characters or underscores are allowed, and must start with an alphabetic character")]
Screaming,
Expand Down
17 changes: 10 additions & 7 deletions assembly/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use vm_core::mast::MastForestError;

use crate::{
ast::QualifiedProcedureName,
diagnostics::{Diagnostic, RelatedError, RelatedLabel, Report, SourceFile},
diagnostics::{Diagnostic, RelatedError, RelatedLabel, SourceFile},
LibraryNamespace, LibraryPath, SourceSpan,
};

Expand Down Expand Up @@ -81,13 +81,16 @@ pub enum AssemblyError {
},
#[error(transparent)]
#[diagnostic(transparent)]
Other(#[from] RelatedError),
#[error(transparent)]
Forest(#[from] MastForestError),
Other(RelatedError),
// Technically MastForestError is the source error here, but since AssemblyError is converted
// into a Report and that doesn't implement core::error::Error, treating MastForestError as a
// source error would effectively swallow it, so we include it in the error message instead.
#[error("{0}: {1}")]
Forest(&'static str, MastForestError),
}

impl From<Report> for AssemblyError {
fn from(report: Report) -> Self {
Self::Other(RelatedError::new(report))
impl AssemblyError {
pub(super) fn forest_error(message: &'static str, source: MastForestError) -> Self {
Self::Forest(message, source)
}
}
4 changes: 2 additions & 2 deletions assembly/src/library/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ pub enum LibraryError {
EmptyKernel,
#[error("invalid export in kernel library: {procedure_path}")]
InvalidKernelExport { procedure_path: QualifiedProcedureName },
#[error(transparent)]
Kernel(#[from] KernelError),
#[error("failed to convert library into kernel library: {0}")]
KernelConversion(KernelError),
#[error("invalid export: no procedure root for {procedure_path} procedure")]
NoProcedureRootForExport { procedure_path: QualifiedProcedureName },
}
2 changes: 1 addition & 1 deletion assembly/src/library/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ impl TryFrom<Library> for KernelLibrary {
kernel_module.add_procedure(proc_path.name.clone(), proc_digest);
}

let kernel = Kernel::new(&proc_digests)?;
let kernel = Kernel::new(&proc_digests).map_err(LibraryError::KernelConversion)?;

Ok(Self {
kernel,
Expand Down
2 changes: 1 addition & 1 deletion assembly/src/library/namespace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::{
// ================================================================================================

/// Represents an error when parsing or validating a library namespace
#[derive(Debug, thiserror::Error, Diagnostic, PartialEq, Eq)]
#[derive(Debug, thiserror::Error, Diagnostic)]
pub enum LibraryNamespaceError {
#[error("invalid library namespace name: cannot be a empty")]
#[diagnostic()]
Expand Down
Loading
Loading