Rec group builder

[somdoron]

Implements the embedder API requested in issue #10176.

Embedders can currently only build one-off struct/array/func types, so types that reference
themselves or each other can't be constructed directly. This adds RecGroupBuilder: declare
kind-typed labels, use them as forward references, and build() the whole recursion group at
once. It lowers to module-canonical WasmSubTypes and reuses the existing rec-group
registration path.

Also fixes an unrelated pre-existing bug in StorageType::is_val_type (matched I16 instead of
ValType), in its own commit.

[fitzgen]

@somdoron given that you just posted an AI plan verbatim in this PR's associated issue, I am assuming you also used AI to implement this PR. Can you confirm that you have already reviewed the AI tool's work in detail before posting it here and asking reviewers to spend their time on it? And are you taking full responsibility for the code in this PR?

https://github.com/bytecodealliance/governance/blob/main/AI_TOOL_POLICY.md

[somdoron]

@fitzgen I confirmed that I reviewed the AI tool. I reviewed the code, and I'm taking full responsibility for the code in this PR. The PR is for a real problem I'm suffering from, and is focused and small.

[github-actions]

Subscribe to Label Action

cc @fitzgen

Details

This issue or pull request has been labeled: "wasm-proposal:gc", "wasmtime:api", "wasmtime:docs"

Thus the following users have been cc'd because of the following labels:

• fitzgen: wasm-proposal:gc

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[fitzgen]

First of all, I want to say that this feature is going to involve a lot of API design, and probably won't land super quickly because of that. If you really need to be able to define rec groups in the host now, you can do that via building Modules (either via WAT strings or via wasm_encoder) that define the rec groups you need and export a global of each of the rec groups types so you can exfiltrate the GlobalType and the global type's ValType on the host via Module::exports. So that should unblock you for the time being, if necessary.

Some high-level thoughts on the design:

• I don't think we need to distinguish between pending structs versus pending arrays at the type level. I don't think it is buying us much in terms of correctness by construction or whatever, but it makes ramping up on the API that much harder.

• I don't love adding FieldTemplate/HeapTypeTemplate/etc... types to Wasmtime's public API. I think we could avoid that (and also not force callers to construct slices of them, potentially allocating when they otherwise wouldn't) by having a struct type builder that borrows the rec group builder and exposes a method to define fields one at a time, something like this:

  impl RecGroupBuilder<'_> {
      pub fn define_struct(&mut self, ty: PendingType) -> StructTypeBuilder<'_> { ... }
  }
  
  pub struct StructTypeBuilder<'a> {
      rec_group: &'a mut RecGroupBuilder,
      // ...
  }
  
  impl StructTypeBuilder<'_> {
      /// Define a normal, non-forward-reference field.
      pub fn field(&mut self, ty: FieldType) { ... }
  
      /// Define a field that is a forward reference to another type defined in this rec group.
      pub fn forward_ref_field(&mut self, ty: PendingType) -> ForwardRefFieldBuilder<'_> { ... }
  }
  
  pub struct ForwardRefFieldBuilder<'a> {
      parent: StructOrArrayTypeBuilder<'a>,
      // ...
  }
  
  impl ForwardRefFieldBuilder<'_> {
      pub fn nullable(&mut self, nullable: bool) -> &mut Self { ... }
  
      pub fn shared(&mut self, shared: bool) -> &mut Self { ... }
  
      pub fn finish(self) -> &mut StructTypeBuilder<'_> { ... }
  }

[somdoron]

Thanks, I have a workaround for now (similar to what you suggested).

I applied your suggestion, using a builder API for struct/array/func. I haven't built the ForwardRefFieldBuilder yet, I went with an API similar to StructType::new, taking both nullability and mutability as parameters. It does lack the shared field at the moment. Let me know if you think the field builder API is preferred.

[fitzgen]

I applied your suggestion, using a builder API for struct/array/func. I haven't built the ForwardRefFieldBuilder yet, I went with an API similar to StructType::new, taking both nullability and mutability as parameters. It does lack the shared field at the moment. Let me know if you think the field builder API is preferred.

Yeah the builder API is preferred because it is less brittle as the Wasm spec evolves (e.g. with the addition of shared to reference types).

[somdoron]

Done, now with the builder API for fields / array elements / function params.

I didn't add shared to any of the builders yet, as it isn't supported yet and I think it depends on the shared-everything-threads proposal.

[fitzgen]

Thanks, I think we are on the right track. Some more comments below.

(Don't want to start getting too detailed with the review until the skeleton / overall design is in the final shape.)

[fitzgen]

This isn't true at the spec level, and is easy to support (just remove this assertion and everything should work, AFAICT) so I don't think we should require it.

[somdoron]

fixed

[fitzgen]

Rather than duplicating all these types for the forward-or-not cases, could we reuse the types from wasmtime_environ::types::* and have the not-forward cases be EngineOrModuleTypeIndex::Engine and references within the rec group be EngineOrModuleTypeIndex::RecGroup or something like that?

[fitzgen]

Re: my previous comment, we would ideally make this become

Suggested change

	members: Vec<Option<MemberDef>>,
	inner: WasmRecGroup,

Although unfortunately that would require defining all struct vs array vs function types up front for all the members, leading to an awkward API for the builder, so I guess we probably actually want something like this:

Suggested change

	members: Vec<Option<MemberDef>>,
	members: Vec<Option<WasmSubType>>,

Although I guess we could do the first if we had different RecGroupBuilder::declare_struct vs RecGroupBuilder::declare_array vs RecGroupBuilder::decalre_func, as you originally formulated it... I didn't realize the implications that change would have here. I think we actually do want to have those separately-typed declare methods so we can have this builder wrap a WasmRecGroup. Sorry for the churn.

[somdoron]

I think Vec<Option<WasmSubType>> might be better. The reason is that TypeRegistryInner::register_rec_group_types takes an iterator of WasmSubType, so we don't get much benefit from wrapping a WasmRecGroup here.

In any case, we can't construct the underlying WasmSubTypes for the WasmRecGroup until the struct/array/func builder finishes. We could create a placeholder of the correct composite kind when declare_* is called, but we'd still have to replace it once the struct/array/func builder finishes. Since that overwrite is needed either way, I don't think the separately-typed declare methods are required to wrap a WasmRecGroup — unless you'd prefer them for API reasons.

[somdoron]

Correction: we do need the separate declare functions — they're required to build the WasmHeapType for a forward reference, whose concrete variant (ConcreteStruct / ConcreteArray / ConcreteFunc) encodes the composite kind. So the kind has to be known at declare time, even though the body isn't filled in yet.

[somdoron]

Some conclusions after digging in:

1. We can use multiple declare functions, one per type.
2. We can use WasmHeapType with EngineOrModuleTypeIndex::Engine and EngineOrModuleTypeIndex::Module for forward references. That would remove FieldDef, ValDef, and SuperDef.
3. Using WasmRecGroup or Vec<WasmSubType> for the RecGroupBuilder's internal type isn't very elegant, since they aren't mutable.
4. We can use Vec<WasmSubType>: create a placeholder on declare_*, and create the real type when we call RecGroupBuilder::build or Array/Func/StructBuilder::finish.

Anyway, I think keeping the MemberDef enum is more elegant than trying to use WasmRecGroup or Vec<WasmSubType>, since it's mutable and allows accumulation.

[fitzgen]

Using WasmRecGroup or Vec<WasmSubType> for the RecGroupBuilder's internal type isn't very elegant, since they aren't mutable.

We can add mutation methods to these types as necessary and change Box<[T]> to Vec<T> where needed. Given that, I'd prefer some combination of (3) and (4) over effectively duplicating the type hierarchy and adding mutation methods to one of the duplicate sets of types.

[somdoron]

done, I added mutation to those types. A separate commit just for those changes:
57b22da

[fitzgen]

This should document that the order of declare calls determines the order that the types are defined within the rec group, which has semantically visible implications. In the following example, $f != $f' and $s != $s', but you might otherwise not realize this when using the RecGroupBuilder, declare, and PendingType.

(rec (type $f (func))
     (type $s (struct)))

(rec (type $s' (struct))
     (type $f' (func)))

[somdoron]

done, added documentation to each declare function and RecGroupBuilder.

[somdoron]

The new commit stores Vec<Option<WasmSubType>> instead of a MemberDef. Each StructTypeBuilder/ArrayTypeBuilder/FuncTypeBuilder must have its finish method called to commit the type to the RecGroupBuilder, consistent with ForwardRefFieldBuilder. If finish is never called on one of these builders, that type's definition is silently dropped.

At the moment only RecGroupBuilder::build reports errors: the first error is stored on the RecGroupBuilder and returned when build is called. We could instead have StructTypeBuilder/ArrayTypeBuilder/FuncTypeBuilder::finish return the error directly.

For example, two structs where $a has a forward reference to $b:

let mut builder = RecGroupBuilder::new(&engine);

let a = builder.declare_struct();
let b = builder.declare_struct();

builder
    .define_struct(a)
    .forward_ref_field(b)
    .nullable(true)
    .finish()  // commit the field, back to the struct builder
    .finish(); // commit the struct to the rec group

builder
    .define_struct(b)
    .field(FieldType::new(
        Mutability::Const,
        StorageType::ValType(ValType::I32),
    ))
    .finish();

let group = builder.build()?;
let a: StructType = group.get_struct(a).unwrap();
let b: StructType = group.get_struct(b).unwrap();

[fitzgen]

If finish is never called on one of these builders, that type's definition is silently dropped.

I think this should become an error when RecGroupBuilder::build is called, rather than silently dropping types.

Also we should probably be consistent with finish vs build for RecGroupBuilder and StructTypeBuilder etc... I'm leaning towards build because we use that for MemoryTypeBuilder already.

At the moment only RecGroupBuilder::build reports errors: the first error is stored on the RecGroupBuilder and returned when build is called. We could instead have StructTypeBuilder/ArrayTypeBuilder/FuncTypeBuilder::finish return the error directly.

This is fine. Intermediate builder errors end up being annoying in practice, and can't cover everything so you need the final error anyways, so its often best to just delay all validation to the end and only have the final method be fallible.

[fitzgen]

Thanks for your ongoing patience with this PR!

[fitzgen]

Let's share a constant rather than have a comment saying it is / should be the same.

[somdoron]

done

[fitzgen]

If we have members: Vec<WasmSubType> in RecGroupBuilder and initialize the entries to the appropriate default WasmCompositeTypeInner variant, then we shouldn't need this anymore. We can just match on the WasmCompositeTypeInner in define_struct et al instead.

[somdoron]

done

[fitzgen]

Is there a reason we can't just delay all error checking until RecGroupBuilder::build? That would be cleaner.

[somdoron]

The accumulated errors are only the cross-engine checks, and those can't be delayed: the builder immediately lowers everything into WasmSubType, where an engine reference is a bare VMSharedTypeIndex with no engine identity — by build() time a foreign engine's index is indistinguishable from a valid one in this engine. Delaying the check would mean holding onto the FieldType/ValType/supertype handles until build(), which contradicts building directly into the wasmtime-environ types instead of keeping our own builder-side representation.

[somdoron]

I think this should become an error when RecGroupBuilder::build is called, rather than silently dropping types.

How do you feel about define_struct/forward_ref_field/etc. taking ownership of the builder? Then not calling build would be a compile error rather than a runtime one, and it all reads as a single chain.

The one downside is the config methods become self -> Self, so building fields in a loop needs x = x.field(..).

let mut builder = RecGroupBuilder::new(&engine);
let a = builder.declare_struct();
let b = builder.declare_struct();
let group = builder
    .define_struct(a).forward_ref_field(b).nullable(true).build().build()
    .define_struct(b).forward_ref_field(a).nullable(false).build().build()
    .build()?;

Anyway, now that the environ types are mutable, build actually does nothing — it only returns the parent builder to continue the chain. I didn't apply the ownership change.

Also we should probably be consistent with finish vs build for RecGroupBuilder and StructTypeBuilder etc... I'm leaning towards build because we use that for MemoryTypeBuilder already.

I changed it to build across the board, but I think there's a small difference: only RecGroupBuilder is actually building something. For the intermediate builders, finish reads nicer — all they do is return the parent builder, and even if they did add the type/field themselves, they still wouldn't be building the full type yet. Anyway, whatever you prefer.