rust: Overhaul errors to use an enum #781

axic · 2021-04-07T19:00:23Z

Depends on #783.
Based on #677.

bindings/rust/src/lib.rs

axic · 2021-04-07T19:01:11Z

bindings/rust/src/lib.rs

+        match self.0.code {
+            sys::FizzyErrorCode_FIZZY_SUCCESS => None,
+            sys::FizzyErrorCode_FIZZY_ERROR_MALFORMED_MODULE => {
+                Some(Error::MalformedModule(self.message()))


Could also think getting rid of code() and message() and only having error().

axic · 2021-04-07T19:06:57Z

bindings/rust/src/lib.rs

@@ -40,6 +40,29 @@ mod sys;
 use std::ffi::CString;
 use std::ptr::NonNull;

+#[derive(Eq, PartialEq, Debug, Clone)]
+pub enum Error {
+    MalformedModule(String),


Would be nicer using &str with a lifetime, but maybe better not getting FIzzyErrorBox involved into lifetime management?

No idea what this mean.

codecov · 2021-04-07T19:55:36Z

Codecov Report

Merging #781 (6f99c4a) into master (9ae449c) will decrease coverage by 0.06%.
The diff coverage is 90.42%.

❗ Current head 6f99c4a differs from pull request most recent head ad96b18. Consider uploading reports for the commit ad96b18 to get more accurate results

@@            Coverage Diff             @@
##           master     #781      +/-   ##
==========================================
- Coverage   99.35%   99.28%   -0.07%     
==========================================
  Files          86       86              
  Lines       13174    13192      +18     
==========================================
+ Hits        13089    13098       +9     
- Misses         85       94       +9

Flag	Coverage Δ
rust	`98.73% <90.42%> (-0.87%)`	⬇️
spectests	`89.98% <ø> (ø)`
unittests	`99.21% <ø> (ø)`
unittests-32	`99.31% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
bindings/rust/src/lib.rs	`99.01% <90.42%> (-0.89%)`	⬇️

axic · 2021-04-09T18:18:16Z

bindings/rust/src/lib.rs

+    InvalidModule(String),
+    InstantiationFailed(String),
+    Other(String),
+    FunctionNotFound,


The top 4 errors are 1-to-1 match with the C ones, and the below 5 are Rust specific.

gumb0

Looks fine to me.

Mirror error codes from C API

gumb0 · 2022-05-24T09:31:33Z

bindings/rust/src/lib.rs

-        assert_eq!(validate(&[]).err().unwrap(), "invalid wasm module prefix");
+        assert_eq!(
+            validate(&[]).err().unwrap(),
+            Error::MalformedModule("invalid wasm module prefix".to_string())


Consider defining expected error once instead of repeating Error::MalformedModule("invalid wasm module prefix".to_string()

(only for this case where error message is repeated. Repeating just an enum like Error::InvalidMemoryOffsetOrSize is fine)

True, I'm slightly leaning towards keeping it as-is because it is easier to change if underlying errors change, and it is only an error message.

chfast · 2022-05-24T09:31:18Z

bindings/rust/src/lib.rs

@@ -40,6 +40,29 @@ mod sys;
 use std::ffi::CString;
 use std::ptr::NonNull;

+#[derive(Eq, PartialEq, Debug, Clone)]
+pub enum Error {
+    MalformedModule(String),


No idea what this mean.

axic · 2022-05-24T09:40:43Z

No idea what this mean.

The top 5 errors are matching the C API: https://github.com/wasmx/fizzy/blob/master/include/fizzy/fizzy.h#L24-L39

The String parts means it can have a string explainer.

axic commented Apr 7, 2021

View reviewed changes

bindings/rust/src/lib.rs Outdated Show resolved Hide resolved

axic commented Apr 7, 2021

View reviewed changes

axic force-pushed the rust-new-error branch 2 times, most recently from 9c9b2d2 to bf89d44 Compare April 7, 2021 19:06

axic commented Apr 7, 2021

View reviewed changes

axic force-pushed the rust-rich-error branch 2 times, most recently from 451be0c to 3da0019 Compare April 9, 2021 16:35

Base automatically changed from rust-rich-error to master April 9, 2021 17:24

axic force-pushed the rust-new-error branch from bf89d44 to 028d869 Compare April 9, 2021 18:01

axic changed the base branch from master to capi-errors April 9, 2021 18:01

axic force-pushed the rust-new-error branch from 028d869 to 7b98d59 Compare April 9, 2021 18:11

axic marked this pull request as ready for review April 9, 2021 18:12

axic requested review from gumb0 and chfast April 9, 2021 18:12

axic commented Apr 9, 2021

View reviewed changes

axic force-pushed the capi-errors branch from 97cc314 to 330d078 Compare April 12, 2021 09:16

Base automatically changed from capi-errors to master April 12, 2021 10:40

axic force-pushed the rust-new-error branch 2 times, most recently from 28ce19b to d580d7e Compare April 12, 2021 10:47

gumb0 approved these changes Apr 12, 2021

View reviewed changes

axic force-pushed the rust-new-error branch from d580d7e to 374494a Compare May 6, 2022 19:03

axic force-pushed the rust-new-error branch from 374494a to 021b276 Compare May 23, 2022 09:16

axic mentioned this pull request May 23, 2022

rust: add error class #677

Closed

axic force-pushed the rust-new-error branch 3 times, most recently from bb220c3 to 8544c58 Compare May 23, 2022 23:22

axic requested a review from gumb0 May 23, 2022 23:25

axic added 9 commits May 24, 2022 01:31

rust: Introduce Error enum

43a4f06

Mirror error codes from C API

rust: Add FizzyErrorBox.error

df9f6eb

rust: Change validate() to Error type

dd2cd2c

rust: Change execute() to Error type

e8198f9

rust: Change parse() to Error type

bfd31c4

rust: Change instantiate() to Error type

985b38d

rust: Remove obsolete display trait (the error enum can be printed)

76e7012

rust: Use Error enum in memory helpers

a4196cd

rust: Add specific Error code for memory helpers

ad96b18

axic force-pushed the rust-new-error branch from 8544c58 to ad96b18 Compare May 23, 2022 23:33

gumb0 reviewed May 24, 2022

View reviewed changes

chfast approved these changes May 24, 2022

View reviewed changes

gumb0 approved these changes May 24, 2022

View reviewed changes

axic merged commit 513b073 into master May 24, 2022

axic deleted the rust-new-error branch May 24, 2022 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rust: Overhaul errors to use an enum #781

rust: Overhaul errors to use an enum #781

axic commented Apr 7, 2021 •

edited

Loading

axic Apr 7, 2021

axic Apr 7, 2021

chfast May 24, 2022

codecov bot commented Apr 7, 2021 •

edited

Loading

axic Apr 9, 2021

gumb0 left a comment

gumb0 May 24, 2022 •

edited

Loading

gumb0 May 24, 2022

axic May 24, 2022

chfast May 24, 2022

axic commented May 24, 2022 •

edited

Loading

rust: Overhaul errors to use an enum #781

rust: Overhaul errors to use an enum #781

Conversation

axic commented Apr 7, 2021 • edited Loading

axic Apr 7, 2021

Choose a reason for hiding this comment

axic Apr 7, 2021

Choose a reason for hiding this comment

chfast May 24, 2022

Choose a reason for hiding this comment

codecov bot commented Apr 7, 2021 • edited Loading

Codecov Report

axic Apr 9, 2021

Choose a reason for hiding this comment

gumb0 left a comment

Choose a reason for hiding this comment

gumb0 May 24, 2022 • edited Loading

Choose a reason for hiding this comment

gumb0 May 24, 2022

Choose a reason for hiding this comment

axic May 24, 2022

Choose a reason for hiding this comment

chfast May 24, 2022

Choose a reason for hiding this comment

axic commented May 24, 2022 • edited Loading

axic commented Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 7, 2021 •

edited

Loading

gumb0 May 24, 2022 •

edited

Loading

axic commented May 24, 2022 •

edited

Loading