rust: add typed value and execution result #705

axic · 2021-01-28T18:48:48Z

Depends on #652.

axic · 2021-01-28T18:50:05Z

bindings/rust/src/lib.rs

+    U64(u64),
+    F32(f32),
+    F64(f64),
+    Void,


I think we don't need Void as we should use Option<> instead.

I don't know Rust style, but seems Void will be useful in C/C++ in some cases.

Using Result/Option is more natural than having a special enum value in this case, it seems.

codecov · 2021-01-28T19:10:06Z

Codecov Report

Merging #705 (07034e0) into master (91f2ba4) will decrease coverage by 0.11%.
The diff coverage is 92.07%.

@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
- Coverage   99.32%   99.20%   -0.12%     
==========================================
  Files          72       73       +1     
  Lines       10660    11110     +450     
==========================================
+ Hits        10588    11022     +434     
- Misses         72       88      +16

Flag	Coverage Δ
rust	`96.44% <92.07%> (?)`
spectests	`90.84% <ø> (ø)`
unittests	`99.32% <ø> (ø)`

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

Impacted Files	Coverage Δ
bindings/rust/src/lib.rs	`96.44% <92.07%> (ø)`

axic · 2021-01-28T20:07:58Z

In this we can also consider introducing TypedExecutionResult or just returning Result<Option<TypedValue>,FizzyError> in execute. We would have a special Trap error for traps.

axic · 2021-02-01T19:55:01Z

bindings/rust/src/lib.rs

@@ -131,6 +131,84 @@ impl From<f64> for Value {
    }
 }

+/// A WebAssembly value i32/i64/f32/f64 with its type specified.
+pub enum TypedValue {
+    U32(u32),


Since sys::FizzyValueType only supports an "unsigned" type for 32/64-bit integers, we are matching that here. The naming discrepancy is that I wanted to make clear U32/U64 matches the unsigned Rust type u32/u64.

axic · 2021-02-01T19:56:08Z

bindings/rust/src/lib.rs

+
+    pub fn as_i32(&self) -> Option<i32> {
+        match self {
+            TypedValue::U32(v) => Some(*v as i32),


This is still a question, whether we want to do this, or use Rust specific helpers for casting between u32 -> i32.

Do you mean the alternative being x.as_u32() as i32? I think this is fine then.

axic · 2021-02-01T19:57:37Z

bindings/rust/src/lib.rs

+    pub fn value(&self) -> Option<TypedValue> {
+        if self.result.has_value {
+            assert!(!self.result.trapped);
+            assert!(self.value_type != sys::FizzyValueTypeVoid);


Can merge these assertions.

These can stay as they are.

axic · 2021-02-03T15:43:31Z

bindings/rust/src/lib.rs

@@ -414,7 +631,7 @@ mod tests {
        let result = result.unwrap();
        assert!(!result.trapped());
        assert!(result.value().is_some());
-        assert_eq!(result.value().unwrap().as_i32(), 42);
+        assert_eq!(result.value().unwrap().as_u32().unwrap(), 42);


Hmm, perhaps could consider a TryInto trait implementation instead, maybe more canonical.

chfast · 2021-02-03T15:51:48Z

bindings/rust/src/lib.rs

@@ -269,6 +380,113 @@ mod tests {
        assert_eq!(v.as_f64(), f64::MAX);
    }

+    #[test]
+    fn typed_value_conversion() {


You need tests for wrong type.

Wrong input type is a compilation error. I can add checks for not being able to retrieve wrong output.

I mean the case when .as_u32() is None.

I pushed that 10 minutes ago, just github is slow today showing up changes.

gumb0 · 2021-02-03T18:42:26Z

bindings/rust/src/lib.rs

@@ -151,6 +218,36 @@ impl ExecutionResult {
    }
 }

+/// The result of an execution.
+pub struct TypedExecutionResult {
+    result: sys::FizzyExecutionResult,


Would this be more convenient as ExecutionResult type?

No, because it hides the very details we are checking. Basically TypedExecutionResult and ExecutionResult is a bit of a duplication. I think in the future we may want to review this and perhaps get rid of the unsafe execution, once we have benchmarked the overheads.

axic commented Jan 28, 2021

View reviewed changes

axic mentioned this pull request Jan 28, 2021

rust: implement safer execution wrapper #652

Merged

axic force-pushed the rust-safe-execute branch from bd46cf2 to 8fe90d3 Compare January 28, 2021 19:38

axic force-pushed the rust-typed-value branch from 96e89a5 to 54fe636 Compare January 28, 2021 19:39

axic force-pushed the rust-safe-execute branch 4 times, most recently from 693ad0d to 5e5e43f Compare January 29, 2021 11:57

Base automatically changed from rust-safe-execute to master January 29, 2021 12:32

axic force-pushed the rust-typed-value branch 2 times, most recently from b4f79c4 to b7a8bb0 Compare February 1, 2021 19:45

axic commented Feb 1, 2021

View reviewed changes

axic marked this pull request as ready for review February 3, 2021 15:39

axic force-pushed the rust-typed-value branch from 39c8b3b to decee82 Compare February 3, 2021 15:39

axic requested review from chfast and gumb0 February 3, 2021 15:40

axic changed the title ~~rust: add typed value~~ rust: add typed value and execution result Feb 3, 2021

axic commented Feb 3, 2021

View reviewed changes

axic force-pushed the rust-typed-value branch from decee82 to 557785f Compare February 3, 2021 15:50

chfast reviewed Feb 3, 2021

View reviewed changes

chfast approved these changes Feb 3, 2021

View reviewed changes

gumb0 reviewed Feb 3, 2021

View reviewed changes

gumb0 approved these changes Feb 3, 2021

View reviewed changes

axic force-pushed the rust-typed-value branch from 7b1d470 to 0c3db09 Compare February 3, 2021 23:31

rust: add TypedValue which allows proper validation of values

deed945

axic force-pushed the rust-typed-value branch from 0c3db09 to 76f9a5a Compare February 4, 2021 00:25

rust: add TypedExecutionResult which allows returning TypedValues

07034e0

axic force-pushed the rust-typed-value branch from 76f9a5a to 07034e0 Compare February 4, 2021 00:31

axic merged commit 2b0693c into master Feb 4, 2021

axic deleted the rust-typed-value branch February 4, 2021 01:00

axic mentioned this pull request Feb 4, 2021

Rust binding #722

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rust: add typed value and execution result #705

rust: add typed value and execution result #705

axic commented Jan 28, 2021 •

edited

Loading

axic Jan 28, 2021

chfast Feb 2, 2021

axic Feb 3, 2021

codecov bot commented Jan 28, 2021 •

edited

Loading

axic commented Jan 28, 2021

axic Feb 1, 2021

axic Feb 1, 2021

chfast Feb 2, 2021

axic Feb 1, 2021

chfast Feb 2, 2021

axic Feb 3, 2021

chfast Feb 3, 2021

axic Feb 3, 2021 •

edited

Loading

chfast Feb 3, 2021

axic Feb 3, 2021

gumb0 Feb 3, 2021

axic Feb 3, 2021

rust: add typed value and execution result #705

rust: add typed value and execution result #705

Conversation

axic commented Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 28, 2021 • edited Loading

Codecov Report

axic commented Jan 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Jan 28, 2021 •

edited

Loading

codecov bot commented Jan 28, 2021 •

edited

Loading

axic Feb 3, 2021 •

edited

Loading