From 8a92716bd961311c45d4b394135d7e7add6ecd69 Mon Sep 17 00:00:00 2001
From: Kurtis Rader <krader@skepticism.us>
Date: Sun, 16 May 2021 20:12:12 -0700
Subject: [PATCH] Add test coverage of `peach`

This also documents the behavior of `break` and `continue` when used in
the function passed to `peach`.

Related #1234
---
 pkg/eval/builtin_fn_flow.go      | 25 +++++++++++++++------
 pkg/eval/builtin_fn_flow_test.go | 37 +++++++++++++++++++++++++++++++-
 website/ref/language.md          |  6 +++---
 3 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/pkg/eval/builtin_fn_flow.go b/pkg/eval/builtin_fn_flow.go
index 0fe761f3..1461c5ea 100644
--- a/pkg/eval/builtin_fn_flow.go
+++ b/pkg/eval/builtin_fn_flow.go
@@ -2,6 +2,7 @@ package eval
 
 import (
 	"sync"
+	"sync/atomic"
 
 	"src.elv.sh/pkg/diag"
 	"src.elv.sh/pkg/eval/vals"
@@ -136,10 +137,15 @@ func each(fm *Frame, f Callable, inputs Inputs) error {
 //elvdoc:fn peach
 //
 // ```elvish
-// peach $f $input-list?
+// peach $f~ $input-list?
 // ```
 //
-// Call `$f` on all inputs, possibly in parallel.
+// Call `$f~` on all inputs, possibly in parallel. The exception from a
+// [`break`](./builtin.html#break) command it will cause `peach` to stop starting new instances of
+// `$f` with any remaining inputs. Because each instance of `$f~` is being run in parallel it is not
+// predictable when the early termination will occur or even that it will happen before all the
+// input has been consumed. The exception from a [`continue`](./builtin.html#continue) command is
+// ignored.
 //
 // Example (your output will differ):
 //
@@ -151,6 +157,12 @@ func each(fm *Frame, f Callable, inputs Inputs) error {
 // ▶ 16
 // ▶ 15
 // ▶ 14
+// ~> var tot = 0
+// ~> range 1 101 |
+//    peach [x]{ if (== 50 $x) { break } else { put $x } } |
+//    each [x]{ set tot = (+ $tot $x) }
+// ~> put $tot # without the break the total should be (num 5050)
+// ▶ (num 1603)
 // ```
 //
 // This command is intended for homogeneous processing of possibly unbound data. If
@@ -161,10 +173,11 @@ func each(fm *Frame, f Callable, inputs Inputs) error {
 
 func peach(fm *Frame, f Callable, inputs Inputs) error {
 	var w sync.WaitGroup
-	broken := false
+	var broken atomic.Value
+	broken.Store(false)
 	var err error
 	inputs(func(v interface{}) {
-		if broken || err != nil {
+		if broken.Load().(bool) || err != nil {
 			return
 		}
 		w.Add(1)
@@ -179,9 +192,9 @@ func peach(fm *Frame, f Callable, inputs Inputs) error {
 				case nil, Continue:
 					// nop
 				case Break:
-					broken = true
+					broken.Store(true)
 				default:
-					broken = true
+					broken.Store(true)
 					err = diag.Errors(err, ex)
 				}
 			}
diff --git a/pkg/eval/builtin_fn_flow_test.go b/pkg/eval/builtin_fn_flow_test.go
index b89ccfb5..3df0568f 100644
--- a/pkg/eval/builtin_fn_flow_test.go
+++ b/pkg/eval/builtin_fn_flow_test.go
@@ -30,7 +30,42 @@ func TestEach(t *testing.T) {
 	)
 }
 
-// TODO: test peach
+func TestPeach(t *testing.T) {
+	// Testing the `peach` builtin is a challenge since, by definition, the order of execution is
+	// undefined.
+	Test(t,
+		// Verify the output has the expected values when sorted.
+		That(`range 5 | peach [x]{ + 1 $x } | to-lines | order`).
+			Puts("1", "2", "3", "4", "5"),
+		// Verify that successive runs produce the output in different order. This test can
+		// theoretically suffer false positives but the vast majority of the time this will produce
+		// the expected output in the first iteration. The probability it will produce the same
+		// order of output in 100 iterations is effectively zero.
+		That(`
+			var cpu-count = 99
+			var x = [(range $cpu-count | peach [x]{ + 1 $x })]
+			for r [(range 100)] {
+				var y = [(range $cpu-count | peach [x]{ sleep 1us; + 1 $x })]
+				if (not-eq $x $y) {
+					put $true
+					break
+				}
+			}
+		`).Puts(true),
+		// Verify that exceptions are propagated.
+		That(`peach [x]{ fail $x } [a]`).
+			Throws(FailError{"a"}, "fail $x ", "peach [x]{ fail $x } [a]"),
+		// Verify that `break` works by terminating the `peach` before the entire sequence is
+		// consumed.
+		That(`
+			var tot = 0
+			range 1 101 |
+				peach [x]{ if (== 50 $x) { break } else { put $x } } |
+				each [x]{ set tot = (+ $tot $x) }
+			if (< $tot (/ (* 100 101) 2)) { put okay }
+		`).Puts("okay"),
+	)
+}
 
 func TestFail(t *testing.T) {
 	Test(t,
diff --git a/website/ref/language.md b/website/ref/language.md
index d757c9f1..f6363450 100644
--- a/website/ref/language.md
+++ b/website/ref/language.md
@@ -2083,9 +2083,9 @@ If an external command exits with a non-zero status, Elvish treats that as an
 exception.
 
 Flow commands -- `break`, `continue` and `return` -- are ordinary builtin
-commands that raise special "flow control" exceptions. The `for` and `while`
-commands capture `break` and `continue`, while `fn` modifies its closure to
-capture `return`.
+commands that raise special "flow control" exceptions. The `for`, `while`, and
+`peach` commands capture `break` and `continue`, while `fn` modifies its closure
+to capture `return`.
 
 One interesting implication is that since flow commands are just ordinary
 commands you can build functions on top of them. For instance, this function