Repository: tetratelabs/wazero
Pull request #283 (2022-02-23 10:25:55)

Backfills integration tests of WithName to ensure bug is fixed
Sorry about the bug


Patch:
diff --git a/wasi.go b/wasi.go
index 67f9a504..4263445b 100644
--- a/wasi.go
+++ b/wasi.go
@@ -94,15 +94,18 @@ func StartWASICommand(store wasm.Store, module *ModuleConfig) (wasm.ModuleExport
 	if !ok {
 		return nil, fmt.Errorf("unsupported Store implementation: %s", store)
 	}
-	m, name, err := decodeModule(module)
+	m, err := decodeModule(module)
 	if err != nil {
 		return nil, err
 	}
-	if err := internalwasi.ValidateWASICommand(m, name); err != nil {
+
+	moduleName := getModuleName(module.Name, m)
+
+	if err = internalwasi.ValidateWASICommand(m, moduleName); err != nil {
 		return nil, err
 	}
 
-	instantiated, err := internal.Instantiate(m, name)
+	instantiated, err := internal.Instantiate(m, moduleName)
 	if err != nil {
 		return nil, err
 	}
@@ -110,7 +113,7 @@ func StartWASICommand(store wasm.Store, module *ModuleConfig) (wasm.ModuleExport
 
 	start := ctx.Function(internalwasi.FunctionStart)
 	if _, err = start(ctx.Context()); err != nil {
-		return nil, fmt.Errorf("module[%s] function[%s] failed: %w", name, internalwasi.FunctionStart, err)
+		return nil, fmt.Errorf("module[%s] function[%s] failed: %w", moduleName, internalwasi.FunctionStart, err)
 	}
 	return instantiated, nil
 }
diff --git a/wasm.go b/wasm.go
index 5736cf8d..45cce225 100644
--- a/wasm.go
+++ b/wasm.go
@@ -74,7 +74,7 @@ type ModuleConfig struct {
 //
 // This is used to pre-flight check and cache the module for later instantiation.
 func (m *ModuleConfig) Validate() (err error) {
-	_, _, err = decodeModule(m)
+	_, err = decodeModule(m)
 	return err
 }
 
@@ -99,14 +99,22 @@ func InstantiateModule(store wasm.Store, module *ModuleConfig) (wasm.ModuleExpor
 	if !ok {
 		return nil, fmt.Errorf("unsupported Store implementation: %s", store)
 	}
-	m, name, err := decodeModule(module)
+	m, err := decodeModule(module)
 	if err != nil {
 		return nil, err
 	}
-	return internal.Instantiate(m, name)
+	return internal.Instantiate(m, getModuleName(module.Name, m))
 }
 
-func decodeModule(module *ModuleConfig) (m *internalwasm.Module, name string, err error) {
+// getModuleName returns the ModuleName from the internalwasm.NameSection if the input name was empty.
+func getModuleName(name string, m *internalwasm.Module) string {
+	if name == "" && m.NameSection != nil {
+		return m.NameSection.ModuleName
+	}
+	return name
+}
+
+func decodeModule(module *ModuleConfig) (m *internalwasm.Module, err error) {
 	if module.Source == nil {
 		err = errors.New("source == nil")
 		return
@@ -136,11 +144,6 @@ func decodeModule(module *ModuleConfig) (m *internalwasm.Module, name string, er
 	// Cache as tools like wapc-go re-instantiate the same module many times.
 	module.validatedSource = module.Source
 	module.decodedModule = m
-
-	name = module.Name
-	if name == "" && m.NameSection != nil {
-		name = m.NameSection.ModuleName
-	}
 	return
 }
 


Test patch:
diff --git a/wasm_test.go b/wasm_test.go
index b6b313fa..820d5573 100644
--- a/wasm_test.go
+++ b/wasm_test.go
@@ -8,6 +8,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	internalwasm "github.com/tetratelabs/wazero/internal/wasm"
 	"github.com/tetratelabs/wazero/internal/wasm/binary"
 	"github.com/tetratelabs/wazero/internal/wasm/text"
 	"github.com/tetratelabs/wazero/wasm"
@@ -35,13 +36,9 @@ func TestDecodeModule(t *testing.T) {
 
 		t.Run(tc.name, func(t *testing.T) {
 			config := &ModuleConfig{Name: tc.moduleName, Source: tc.source}
-			_, name, err := decodeModule(config)
+			_, err := decodeModule(config)
 			require.NoError(t, err)
-			if tc.moduleName == "" {
-				require.Equal(t, "test" /* from the text format */, name)
-			} else {
-				require.Equal(t, tc.moduleName, name)
-			}
+			require.Equal(t, tc.moduleName, config.Name)
 
 			// Avoid adding another test just to check Validate works
 			require.NoError(t, config.Validate())
@@ -50,35 +47,35 @@ func TestDecodeModule(t *testing.T) {
 
 	t.Run("caches repetitive decodes", func(t *testing.T) {
 		config := &ModuleConfig{Source: wat}
-		m, _, err := decodeModule(config)
+		m, err := decodeModule(config)
 		require.NoError(t, err)
 
-		again, _, err := decodeModule(config)
+		again, err := decodeModule(config)
 		require.NoError(t, err)
 		require.Same(t, m, again)
 
 		// Ensure config that only changes the name doesn't have to re-decode the source.
-		cloned, _, err := decodeModule(config.WithName("wazero"))
+		cloned, err := decodeModule(config.WithName("wazero"))
 		require.NoError(t, err)
 		require.Same(t, m, cloned)
 	})
 
 	t.Run("changing source invalidates decode cache", func(t *testing.T) {
 		config := &ModuleConfig{Source: wat}
-		m, _, err := decodeModule(config)
+		m, err := decodeModule(config)
 		require.NoError(t, err)
 
 		clonedConfig := config.WithName("wazero")
 
 		// When the source is changed, the module needs to be decoded again
 		config.Source = wasm
-		again, _, err := decodeModule(config)
+		again, err := decodeModule(config)
 		require.NoError(t, err)
 		require.Equal(t, m, again)
 		require.NotSame(t, m, again)
 
 		// Any copies of the config shouldn't be invalidated
-		cloned, _, err := decodeModule(clonedConfig)
+		cloned, err := decodeModule(clonedConfig)
 		require.NoError(t, err)
 		require.Same(t, m, cloned)
 	})
@@ -111,7 +108,7 @@ func TestDecodeModule_Errors(t *testing.T) {
 
 		t.Run(tc.name, func(t *testing.T) {
 			config := &ModuleConfig{Source: tc.source}
-			_, _, err := decodeModule(config)
+			_, err := decodeModule(config)
 			require.EqualError(t, err, tc.expectedErr)
 
 			// Avoid adding another test just to check Validate works
@@ -120,6 +117,22 @@ func TestDecodeModule_Errors(t *testing.T) {
 	}
 }
 
+// TestInstantiateModule_Renamed tests that we can pre-validate (cache) a module and instantiate it under different
+// names. This pattern is used in wapc-go.
+func TestInstantiateModule_Renamed(t *testing.T) {
+	base := &ModuleConfig{Source: []byte(`(module (memory 1))`)}
+	require.NoError(t, base.Validate()) // cache
+
+	// Use the same store to instantiate multiple modules
+	store := NewStore()
+
+	_, err := InstantiateModule(store, base.WithName("1"))
+	require.NoError(t, err)
+
+	_, err = InstantiateModule(store, base.WithName("2"))
+	require.NoError(t, err)
+}
+
 // TestModuleExports_Memory only covers a couple cases to avoid duplication of internal/wasm/store_test.go
 func TestModuleExports_Memory(t *testing.T) {
 	tests := []struct {
@@ -243,3 +256,52 @@ func requireImportAndExportFunction(t *testing.T, store wasm.Store, hostFn func(
 		`(module (import "host" "%[1]s" (func (result i64))) (export "%[1]s" (func 0)))`, functionName,
 	))
 }
+
+func TestGetModuleName(t *testing.T) {
+	tests := []struct {
+		name       string
+		moduleName string
+		module     *internalwasm.Module
+		expected   string
+	}{
+		{
+			name:   "empty on nil name section",
+			module: &internalwasm.Module{},
+		},
+		{
+			name:   "empty on empty NameSection.ModuleName",
+			module: &internalwasm.Module{NameSection: &internalwasm.NameSection{}},
+		},
+		{
+			name:     "NameSection.ModuleName",
+			module:   &internalwasm.Module{NameSection: &internalwasm.NameSection{ModuleName: "test"}},
+			expected: "test",
+		},
+		{
+			name:       "overrides nil name section",
+			moduleName: "wazero",
+			module:     &internalwasm.Module{},
+			expected:   "wazero",
+		},
+		{
+			name:       "overrides empty NameSection.ModuleName",
+			moduleName: "wazero",
+			module:     &internalwasm.Module{NameSection: &internalwasm.NameSection{}},
+			expected:   "wazero",
+		},
+		{
+			name:       "overrides NameSection.ModuleName",
+			moduleName: "wazero",
+			module:     &internalwasm.Module{NameSection: &internalwasm.NameSection{ModuleName: "test"}},
+			expected:   "wazero",
+		},
+	}
+
+	for _, tt := range tests {
+		tc := tt
+
+		t.Run(tc.name, func(t *testing.T) {
+			require.Equal(t, tc.expected, getModuleName(tc.moduleName, tc.module))
+		})
+	}
+}