Repository: tetratelabs/wazero
Pull request #1452 (2023-05-10 16:32:46)

experimental: remove error parameter from FunctionListener.After
Following up on our discussion in #wazero-dev, I would like to suggest we remove the `error` parameter from `FunctionListener.After` since it is currently unused: wazero always passes the value `nil`.

I believe this change is worth it so users of `FunctionListener` do not have to concern themselves with understanding how to deal with the parameter nor write tests for code paths that will never be taken. It also means that we do not need to write documentation explaining why the parameter exists, and why it's currently unused.

Since this is still an experimental API, the breaking change should be acceptable, as well as considering bringing back a form of error handling in the future when we do have the ability to propagate errors to function listeners.

---

As part of this PR, I also made a few aesthetic changes:
- I renamed `paramValues` and `resultValues` to `params` and `results` since those were terms that were much more common within wazero to represent parameters and result values of function calls
- I modified the internal implementation of the logging listener's `logIndented` method to accept a function parameter instead of a boolean in order to configure which of the `logParams` or `logResults` methods gets called

---

Please take a look and let me know if you would like to see anything changed!


Patch:
diff --git a/experimental/listener.go b/experimental/listener.go
index c1193b59..ecf89bd5 100644
--- a/experimental/listener.go
+++ b/experimental/listener.go
@@ -55,14 +55,14 @@ type FunctionListener interface {
 	//	   instance or parent of the result.
 	//   - mod: the calling module.
 	//   - def: the function definition.
-	//   - paramValues:  api.ValueType encoded parameters.
+	//   - params:  api.ValueType encoded parameters.
 	//   - stackIterator: iterator on the call stack. At least one entry is
 	//     guaranteed (the called function), whose Args() will be equal to
-	//     paramValues. The iterator will be reused between calls to Before.
+	//     params. The iterator will be reused between calls to Before.
 	//
 	// Note: api.Memory is meant for inspection, not modification.
 	// mod can be cast to InternalModule to read non-exported globals.
-	Before(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, stackIterator StackIterator) context.Context
+	Before(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, stackIterator StackIterator) context.Context
 
 	// After is invoked after a function is called.
 	//
@@ -71,11 +71,10 @@ type FunctionListener interface {
 	//   - ctx: the context returned by Before.
 	//   - mod: the calling module.
 	//   - def: the function definition.
-	//   - err: nil if the function didn't err
-	//   - resultValues: api.ValueType encoded results.
+	//   - results: api.ValueType encoded results.
 	//
 	// Note: api.Memory is meant for inspection, not modification.
-	After(ctx context.Context, mod api.Module, def api.FunctionDefinition, err error, resultValues []uint64)
+	After(ctx context.Context, mod api.Module, def api.FunctionDefinition, results []uint64)
 }
 
 // FunctionListenerFunc is a function type implementing the FunctionListener
@@ -89,14 +88,14 @@ type FunctionListener interface {
 type FunctionListenerFunc func(context.Context, api.Module, api.FunctionDefinition, []uint64, StackIterator)
 
 // Before satisfies the FunctionListener interface, calls f.
-func (f FunctionListenerFunc) Before(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, stackIterator StackIterator) context.Context {
-	f(ctx, mod, def, paramValues, stackIterator)
+func (f FunctionListenerFunc) Before(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, stackIterator StackIterator) context.Context {
+	f(ctx, mod, def, params, stackIterator)
 	return ctx
 }
 
 // After is declared to satisfy the FunctionListener interface, but it does
 // nothing.
-func (f FunctionListenerFunc) After(context.Context, api.Module, api.FunctionDefinition, error, []uint64) {
+func (f FunctionListenerFunc) After(context.Context, api.Module, api.FunctionDefinition, []uint64) {
 }
 
 // FunctionListenerFactoryFunc is a function type implementing the
@@ -159,9 +158,9 @@ func (multi *multiFunctionListener) Before(ctx context.Context, mod api.Module,
 	return ctx
 }
 
-func (multi *multiFunctionListener) After(ctx context.Context, mod api.Module, def api.FunctionDefinition, err error, results []uint64) {
+func (multi *multiFunctionListener) After(ctx context.Context, mod api.Module, def api.FunctionDefinition, results []uint64) {
 	for _, lstn := range multi.lstns {
-		lstn.After(ctx, mod, def, err, results)
+		lstn.After(ctx, mod, def, results)
 	}
 }
 
@@ -312,16 +311,16 @@ func BenchmarkFunctionListener(n int, module api.Module, stack []StackFrame, lis
 		panic("cannot benchmark function listener with an empty stack")
 	}
 
-	functionDefinition := stack[0].Function.Definition()
-	functionParams := stack[0].Params
-	functionResults := stack[0].Results
-	stackIterator := &stackIterator{base: NewStackIterator(stack...)}
 	ctx := context.Background()
+	def := stack[0].Function.Definition()
+	params := stack[0].Params
+	results := stack[0].Results
+	stackIterator := &stackIterator{base: NewStackIterator(stack...)}
 
 	for i := 0; i < n; i++ {
 		stackIterator.index = -1
-		callContext := listener.Before(ctx, module, functionDefinition, functionParams, stackIterator)
-		listener.After(callContext, module, functionDefinition, nil, functionResults)
+		callContext := listener.Before(ctx, module, def, params, stackIterator)
+		listener.After(callContext, module, def, results)
 	}
 }
 
@@ -329,8 +328,6 @@ func BenchmarkFunctionListener(n int, module api.Module, stack []StackFrame, lis
 // indirect in terms of depth and breadth. The test could show a tree 3 calls deep where the there are a couple calls at
 // each depth under the root. The main thing this can help prevent is accidentally swapping the context internally.
 
-// TODO: Errors aren't handled, and the After hook should accept one along with the result values.
-
 // TODO: The context parameter of the After hook is not the same as the Before hook. This means interceptor patterns
 // are awkward. e.g. something like timing is difficult as it requires propagating a stack. Otherwise, nested calls will
 // overwrite each other's "since" time. Propagating a stack is further awkward as the After hook needs to know the
diff --git a/experimental/logging/log_listener.go b/experimental/logging/log_listener.go
index 51452f86..7e2aecc7 100644
--- a/experimental/logging/log_listener.go
+++ b/experimental/logging/log_listener.go
@@ -203,7 +203,9 @@ func (l *loggingListener) Before(ctx context.Context, mod api.Module, _ api.Func
 	// We're starting to log: increase the indentation level.
 	nestLevel++
 
-	l.logIndented(ctx, mod, nestLevel, true, params, nil, nil)
+	l.logIndented(nestLevel, l.beforePrefix, func() {
+		l.logParams(ctx, mod, params)
+	})
 
 	// We need to propagate this invocation's parameters to the after callback.
 	state = &logState{w: l.w, nestLevel: nestLevel}
@@ -220,34 +222,27 @@ func (l *loggingListener) Before(ctx context.Context, mod api.Module, _ api.Func
 
 // After logs to stdout the module and function name, prefixed with '<--' and
 // indented based on the call nesting level.
-func (l *loggingListener) After(ctx context.Context, mod api.Module, _ api.FunctionDefinition, err error, results []uint64) {
+func (l *loggingListener) After(ctx context.Context, mod api.Module, _ api.FunctionDefinition, results []uint64) {
 	// Note: We use the nest level directly even though it is the "next" nesting level.
 	// This works because our indent of zero nesting is one tab.
 	if state, ok := ctx.Value(logging.LoggerKey{}).(*logState); ok {
 		if state == unsampledLogState {
 			return
 		}
-		l.logIndented(ctx, mod, state.nestLevel, false, state.params, err, results)
+		l.logIndented(state.nestLevel, l.afterPrefix, func() {
+			l.logResults(ctx, mod, state.params, results)
+		})
 	}
 }
 
-// logIndented logs an indented l.w like this: "-->\t\t\t$nestLevel$funcName\n"
-func (l *loggingListener) logIndented(ctx context.Context, mod api.Module, nestLevel int, isBefore bool, params []uint64, err error, results []uint64) {
+// logIndented writes an indentation level and prefix prior to calling log to
+// output the log line.
+func (l *loggingListener) logIndented(nestLevel int, prefix string, log func()) {
 	for i := 1; i < nestLevel; i++ {
 		l.w.WriteByte('\t') //nolint
 	}
-	if isBefore { // before
-		l.w.WriteString(l.beforePrefix) //nolint
-		l.logParams(ctx, mod, params)
-	} else { // after
-		l.w.WriteString(l.afterPrefix) //nolint
-		if err != nil {
-			l.w.WriteString(" error: ")  //nolint
-			l.w.WriteString(err.Error()) //nolint
-		} else {
-			l.logResults(ctx, mod, params, results)
-		}
-	}
+	l.w.WriteString(prefix) //nolint
+	log()
 	l.w.WriteByte('\n') //nolint
 
 	if f, ok := l.w.(flusher); ok {
diff --git a/internal/engine/compiler/engine.go b/internal/engine/compiler/engine.go
index c848c2c0..a6ec03f9 100644
--- a/internal/engine/compiler/engine.go
+++ b/internal/engine/compiler/engine.go
@@ -1183,7 +1183,7 @@ func (ce *callEngine) builtinFunctionFunctionListenerBefore(ctx context.Context,
 
 func (ce *callEngine) builtinFunctionFunctionListenerAfter(ctx context.Context, mod api.Module, fn *function) {
 	base := int(ce.stackBasePointerInBytes >> 3)
-	fn.parent.listener.After(ctx, mod, fn.definition(), nil, ce.stack[base:base+fn.funcType.ResultNumInUint64])
+	fn.parent.listener.After(ctx, mod, fn.definition(), ce.stack[base:base+fn.funcType.ResultNumInUint64])
 
 	i := len(ce.contextStack) - 1
 	ce.ctx = ce.contextStack[i]
diff --git a/internal/engine/interpreter/interpreter.go b/internal/engine/interpreter/interpreter.go
index fd2233c2..c25bcc9a 100644
--- a/internal/engine/interpreter/interpreter.go
+++ b/internal/engine/interpreter/interpreter.go
@@ -608,7 +608,7 @@ func (ce *callEngine) callGoFunc(ctx context.Context, m *wasm.ModuleInstance, f
 	if lsn != nil {
 		// TODO: This doesn't get the error due to use of panic to propagate them.
 		results := stack[:typ.ResultNumInUint64]
-		lsn.After(ctx, m, f.definition(), nil, results)
+		lsn.After(ctx, m, f.definition(), results)
 	}
 }
 
@@ -4087,7 +4087,7 @@ func (ce *callEngine) callNativeFuncWithListener(ctx context.Context, m *wasm.Mo
 	ctx = fnl.Before(ctx, m, def, ce.peekValues(len(typ.Params)), &ce.stackIterator)
 	ce.stackIterator.clear()
 	ce.callNativeFunc(ctx, m, f)
-	fnl.After(ctx, m, def, nil, ce.peekValues(len(typ.Results)))
+	fnl.After(ctx, m, def, ce.peekValues(len(typ.Results)))
 	return ctx
 }
 


Test patch:
diff --git a/experimental/listener_example_test.go b/experimental/listener_example_test.go
index 38f528e7..6fc48844 100644
--- a/experimental/listener_example_test.go
+++ b/experimental/listener_example_test.go
@@ -50,7 +50,7 @@ func (u uniqGoFuncs) Before(ctx context.Context, _ api.Module, def api.FunctionD
 }
 
 // After implements FunctionListener.After
-func (u uniqGoFuncs) After(context.Context, api.Module, api.FunctionDefinition, error, []uint64) {}
+func (u uniqGoFuncs) After(context.Context, api.Module, api.FunctionDefinition, []uint64) {}
 
 // This shows how to make a listener that counts go function calls.
 func Example_customListenerFactory() {
diff --git a/experimental/listener_test.go b/experimental/listener_test.go
index 8555f6f1..55a8aa5c 100644
--- a/experimental/listener_test.go
+++ b/experimental/listener_test.go
@@ -27,7 +27,7 @@ func (r *recorder) Before(ctx context.Context, _ api.Module, def api.FunctionDef
 	return ctx
 }
 
-func (r *recorder) After(_ context.Context, _ api.Module, def api.FunctionDefinition, _ error, _ []uint64) {
+func (r *recorder) After(_ context.Context, _ api.Module, def api.FunctionDefinition, _ []uint64) {
 	r.afterNames = append(r.afterNames, def.DebugName())
 }
 
@@ -116,7 +116,7 @@ func TestMultiFunctionListenerFactory(t *testing.T) {
 	}
 
 	n := 0
-	f := func(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, stackIterator experimental.StackIterator) {
+	f := func(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, stackIterator experimental.StackIterator) {
 		n++
 		i := 0
 		for stackIterator.Next() {
@@ -180,12 +180,12 @@ func BenchmarkMultiFunctionListener(b *testing.B) {
 	}{
 		{
 			scenario: "simple function listener",
-			function: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, stackIterator experimental.StackIterator) {
+			function: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, stackIterator experimental.StackIterator) {
 			},
 		},
 		{
 			scenario: "stack iterator",
-			function: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, stackIterator experimental.StackIterator) {
+			function: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, stackIterator experimental.StackIterator) {
 				for stackIterator.Next() {
 				}
 			},
diff --git a/experimental/logging/log_listener_test.go b/experimental/logging/log_listener_test.go
index 8417f56f..81580bef 100644
--- a/experimental/logging/log_listener_test.go
+++ b/experimental/logging/log_listener_test.go
@@ -3,7 +3,6 @@ package logging_test
 import (
 	"bytes"
 	"context"
-	"io"
 	"math"
 	"testing"
 
@@ -36,7 +35,6 @@ func Test_loggingListener(t *testing.T) {
 		isHostFunc              bool
 		paramNames, resultNames []string
 		params, results         []uint64
-		err                     error
 		expected                string
 	}{
 		{
@@ -44,14 +42,6 @@ func Test_loggingListener(t *testing.T) {
 			functype: wasm.FunctionType{},
 			expected: `--> test.fn()
 <--
-`,
-		},
-		{
-			name:     "error",
-			functype: wasm.FunctionType{},
-			err:      io.EOF,
-			expected: `--> test.fn()
-<-- error: EOF
 `,
 		},
 		{
@@ -88,20 +78,6 @@ func Test_loggingListener(t *testing.T) {
 			results:     []uint64{uint64(wasi.ErrnoFault)},
 			expected: `==> wasi_snapshot_preview1.random_get(buf=0,buf_len=8)
 <== errno=EFAULT
-`,
-		},
-		{
-			name:        "wasi error",
-			functype:    wasiFuncType,
-			moduleName:  wasi.InternalModuleName,
-			funcName:    wasiFuncName,
-			paramNames:  wasiParamNames,
-			resultNames: wasiResultNames,
-			isHostFunc:  true,
-			params:      wasiParams,
-			err:         io.EOF, // not possible as we coerce errors to numbers, but test anyway!
-			expected: `==> wasi_snapshot_preview1.random_get(buf=0,buf_len=8)
-<== error: EOF
 `,
 		},
 		{
@@ -304,7 +280,7 @@ func Test_loggingListener(t *testing.T) {
 
 			out.Reset()
 			ctx := l.Before(testCtx, nil, def, tc.params, nil)
-			l.After(ctx, nil, def, tc.err, tc.results)
+			l.After(ctx, nil, def, tc.results)
 			require.Equal(t, tc.expected, out.String())
 		})
 	}
@@ -341,8 +317,8 @@ func Test_loggingListener_indentation(t *testing.T) {
 
 	ctx := l1.Before(testCtx, nil, def1, []uint64{}, nil)
 	ctx1 := l2.Before(ctx, nil, def2, []uint64{}, nil)
-	l2.After(ctx1, nil, def2, nil, []uint64{})
-	l1.After(ctx, nil, def1, nil, []uint64{})
+	l2.After(ctx1, nil, def2, []uint64{})
+	l1.After(ctx, nil, def1, []uint64{})
 	require.Equal(t, `--> test.fn1()
 	--> test.fn2()
 	<--
diff --git a/internal/engine/compiler/engine_bench_test.go b/internal/engine/compiler/engine_bench_test.go
index 23ca605a..c3b6fc06 100644
--- a/internal/engine/compiler/engine_bench_test.go
+++ b/internal/engine/compiler/engine_bench_test.go
@@ -17,7 +17,7 @@ func BenchmarkCallEngine_builtinFunctionFunctionListener(b *testing.B) {
 				before: func(context.Context, api.Module, api.FunctionDefinition, []uint64, experimental.StackIterator) context.Context {
 					return context.Background()
 				},
-				after: func(context.Context, api.Module, api.FunctionDefinition, error, []uint64) {
+				after: func(context.Context, api.Module, api.FunctionDefinition, []uint64) {
 				},
 			},
 			index: 0,
diff --git a/internal/engine/compiler/engine_test.go b/internal/engine/compiler/engine_test.go
index cb015753..3b081e74 100644
--- a/internal/engine/compiler/engine_test.go
+++ b/internal/engine/compiler/engine_test.go
@@ -589,9 +589,9 @@ func TestCallEngine_builtinFunctionFunctionListenerBefore(t *testing.T) {
 		funcType: &wasm.FunctionType{ParamNumInUint64: 3},
 		parent: &compiledFunction{
 			listener: mockListener{
-				before: func(ctx context.Context, _ api.Module, def api.FunctionDefinition, paramValues []uint64, stackIterator experimental.StackIterator) context.Context {
+				before: func(ctx context.Context, _ api.Module, def api.FunctionDefinition, params []uint64, stackIterator experimental.StackIterator) context.Context {
 					require.Equal(t, currentContext, ctx)
-					require.Equal(t, []uint64{2, 3, 4}, paramValues)
+					require.Equal(t, []uint64{2, 3, 4}, params)
 					assertStackIterator(t, stackIterator, []stackEntry{{def: def, args: []uint64{2, 3, 4}}})
 					return nextContext
 				},
@@ -620,9 +620,9 @@ func TestCallEngine_builtinFunctionFunctionListenerAfter(t *testing.T) {
 		funcType: &wasm.FunctionType{ResultNumInUint64: 1},
 		parent: &compiledFunction{
 			listener: mockListener{
-				after: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, err error, resultValues []uint64) {
+				after: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, results []uint64) {
 					require.Equal(t, currentContext, ctx)
-					require.Equal(t, []uint64{5}, resultValues)
+					require.Equal(t, []uint64{5}, results)
 				},
 			},
 			index: 0,
@@ -645,16 +645,16 @@ func TestCallEngine_builtinFunctionFunctionListenerAfter(t *testing.T) {
 }
 
 type mockListener struct {
-	before func(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, stackIterator experimental.StackIterator) context.Context
-	after  func(ctx context.Context, mod api.Module, def api.FunctionDefinition, err error, resultValues []uint64)
+	before func(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, stackIterator experimental.StackIterator) context.Context
+	after  func(ctx context.Context, mod api.Module, def api.FunctionDefinition, results []uint64)
 }
 
-func (m mockListener) Before(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, stackIterator experimental.StackIterator) context.Context {
-	return m.before(ctx, mod, def, paramValues, stackIterator)
+func (m mockListener) Before(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, stackIterator experimental.StackIterator) context.Context {
+	return m.before(ctx, mod, def, params, stackIterator)
 }
 
-func (m mockListener) After(ctx context.Context, mod api.Module, def api.FunctionDefinition, err error, resultValues []uint64) {
-	m.after(ctx, mod, def, err, resultValues)
+func (m mockListener) After(ctx context.Context, mod api.Module, def api.FunctionDefinition, results []uint64) {
+	m.after(ctx, mod, def, results)
 }
 
 func TestFunction_getSourceOffsetInWasmBinary(t *testing.T) {
diff --git a/internal/testing/enginetest/enginetest.go b/internal/testing/enginetest/enginetest.go
index 1a103a42..12e39860 100644
--- a/internal/testing/enginetest/enginetest.go
+++ b/internal/testing/enginetest/enginetest.go
@@ -556,7 +556,7 @@ func RunTestModuleEngineBeforeListenerStackIterator(t *testing.T, et EngineTeste
 	}
 
 	fnListener := &fnListener{
-		beforeFn: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, si experimental.StackIterator) context.Context {
+		beforeFn: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, si experimental.StackIterator) context.Context {
 			require.True(t, len(expectedCallstacks) > 0)
 			expectedCallstack := expectedCallstacks[0]
 			for si.Next() {
@@ -693,7 +693,7 @@ func RunTestModuleEngineBeforeListenerGlobals(t *testing.T, et EngineTester) {
 	}
 
 	fnListener := &fnListener{
-		beforeFn: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, si experimental.StackIterator) context.Context {
+		beforeFn: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, si experimental.StackIterator) context.Context {
 			require.True(t, len(expectedGlobals) > 0)
 
 			imod := mod.(experimental.InternalModule)
@@ -803,24 +803,24 @@ func RunTestModuleEngineBeforeListenerGlobals(t *testing.T, et EngineTester) {
 }
 
 type fnListener struct {
-	beforeFn func(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, stackIterator experimental.StackIterator) context.Context
-	afterFn  func(ctx context.Context, mod api.Module, def api.FunctionDefinition, err error, resultValues []uint64)
+	beforeFn func(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, stackIterator experimental.StackIterator) context.Context
+	afterFn  func(ctx context.Context, mod api.Module, def api.FunctionDefinition, results []uint64)
 }
 
 func (f *fnListener) NewFunctionListener(api.FunctionDefinition) experimental.FunctionListener {
 	return f
 }
 
-func (f *fnListener) Before(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, stackIterator experimental.StackIterator) context.Context {
+func (f *fnListener) Before(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, stackIterator experimental.StackIterator) context.Context {
 	if f.beforeFn != nil {
-		return f.beforeFn(ctx, mod, def, paramValues, stackIterator)
+		return f.beforeFn(ctx, mod, def, params, stackIterator)
 	}
 	return ctx
 }
 
-func (f *fnListener) After(ctx context.Context, mod api.Module, def api.FunctionDefinition, err error, resultValues []uint64) {
+func (f *fnListener) After(ctx context.Context, mod api.Module, def api.FunctionDefinition, results []uint64) {
 	if f.afterFn != nil {
-		f.afterFn(ctx, mod, def, err, resultValues)
+		f.afterFn(ctx, mod, def, results)
 	}
 }
 
@@ -835,7 +835,7 @@ func RunTestModuleEngineStackIteratorOffset(t *testing.T, et EngineTester) {
 	var tape [][]frame
 
 	fnListener := &fnListener{
-		beforeFn: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, paramValues []uint64, si experimental.StackIterator) context.Context {
+		beforeFn: func(ctx context.Context, mod api.Module, def api.FunctionDefinition, params []uint64, si experimental.StackIterator) context.Context {
 			var stack []frame
 			for si.Next() {
 				fn := si.Function()