Implement type imports and exports by vouillon · Pull Request #7330 · WebAssembly/binaryen

vouillon · 2025-02-26T17:04:38Z

Implementation of the Type Imports and Exports proposal, which allows Wasm module to export and import types:

        (type $File (export "File") (struct ...))
        (import "file" "File" (type $File (sub any)))

wasm-merge can combine several modules using this proposal, connecting type imports to corresponding type exports. To produce an output compatible with existing tools, an option --strip-type-exports can be used to omit any type export from the output.

From an implementation point of view, for imports, a new kind of heap type is used:

        (import "module" "base" (sub absheaptype))

Including the module and base names in the type definition works well with type canonicalization.

For exports, we separate type exports from other exports, since they don't have an internal name but a heap type:

        class TypeExport {
          Name name;
          HeapType heaptype; // exported type
        };

This is somewhat error-prone: to check for repeated exports, we need to check two tables. But the alternatives do not seem much better. If we put all exports together, then we have to make sure that we are never trying to access the internal name of a type import.

Implementation of the Type Imports and Exports proposal, which allows Wasm module to export and import types: (type $File (export "File") (struct ...)) (import "file" "File" (type $File (sub any))) wasm-merge can be used to combine several modules using this proposal, connecting type imports to corresponding type exports. To produce an output compatible with existing tools, an option --strip-type-exports can be use to omit any type export from the output. From an implementation point of view, for imports, a new kind of heap type is used: (import "module" "base" (sub absheaptype)) Including the module and base names in the type definition works well with type canonicalisation. For exports, we separate type exports from other exports, since they don't have an internal name but a heap type: class TypeExport { Name name; HeapType heaptype; // exported type }; This is somewhat error-prone. Typically, to check for repeated exports, we need to check two tables. But the alternatives do not seem much better. If we put all export together, then we have to make sure that we are never trying to access the internal name of a type import.

tlively

Thanks for the PR, this looks fantastic. In addition to the comments below, it would be good to add unit tests for type canonicalization and subtyping relationships in test/gtest/type-builder.cpp. It would also be good to add tests for MinifyImportsAndExports, StripTypeExports, and TypeMerging passes with type imports to check that they do the right thing, as well as some tests for errors such as trying to declare a subtype of an imported type.

src/ir/module-utils.cpp

tlively · 2025-02-26T21:31:15Z

src/parser/parsers.h

+  if (!ctx.in.takeSExprStart("sub"sv)) {
+    return ctx.makeAnyType(Unshared);
+  }


It doesn't look like the (sub absheaptype) clause is optional in the proposal overview. Is this shorthand documented elsewhere?

This is mentioned in the Proposal Summary

The text format allows to omit the constraint, in which case it defaults to (sub any).

tlively · 2025-02-26T21:36:43Z

src/parser/parsers.h

+    if (inRecGroup) {
+      return ctx.in.err("type import not allowed in recursive group");
+    }


Unless this is part of the proposed text format extension, what do you think about deferring this check to TypeBuilder::build? Since all type definitions that are not in rec groups desugar to singleton rec groups, I think it would be more natural to allow type imports only in singleton rec groups, but also allow the rec group to be explicit in the text.

The proposal currently does not say anything about the abbreviation (type $t (import "foo" "bar").
I'll make the change.

tlively · 2025-02-26T22:22:27Z

src/parser/contexts.h

  std::vector<DefPos> dataDefs;
  std::vector<DefPos> tagDefs;

+  // Type imports: name, positions of type and import names.


Suggested change

// Type imports: name, positions of type and import names.

// Type imports: name, export names, import names, and type index.

tlively · 2025-02-26T22:41:44Z

src/passes/TypeMerging.cpp

    case HeapTypeKind::Cont:
      WASM_UNREACHABLE("TODO: cont");
+    case HeapTypeKind::Import:
+      return false;


Would it make sense to merge duplicate imports here?

Duplicate imports are merged by the type canonicalization.

tlively · 2025-02-27T00:15:02Z

src/wasm/wasm-type-shape.cpp

        hash_combine(digest, hash(type.getContinuation()));
        return digest;
+      case HeapTypeKind::Import:
+        assert(type.isContinuation());


Copy-paste error?

tlively · 2025-02-27T00:16:18Z

src/wasm/wasm-type-shape.cpp


  size_t hash(Continuation cont) { return hash(cont.type); }

+  size_t hash(Import import) { return hash(import.bound); }


Same, this should include the names.

tlively · 2025-02-27T00:44:12Z

src/wasm/wasm-type.cpp

      return typer.isSubType(sub.struct_, super.struct_);
    case HeapTypeKind::Array:
      return typer.isSubType(sub.array, super.array);
+    case HeapTypeKind::Import:


Perhaps we should return false here. It seems possible for someone to accidentally set a supertype on an import entry in a TypeBuilder. We should also check whether the supertype is an import here.

tlively · 2025-02-27T00:46:24Z

src/wasm/wasm-type.cpp

+        if (!info.import.bound.isShared()) {
+          return TypeBuilder::ErrorReason::InvalidBoundType;
+        }
+        break;


It might make sense to just inherit sharedness from the bound rather than requiring it to be set separately and match. What do you think?

Indeed, we should do that.

tlively · 2025-02-27T01:08:01Z

test/lit/merge/type-imports.wat

+  (import "second" "g" (func $g (param (ref $t2))))
+
+  ;; Check that the function parameters are updated
+  (func (export "f") (param (ref $t1) (ref $t2) (ref $t3) (ref $t4))


If you give these functions $names, then the test output update script should be able to match them up with functions in the output and put them next to each other.

tlively · 2025-02-28T01:28:28Z

Please re-request review from me when you're ready :)

vouillon added 3 commits February 26, 2025 17:24

Add tests

48cc48e

Test updates

9e527ef

vouillon force-pushed the type-imports branch from f57a4d1 to 9e527ef Compare February 26, 2025 17:40

More test updates

7e56d35

vouillon force-pushed the type-imports branch from 56a545e to 7e56d35 Compare February 26, 2025 18:57

tlively reviewed Feb 27, 2025

View reviewed changes

vouillon added 9 commits February 27, 2025 20:05

Fix hashing and comparison

df60079

Fuzzing updates

f098ced

Validation fixes

fce76ff

Reuse the type-sorting code from ModuleUtils in wasm-merge

e316807

Comment updates

982ab07

Tests: name functions

6dfcdb9

Rename Import to TypeImport

22e093b

Unnecessary include

60bd789

Fix Subtypes::getMaxDepths

bc7c3f8

vouillon added 6 commits March 13, 2025 23:43

Merge remote-tracking branch 'origin/main' into type-imports

1541147

fix

7da566f

Put type exports with other exports

c989d27

Fix tool options

ad45a6d

format

aa1e4ae

Fix tool outputs

682ab0f

vouillon mentioned this pull request Mar 19, 2025

Implementing type imports and exports in Binaryen WebAssembly/proposal-type-imports#17

Open

	// Type imports: name, positions of type and import names.
	// Type imports: name, export names, import names, and type index.


		size_t hash(Continuation cont) { return hash(cont.type); }

		size_t hash(Import import) { return hash(import.bound); }

Comments

Conversation

vouillon commented Feb 26, 2025

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Feb 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants