Repository: tetratelabs/wazero
Pull request #519 (2022-05-02 02:50:55)

Makes wazero.CompiledCode an interface instead of a struct
This makes wazero.CompiledCode an interface instead of a struct to
prevent it from being used incorrectly. For example, even though the
fields are not exported, someone can mistakenly instantiate this
when it is a struct, and in doing so violate internal assumptions.

Follow-up from #518 and the next and final PR will be ModuleConfig.


Patch:
diff --git a/builder.go b/builder.go
index 7432df20..724fa638 100644
--- a/builder.go
+++ b/builder.go
@@ -153,7 +153,7 @@ type ModuleBuilder interface {
 	ExportGlobalF64(name string, v float64) ModuleBuilder
 
 	// Build returns a module to instantiate, or returns an error if any of the configuration is invalid.
-	Build(context.Context) (*CompiledCode, error)
+	Build(context.Context) (CompiledCode, error)
 
 	// Instantiate is a convenience that calls Build, then Runtime.InstantiateModule
 	//
@@ -250,7 +250,7 @@ func (b *moduleBuilder) ExportGlobalF64(name string, v float64) ModuleBuilder {
 }
 
 // Build implements ModuleBuilder.Build
-func (b *moduleBuilder) Build(ctx context.Context) (*CompiledCode, error) {
+func (b *moduleBuilder) Build(ctx context.Context) (CompiledCode, error) {
 	// Verify the maximum limit here, so we don't have to pass it to wasm.NewHostModule
 	memoryLimitPages := b.r.memoryLimitPages
 	for name, mem := range b.nameToMemory {
@@ -271,19 +271,19 @@ func (b *moduleBuilder) Build(ctx context.Context) (*CompiledCode, error) {
 		return nil, err
 	}
 
-	return &CompiledCode{module: module, compiledEngine: b.r.store.Engine}, nil
+	return &compiledCode{module: module, compiledEngine: b.r.store.Engine}, nil
 }
 
 // Instantiate implements ModuleBuilder.Instantiate
 func (b *moduleBuilder) Instantiate(ctx context.Context) (api.Module, error) {
-	if module, err := b.Build(ctx); err != nil {
+	if compiled, err := b.Build(ctx); err != nil {
 		return nil, err
 	} else {
-		if err = b.r.store.Engine.CompileModule(ctx, module.module); err != nil {
+		if err = b.r.store.Engine.CompileModule(ctx, compiled.(*compiledCode).module); err != nil {
 			return nil, err
 		}
 		// *wasm.ModuleInstance cannot be tracked, so we release the cache inside this function.
-		defer module.Close(ctx)
-		return b.r.InstantiateModuleWithConfig(ctx, module, NewModuleConfig().WithName(b.moduleName))
+		defer compiled.Close(ctx)
+		return b.r.InstantiateModuleWithConfig(ctx, compiled, NewModuleConfig().WithName(b.moduleName))
 	}
 }
diff --git a/config.go b/config.go
index f7aa199e..4cd796ab 100644
--- a/config.go
+++ b/config.go
@@ -238,22 +238,27 @@ func (c *runtimeConfig) WithWasmCore2() RuntimeConfig {
 	return ret
 }
 
-// CompiledCode is a WebAssembly 1.0 (20191205) module ready to be instantiated (Runtime.InstantiateModule) as an\
+// CompiledCode is a WebAssembly 1.0 (20191205) module ready to be instantiated (Runtime.InstantiateModule) as an
 // api.Module.
 //
 // Note: In WebAssembly language, this is a decoded, validated, and possibly also compiled module. wazero avoids using
 // the name "Module" for both before and after instantiation as the name conflation has caused confusion.
 // See https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#semantic-phases%E2%91%A0
-type CompiledCode struct {
+type CompiledCode interface {
+	// Close releases all the allocated resources for this CompiledCode.
+	//
+	// Note: It is safe to call Close while having outstanding calls from Modules instantiated from this CompiledCode.
+	Close(context.Context) error
+}
+
+type compiledCode struct {
 	module *wasm.Module
 	// compiledEngine holds an engine on which `module` is compiled.
 	compiledEngine wasm.Engine
 }
 
-// Close releases all the allocated resources for this CompiledCode.
-//
-// Note: It is safe to call Close while having outstanding calls from Modules instantiated from this *CompiledCode.
-func (c *CompiledCode) Close(_ context.Context) error {
+// Close implements CompiledCode.Close
+func (c *compiledCode) Close(_ context.Context) error {
 	// Note: If you use the context.Context param, don't forget to coerce nil to context.Background()!
 
 	c.compiledEngine.DeleteCompiledModule(c.module)
diff --git a/wasm.go b/wasm.go
index 7296d012..9d1ca33a 100644
--- a/wasm.go
+++ b/wasm.go
@@ -49,7 +49,7 @@ type Runtime interface {
 	// Note: When the context is nil, it defaults to context.Background.
 	// Note: The resulting module name defaults to what was binary from the custom name section.
 	// See https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#name-section%E2%91%A0
-	CompileModule(ctx context.Context, source []byte) (*CompiledCode, error)
+	CompileModule(ctx context.Context, source []byte) (CompiledCode, error)
 
 	// InstantiateModuleFromCode instantiates a module from the WebAssembly 1.0 (20191205) text or binary source or
 	// errs if invalid.
@@ -94,7 +94,7 @@ type Runtime interface {
 	//  * The module has a start function, and it failed to execute.
 	//
 	// Note: When the context is nil, it defaults to context.Background.
-	InstantiateModule(ctx context.Context, compiled *CompiledCode) (api.Module, error)
+	InstantiateModule(ctx context.Context, compiled CompiledCode) (api.Module, error)
 
 	// InstantiateModuleWithConfig is like InstantiateModule, except you can override configuration such as the module
 	// name or ENV variables.
@@ -114,7 +114,7 @@ type Runtime interface {
 	//
 	// Note: When the context is nil, it defaults to context.Background.
 	// Note: Config is copied during instantiation: Later changes to config do not affect the instantiated result.
-	InstantiateModuleWithConfig(ctx context.Context, compiled *CompiledCode, config *ModuleConfig) (mod api.Module, err error)
+	InstantiateModuleWithConfig(ctx context.Context, compiled CompiledCode, config *ModuleConfig) (mod api.Module, err error)
 }
 
 func NewRuntime() Runtime {
@@ -149,7 +149,7 @@ func (r *runtime) Module(moduleName string) api.Module {
 }
 
 // CompileModule implements Runtime.CompileModule
-func (r *runtime) CompileModule(ctx context.Context, source []byte) (*CompiledCode, error) {
+func (r *runtime) CompileModule(ctx context.Context, source []byte) (CompiledCode, error) {
 	if source == nil {
 		return nil, errors.New("source == nil")
 	}
@@ -202,7 +202,7 @@ func (r *runtime) CompileModule(ctx context.Context, source []byte) (*CompiledCo
 		return nil, err
 	}
 
-	return &CompiledCode{module: internal, compiledEngine: r.store.Engine}, nil
+	return &compiledCode{module: internal, compiledEngine: r.store.Engine}, nil
 }
 
 // InstantiateModuleFromCode implements Runtime.InstantiateModuleFromCode
@@ -228,23 +228,28 @@ func (r *runtime) InstantiateModuleFromCodeWithConfig(ctx context.Context, sourc
 }
 
 // InstantiateModule implements Runtime.InstantiateModule
-func (r *runtime) InstantiateModule(ctx context.Context, compiled *CompiledCode) (mod api.Module, err error) {
+func (r *runtime) InstantiateModule(ctx context.Context, compiled CompiledCode) (mod api.Module, err error) {
 	return r.InstantiateModuleWithConfig(ctx, compiled, NewModuleConfig())
 }
 
 // InstantiateModuleWithConfig implements Runtime.InstantiateModuleWithConfig
-func (r *runtime) InstantiateModuleWithConfig(ctx context.Context, compiled *CompiledCode, config *ModuleConfig) (mod api.Module, err error) {
+func (r *runtime) InstantiateModuleWithConfig(ctx context.Context, compiled CompiledCode, config *ModuleConfig) (mod api.Module, err error) {
 	var sysCtx *wasm.SysContext
 	if sysCtx, err = config.toSysContext(); err != nil {
 		return
 	}
 
+	code, ok := compiled.(*compiledCode)
+	if !ok {
+		panic(fmt.Errorf("unsupported wazero.CompiledCode implementation: %#v", compiled))
+	}
+
 	name := config.name
-	if name == "" && compiled.module.NameSection != nil && compiled.module.NameSection.ModuleName != "" {
-		name = compiled.module.NameSection.ModuleName
+	if name == "" && code.module.NameSection != nil && code.module.NameSection.ModuleName != "" {
+		name = code.module.NameSection.ModuleName
 	}
 
-	module := config.replaceImports(compiled.module)
+	module := config.replaceImports(code.module)
 
 	var functionListenerFactory experimentalapi.FunctionListenerFactory
 	if ctx != nil { // Test to see if internal code are using an experimental feature.


Test patch:
diff --git a/builder_test.go b/builder_test.go
index bb250f44..5cc13a6a 100644
--- a/builder_test.go
+++ b/builder_test.go
@@ -344,8 +344,9 @@ func TestNewModuleBuilder_Build(t *testing.T) {
 
 		t.Run(tc.name, func(t *testing.T) {
 			b := tc.input(NewRuntime()).(*moduleBuilder)
-			m, err := b.Build(testCtx)
+			compiled, err := b.Build(testCtx)
 			require.NoError(t, err)
+			m := compiled.(*compiledCode)
 
 			requireHostModuleEquals(t, tc.expected, m.module)
 
diff --git a/config_test.go b/config_test.go
index dcc77cf5..3523190d 100644
--- a/config_test.go
+++ b/config_test.go
@@ -853,12 +853,12 @@ func TestCompiledCode_Close(t *testing.T) {
 	for _, ctx := range []context.Context{nil, testCtx} { // Ensure it doesn't crash on nil!
 		e := &mockEngine{name: "1", cachedModules: map[*wasm.Module]struct{}{}}
 
-		var cs []*CompiledCode
+		var cs []*compiledCode
 		for i := 0; i < 10; i++ {
 			m := &wasm.Module{}
 			err := e.CompileModule(ctx, m)
 			require.NoError(t, err)
-			cs = append(cs, &CompiledCode{module: m, compiledEngine: e})
+			cs = append(cs, &compiledCode{module: m, compiledEngine: e})
 		}
 
 		// Before Close.
diff --git a/internal/integration_test/engine/adhoc_test.go b/internal/integration_test/engine/adhoc_test.go
index b7e5f16c..766ca4c7 100644
--- a/internal/integration_test/engine/adhoc_test.go
+++ b/internal/integration_test/engine/adhoc_test.go
@@ -364,7 +364,7 @@ func testCloseInFlight(t *testing.T, r wazero.Runtime) {
 		tc := tt
 
 		t.Run(tc.name, func(t *testing.T) {
-			var importingCode, importedCode *wazero.CompiledCode
+			var importingCode, importedCode wazero.CompiledCode
 			var imported, importing api.Module
 			var err error
 			closeAndReturn := func(ctx context.Context, x uint32) uint32 {
diff --git a/internal/integration_test/vs/runtime.go b/internal/integration_test/vs/runtime.go
index e70d3f91..7a7cd796 100644
--- a/internal/integration_test/vs/runtime.go
+++ b/internal/integration_test/vs/runtime.go
@@ -54,7 +54,7 @@ type wazeroRuntime struct {
 	config        wazero.RuntimeConfig
 	runtime       wazero.Runtime
 	logFn         func([]byte) error
-	env, compiled *wazero.CompiledCode
+	env, compiled wazero.CompiledCode
 }
 
 type wazeroModule struct {
diff --git a/wasm_test.go b/wasm_test.go
index b6a4c49b..58d6ef14 100644
--- a/wasm_test.go
+++ b/wasm_test.go
@@ -19,7 +19,7 @@ import (
 var testCtx = context.WithValue(context.Background(), struct{}{}, "arbitrary")
 
 func TestNewRuntimeWithConfig_PanicsOnWrongImpl(t *testing.T) {
-	// It is too burdensome to define an impl of RuntimeConfig in tests just to verify the error when it is wrong.
+	// It causes maintenance to define an impl of RuntimeConfig in tests just to verify the error when it is wrong.
 	// Instead, we pass nil which is implicitly the wrong type, as that's less work!
 	err := require.CapturePanic(func() {
 		NewRuntimeWithConfig(nil)
@@ -68,8 +68,9 @@ func TestRuntime_CompileModule(t *testing.T) {
 		tc := tt
 
 		t.Run(tc.name, func(t *testing.T) {
-			code, err := r.CompileModule(testCtx, tc.source)
+			m, err := r.CompileModule(testCtx, tc.source)
 			require.NoError(t, err)
+			code := m.(*compiledCode)
 			defer code.Close(testCtx)
 			if tc.expectedName != "" {
 				require.Equal(t, tc.expectedName, code.module.NameSection.ModuleName)
@@ -84,8 +85,9 @@ func TestRuntime_CompileModule(t *testing.T) {
 
 		source := []byte(`(module (memory 1 3))`)
 
-		code, err := r.CompileModule(testCtx, source)
+		m, err := r.CompileModule(testCtx, source)
 		require.NoError(t, err)
+		code := m.(*compiledCode)
 		defer code.Close(testCtx)
 
 		require.Equal(t, &wasm.Memory{
@@ -312,12 +314,13 @@ func TestModule_Global(t *testing.T) {
 
 		r := NewRuntime().(*runtime)
 		t.Run(tc.name, func(t *testing.T) {
-			var code *CompiledCode
+			var m CompiledCode
 			if tc.module != nil {
-				code = &CompiledCode{module: tc.module}
+				m = &compiledCode{module: tc.module}
 			} else {
-				code, _ = tc.builder(r).Build(testCtx)
+				m, _ = tc.builder(r).Build(testCtx)
 			}
+			code := m.(*compiledCode)
 
 			err := r.store.Engine.CompileModule(testCtx, code.module)
 			require.NoError(t, err)
@@ -463,6 +466,17 @@ func TestRuntime_InstantiateModuleFromCode_UsesContext(t *testing.T) {
 	require.True(t, calledStart)
 }
 
+func TestInstantiateModuleWithConfig_PanicsOnWrongCompiledCodeImpl(t *testing.T) {
+	// It causes maintenance to define an impl of CompiledCode in tests just to verify the error when it is wrong.
+	// Instead, we pass nil which is implicitly the wrong type, as that's less work!
+	r := NewRuntime()
+	err := require.CapturePanic(func() {
+		_, _ = r.InstantiateModuleWithConfig(testCtx, nil, NewModuleConfig())
+	})
+
+	require.EqualError(t, err, "unsupported wazero.CompiledCode implementation: <nil>")
+}
+
 // TestInstantiateModuleWithConfig_WithName tests that we can pre-validate (cache) a module and instantiate it under
 // different names. This pattern is used in wapc-go.
 func TestInstantiateModuleWithConfig_WithName(t *testing.T) {
@@ -471,7 +485,7 @@ func TestInstantiateModuleWithConfig_WithName(t *testing.T) {
 	require.NoError(t, err)
 	defer base.Close(testCtx)
 
-	require.Equal(t, "0", base.module.NameSection.ModuleName)
+	require.Equal(t, "0", base.(*compiledCode).module.NameSection.ModuleName)
 
 	// Use the same runtime to instantiate multiple modules
 	internal := r.(*runtime).store