2873 - fix a bug in converting conditional returns

This was an interaction between two transforms. The first turned:

  return-if ...

into:

  jump-unless ..., 1:offset  # skip next instruction
  return ...

The second added an unconditional return at the end of the recipe if it
didn't already exist (so that functions always end with a return).
However, it was getting confused by the return instructions generated
above, which look unconditional but sometimes get skipped.

To fix this, conditional returns are now transformed into this:

  {
    break-unless ...
    return ...
  }

Since the final instruction is now no longer a reply (but rather the '}'
label), the second transform triggers and adds the unconditional return
after it.

This commit removes the final place marked 'BUG:' in the codebase
yesterday (see commit 2870).

5 files changed, 100 insertions, 74 deletions

diff --git a/028call_reply.cc b/028call_reply.cc
index df5fa557..09360d25 100644
--- a/028call_reply.cc
+++ b/028call_reply.cc
@@ -158,73 +158,3 @@ string to_string(const vector<double>& in) {
   out << "]";
   return out.str();
 }
-
-//: Conditional reply.
-
-:(scenario reply_if)
-def main [
-  1:number <- test1
-]
-def test1 [
-  return-if 0, 34
-  return 35
-]
-+mem: storing 35 in location 1
-
-:(scenario reply_if_2)
-def main [
-  1:number <- test1
-]
-def test1 [
-  return-if 1, 34
-  return 35
-]
-+mem: storing 34 in location 1
-
-:(before "End Rewrite Instruction(curr, recipe result)")
-// rewrite `reply-if a, b, c, ...` to
-//   ```
-//   jump-unless a, 1:offset
-//   reply b, c, ...
-//   ```
-if (curr.name == "reply-if" || curr.name == "return-if") {
-  if (curr.products.empty()) {
-    curr.operation = get(Recipe_ordinal, "jump-unless");
-    curr.name = "jump-unless";
-    vector<reagent> results;
-    copy(++curr.ingredients.begin(), curr.ingredients.end(), inserter(results, results.end()));
-    curr.ingredients.resize(1);
-    curr.ingredients.push_back(reagent("1:offset"));
-    result.steps.push_back(curr);
-    curr.clear();
-    curr.operation = get(Recipe_ordinal, "reply");
-    curr.name = "reply";
-    curr.ingredients.swap(results);
-  }
-  else {
-    raise << "'" << curr.name << "' never yields any products\n" << end();
-  }
-}
-// rewrite `reply-unless a, b, c, ...` to
-//   ```
-//   jump-if a, 1:offset
-//   reply b, c, ...
-//   ```
-if (curr.name == "reply-unless" || curr.name == "return-unless") {
-  if (curr.products.empty()) {
-    curr.operation = get(Recipe_ordinal, "jump-if");
-    curr.name = "jump-if";
-    vector<reagent> results;
-    copy(++curr.ingredients.begin(), curr.ingredients.end(), inserter(results, results.end()));
-    curr.ingredients.resize(1);
-    curr.ingredients.push_back(reagent("1:offset"));
-    result.steps.push_back(curr);
-    curr.clear();
-    curr.operation = get(Recipe_ordinal, "reply");
-    curr.name = "reply";
-    curr.ingredients.swap(results);
-  }
-  else {
-    raise << "'" << curr.name << "' never yields any products\n" << end();
-  }
-}
diff --git a/040brace.cc b/040brace.cc
index 2c4598e0..3c295df9 100644
--- a/040brace.cc
+++ b/040brace.cc
@@ -361,6 +361,91 @@ def main [
 ]
 +error: break-if expects 1 or 2 ingredients, but got none
 
+//: Using break we can now implement conditional reply.
+
+:(scenario reply_if)
+def main [
+  1:number <- test1
+]
+def test1 [
+  return-if 0, 34
+  return 35
+]
++mem: storing 35 in location 1
+
+:(scenario reply_if_2)
+def main [
+  1:number <- test1
+]
+def test1 [
+  return-if 1, 34
+  return 35
+]
++mem: storing 34 in location 1
+
+:(before "End Rewrite Instruction(curr, recipe result)")
+// rewrite `reply-if a, b, c, ...` to
+//   ```
+//   {
+//     break-unless a
+//     reply b, c, ...
+//   }
+//   ```
+if (curr.name == "reply-if" || curr.name == "return-if") {
+  if (curr.products.empty()) {
+    emit_reply_block(result, "break-unless", curr.ingredients);
+    curr.clear();
+  }
+  else {
+    raise << "'" << curr.name << "' never yields any products\n" << end();
+  }
+}
+// rewrite `reply-unless a, b, c, ...` to
+//   ```
+//   {
+//     break-if a
+//     reply b, c, ...
+//   }
+//   ```
+if (curr.name == "reply-unless" || curr.name == "return-unless") {
+  if (curr.products.empty()) {
+    emit_reply_block(result, "break-if", curr.ingredients);
+    curr.clear();
+  }
+  else {
+    raise << "'" << curr.name << "' never yields any products\n" << end();
+  }
+}
+
+:(code)
+void emit_reply_block(recipe& out, const string& break_command, const vector<reagent>& ingredients) {
+  reagent condition = ingredients.at(0);
+  vector<reagent> reply_ingredients;
+  copy(++ingredients.begin(), ingredients.end(), inserter(reply_ingredients, reply_ingredients.end()));
+
+  // {
+  instruction open_label;  open_label.is_label=true;  open_label.label = "{";
+  out.steps.push_back(open_label);
+
+  // <break command> <condition>
+  instruction break_inst;
+  break_inst.operation = get(Recipe_ordinal, break_command);
+  break_inst.name = break_inst.old_name = break_command;
+  break_inst.ingredients.push_back(condition);
+  out.steps.push_back(break_inst);
+
+  // reply <reply ingredients>
+  instruction reply_inst;
+  reply_inst.operation = get(Recipe_ordinal, "reply");
+  reply_inst.name = "reply";
+  reply_inst.ingredients.swap(reply_ingredients);
+  out.steps.push_back(reply_inst);
+
+  // }
+  instruction close_label;  close_label.is_label=true;  close_label.label = "}";
+  out.steps.push_back(close_label);
+}
+
 //: Make sure these pseudo recipes get consistent numbers in all tests, even
 //: though they aren't implemented. Allows greater flexibility in ordering
 //: transforms.
diff --git a/055recipe_header.cc b/055recipe_header.cc
index a9ac73ba..9a4ec089 100644
--- a/055recipe_header.cc
+++ b/055recipe_header.cc
@@ -452,6 +452,18 @@ def add2 x:number, y:number -> z:number [
 -transform: instruction: reply z:number
 +mem: storing 8 in location 1
 
+:(scenario reply_after_conditional_reply_based_on_header)
+def main [
+  1:number/raw <- add2 3, 5
+]
+def add2 x:number, y:number -> z:number [
+  local-scope
+  load-ingredients
+  z <- add x, y  # no type for z
+  return-if 0/false, 34
+]
++mem: storing 8 in location 1
+
 :(scenario recipe_headers_perform_same_ingredient_check)
 % Hide_errors = true;
 def main [
diff --git a/072channel.mu b/072channel.mu
index 6fe96e91..8565cd34 100644
--- a/072channel.mu
+++ b/072channel.mu
@@ -284,12 +284,12 @@ def close x:address:sink:_elem -> x:address:sink:_elem [
 #   then the channel is also closed for reading
 after <channel-write-initial> [
   closed?:boolean <- get *chan, closed?:offset
-  reply-if closed?
+  return-if closed?
 ]
 
 after <channel-read-empty> [
   closed?:boolean <- get *chan, closed?:offset
-  reply-if closed?, 0/hack  # only scalar _elems supported in channels; need to support construction of arbitrary empty containers
+  return-if closed?, 0/hack  # only scalar _elems supported in channels; need to support construction of arbitrary empty containers
 ]
 
 ## helpers
@@ -382,7 +382,7 @@ def buffer-lines in:address:source:character, buffered-out:address:sink:characte
     {
       break-unless eof?
       buffered-out <- close buffered-out
-      reply
+      return
     }
     loop
   }
diff --git a/chessboard.mu b/chessboard.mu
index 3d6b93e2..9bd5adef 100644
--- a/chessboard.mu
+++ b/chessboard.mu
@@ -259,7 +259,6 @@ def read-move stdin:address:source:character, screen:address:screen -> result:ad
   *result <- put *result, to-rank:offset, to-rank
   error? <- expect-from-channel stdin, 10/newline, screen
   return-if error?, 0/dummy, 0/quit
-  return result  # BUG: why is this statement required?
 ]
 
 # valid values for file: 0-7