fix({Slice,Map}End): Not allowed with ContinueOnError when instrument…

…ed (#20) The compile-time check that verifies that SliceEnd and MapEnd are not allowed with ContinueOnError was part of the function that validates instrumentation. This introduced a bug: if that function returned early because the number of emitters was non-zero, it would never run this check. Fix this and add a failing test case. This allowed a bug in our magic.go example files, because they were using SliceEnd/MapEnd despite ContinueOnError. Found while working on #21.
uber-go · Oct 21, 2022 · 5525184 · 5525184
1 parent 1a94aa8
commit 5525184
Show file tree

Hide file tree

Showing 7 changed files with 79 additions and 88 deletions.
diff --git a/examples/magic.go b/examples/magic.go
@@ -126,9 +126,6 @@ func (h *fooHandler) HandleFoo(ctx context.Context, req *Request) (*Response, er
 				return nil
 			},
 			[]string{"more", "messages", "sent"},
-			cff.SliceEnd(func(context.Context) error {
-				return nil
-			}),
 		),
 		cff.Map(
 			func(ctx context.Context, key string, value string) error {
@@ -144,9 +141,6 @@ func (h *fooHandler) HandleFoo(ctx context.Context, req *Request) (*Response, er
 				return nil
 			},
 			map[string]int{"a": 1, "b": 2, "c": 3},
-			cff.MapEnd(func(context.Context) {
-				_ = fmt.Sprint("}")
-			}),
 		),
 	)
 	return res, err

diff --git a/examples/magic_gen.go b/examples/magic_gen.go
diff --git a/examples/magic_v2.go b/examples/magic_v2.go
@@ -126,9 +126,6 @@ func (h *fooHandlerV2) HandleFoo(ctx context.Context, req *RequestV2) (*Response
 				return nil
 			},
 			[]string{"more", "messages", "sent"},
-			cff.SliceEnd(func(context.Context) error {
-				return nil
-			}),
 		),
 		cff.Map(
 			func(ctx context.Context, key string, value string) error {
@@ -144,9 +141,6 @@ func (h *fooHandlerV2) HandleFoo(ctx context.Context, req *RequestV2) (*Response
 				return nil
 			},
 			map[string]int{"a": 1, "b": 2, "c": 3},
-			cff.MapEnd(func(context.Context) {
-				_ = fmt.Sprint("}")
-			}),
 		),
 	)
 	return res, err

diff --git a/examples/magic_v2_gen.go b/examples/magic_v2_gen.go
diff --git a/internal/aquaregia_test.go b/internal/aquaregia_test.go
@@ -229,6 +229,7 @@ var codeGenerateFailCases = map[string][]errorCase{
 			ErrorMatches: `"cff.SliceEnd" is an invalid option when "ContinueOnError" is used`,
 			TestFuncs: []string{
 				"ParallelSliceEndWithContinueOnError",
+				"ParallelSliceEndWithContinueOnErrorAndInstrument",
 			},
 		},
 		{
@@ -393,7 +394,10 @@ var codeGenerateFailCases = map[string][]errorCase{
 		{
 			File:         "parallel.go",
 			ErrorMatches: `"cff.MapEnd" is an invalid option when "ContinueOnError" is used`,
-			TestFuncs:    []string{"ParallelMapEndWithContinueOnError"},
+			TestFuncs: []string{
+				"ParallelMapEndWithContinueOnError",
+				"ParallelMapEndWithContinueOnErrorAndInstrument",
+			},
 		},
 	},
 	"cycles": {

diff --git a/internal/compile_parallel.go b/internal/compile_parallel.go
@@ -100,6 +100,18 @@ func (c *compiler) compileParallel(file *ast.File, call *ast.CallExpr) *parallel
 	}
 	c.validateParallelInstrument(parallel)
 
+	for _, s := range parallel.SliceTasks {
+		if s.SliceEndFn != nil && parallel.ContinueOnError != nil {
+			c.errf(c.nodePosition(parallel.Node), `"cff.SliceEnd" is an invalid option when "ContinueOnError" is used`)
+		}
+	}
+
+	for _, m := range parallel.MapTasks {
+		if m.MapEndFn != nil && parallel.ContinueOnError != nil {
+			c.errf(c.nodePosition(parallel.Node), `"cff.MapEnd" is an invalid option when "ContinueOnError" is used`)
+		}
+	}
+
 	return parallel
 }
 
@@ -119,18 +131,6 @@ func (c *compiler) validateParallelInstrument(p *parallel) {
 			c.errf(c.nodePosition(p.Node), "cff.Instrument requires a cff.Emitter to be provided: use cff.WithEmitter")
 		}
 	}
-
-	for _, s := range p.SliceTasks {
-		if s.SliceEndFn != nil && p.ContinueOnError != nil {
-			c.errf(c.nodePosition(p.Node), `"cff.SliceEnd" is an invalid option when "ContinueOnError" is used`)
-		}
-	}
-
-	for _, m := range p.MapTasks {
-		if m.MapEndFn != nil && p.ContinueOnError != nil {
-			c.errf(c.nodePosition(p.Node), `"cff.MapEnd" is an invalid option when "ContinueOnError" is used`)
-		}
-	}
 }
 
 func (c *compiler) compileParallelTask(p *parallel, call ast.Expr, opts []ast.Expr) *parallelTask {

diff --git a/internal/failing_tests/bad-inputs/parallel.go b/internal/failing_tests/bad-inputs/parallel.go
@@ -357,6 +357,50 @@ func ParallelSliceEndWithInvalidReturn() {
 	)
 }
 
+// ParallelSliceEndWithContinueOnErrorAndInstrument is a cff.Slice function
+// that uses SliceEnd, ContinueOnError, and Instrument.
+//
+// This tests for a regression where SliceEnd-ContinueOnError incompatibility
+// was not verified if Instrument was used.
+func ParallelSliceEndWithContinueOnErrorAndInstrument() {
+	cff.Parallel(context.Background(),
+		cff.WithEmitter(cff.NopEmitter()),
+		cff.InstrumentParallel("myparallel"),
+		cff.ContinueOnError(true),
+		cff.Slice(
+			func(int, string) {
+				// stuff
+			},
+			[]string{"foo", "bar"},
+			cff.SliceEnd(func() error {
+				return nil
+			}),
+		),
+	)
+}
+
+// ParallelSliceEndWithContinueOnErrorAndInstrument is a cff.Slice function
+// that uses SliceEnd, ContinueOnError, and Instrument.
+//
+// This tests for a regression where SliceEnd-ContinueOnError incompatibility
+// was not verified if Instrument was used.
+func ParallelMapEndWithContinueOnErrorAndInstrument() {
+	cff.Parallel(context.Background(),
+		cff.WithEmitter(cff.NopEmitter()),
+		cff.InstrumentParallel("myparallel"),
+		cff.ContinueOnError(true),
+		cff.Map(
+			func(string, string) {
+				// stuff
+			},
+			map[string]string{"foo": "bar"},
+			cff.MapEnd(func() error {
+				return nil
+			}),
+		),
+	)
+}
+
 // ParallelSliceWithTwoSliceEnds is a cff.Slice function that has more than
 // one cff.SliceEnd.
 func ParallelSliceWithTwoSliceEnds() {