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

feat(interface-types) Implement the record instructions #1331

Merged
merged 44 commits into from
Apr 6, 2020

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Mar 24, 2020

Description

This PR implements the record WIT type, along with the record.lift and record.lower instructions.

With my current understanding of the draft/specification, here is how it works. Let's say we want to represent a Rust struct like the following:

struct S {
    x: String,
    y: i32
}

First declare a WIT type, such as:

(@interface type (record (field string) (field i32)))

The record type is supported by the binary encoder, the WAT encoder, the binary decoder, and the WAT decoder. A new TypeKind node has been introduced in the AST to differentiate a function type ((@interface type (func (param …) (result …)))) of a record type (see above).

Second, the record.lower transforms a host value (here Rust value, S) into a WIT value. In our implementation, a record value is defined as:

InterfaceValue::Record(Vec<InterfaceValue>)

Multiple mechanisms are used to type check a record value based on a record type. The code of the record.lower is pretty straightforward.

Because transforming a host value into a WIT value isn't obvious, a Serializer has been implemented, based on serde. This feature is behind the serde flag, which is turned on by default.

Serde is only used to cross the host/WIT boundary, but it's not used to represent the value in memory or anything. It's only a shortcut to transform a host value into a WIT value, and vice versa.

Use the following code to transform S into a WIT value:

#[derive(Serialize, Deserialize, Debug, PartialEq)]
struct S {
    x: String,
    y: i32,
}

let host_value = S { x: "hello".to_string(), y: 42 };
let wit_value = to_interface_value(&host_value).unwrap();

assert_eq!(
    wit_value,
    InterfaceValue::Record(vec![
        InterfaceValue::String("hello".to_string()),
        InterfaceValue::I32(42),
    ])
);

Third, the record.lift instruction does the opposite of record.lower: It transforms WIT values into a host value. To also facilitate the user experience, this PR contains a Deserializer implementation, still based on serde, with the from_interface_values function. It looks like this:

let wit_values = vec![
    InterfaceValue::Record(vec![
        InterfaceValue::String("hello".to_string()),
        InterfaceValue::I32(42),
    ])
];
let host_value = from_interface_values::<S>(&wit_values).unwrap();

assert_eq!(
    host_value,
    S { x: "hello".to_string(), y: 42 },
);

With the Serializer and Deserializer, it's super easy for the user to send or receive values from WIT.

The record.lift and record.lower instructions are kind of basic. The record.lift instruction has a little trick to reduce vector allocations, but there is a documentation for that.

Opened questions

Edit: Discussed here, WebAssembly/interface-types#108

Records of dimension 1 do not raise any issue. With record.lift, all values on the stack (the WIT interpreter stack) are popped, and are used as record field values. Something like:

[stack]
i32(1)
i64(2),
string("hello")
record.lift <record_type>

generates

[stack]
record { i32(1), i64(2), string("hello") }

But it's not clear what happens with record of dimension > 1, for instance for a type like record (field i32) (record (field i32) (field i32)) (field string), it is assumed (in this PR) that the stack must be like this:

[stack]
i32(1)
i32(2)
i32(3)
string("hello")
record.lift <record_type>

to generate:

[stack]
record { i32(1), record { i32(2), i32(3) }, string("hello") }

If we want the stack to contain an intermediate record, we should have something like this:

[stack]
i32(1)
i32(2)
i32(3)
record.lift <record_type_2>
string("hello")
record.lift <record_type_1>

But it would imply that record_type_1 is defined as record (field i32) (record (type record_type_2)) (field i32).

A sub-record defined by another record type isn't support, as it is not specified in the draft. I believe my assumption is fine enough for a first implementation of records in WIT.

To do

  • Encode and decode record type ((@interface type (record string i32))):
    • Binary encoder/decoder
    • WAT encoder/decoder
  • Implement the record.lift instruction
  • Implement the record.lower instruction
  • Test
  • Documentation
  • Surprise!
    • Serialize a Rust value to WIT values (useful for records)
    • Deserialize WIT values to a Rust value (useful for records)

@Hywan Hywan self-assigned this Mar 24, 2020
This patch updates the `Type` type to be an enum with 2 variants:
`Function` and `Record`, resp. to represent:

1. `(@interface type (func (param i32 i32) (result string)))`
2. `(@interface type (record string i32))`

This patch updates the binary encoder and decoder, along with the WAT
encoder and decoder.
@Hywan Hywan force-pushed the feat-interface-types-record-instructions branch from 68f248f to bbb4f1f Compare March 26, 2020 09:55
…`Type`.

The `Type::Record` variant now is defined by `RecordType`. In
addition, `InterfaceType` has a new variant: `Record`, that is also
defined by `RecordType`. Encoders and decoders are updated to consider
`RecordType`, which removes code duplication and simplify code.
@Hywan Hywan force-pushed the feat-interface-types-record-instructions branch from 61b4a42 to bd9226e Compare March 26, 2020 12:35
@Hywan Hywan force-pushed the feat-interface-types-record-instructions branch from 1c0f894 to 02b7e21 Compare March 31, 2020 10:34
@Hywan Hywan marked this pull request as ready for review April 2, 2020 10:07
@Hywan Hywan requested a review from MarkMcCaskey as a code owner April 2, 2020 10:07
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Looks good to me!

lib/interface-types/src/interpreter/wasm/serde/de.rs Outdated Show resolved Hide resolved
lib/interface-types/src/interpreter/wasm/serde/ser.rs Outdated Show resolved Hide resolved
lib/interface-types/src/interpreter/wasm/values.rs Outdated Show resolved Hide resolved
lib/interface-types/src/decoders/binary.rs Show resolved Hide resolved
@Hywan
Copy link
Contributor Author

Hywan commented Apr 6, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 6, 2020

Build succeeded

  • wasmerio.wasmer

@bors bors bot merged commit 4d33020 into wasmerio:master Apr 6, 2020
Hywan added a commit to Hywan/wasmer that referenced this pull request Apr 6, 2020
@Hywan Hywan mentioned this pull request Apr 6, 2020
syrusakbary added a commit that referenced this pull request Apr 6, 2020
@Hywan
Copy link
Contributor Author

Hywan commented Apr 7, 2020

The opened questions mentionned in the description of this PR are discussed here, WebAssembly/interface-types#108.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 documentation Do you like to read? 🎉 enhancement New feature! 🧪 tests I love tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants