Clean up parsing (if ...) (bytecodealliance#1215)

* Clean up parsing `(if ...)`

Much of this code dates back to when this crate was trying to parse all
of WABT's tests as well as the upstream test suite. WABT at the time had
syntactical forms that weren't technically supposed to be valid given
the upstream test suite, so at the time more was accepted than should be.

This commit fixes how `(if ...)` is parsed to require `(then ..)` and
`(else ..)` wrappers. Previously these were mistakenly only optional.
Additionally this fixes bytecodealliance#1213 by allowing multiple condition-related
folded expressions before `(then ..)`.

This updates a few of the golden outputs for the spec test suites,
notably those that use `(if (foo) (then) (else))` where previously
`wast` would erroneously not emit an `else` instruction but now it does.

Closes bytecodealliance#1213

* Fix some test syntax

crates/wast/src/core/expr.rs

@@ -98,18 +98,18 @@ enum Level<'a> {
     TryArm,
 }
 
-/// Possible states of "what should be parsed next?" in an `if` expression.
+/// Possible states of "what is currently being parsed?" in an `if` expression.
 enum If<'a> {
-    /// Only the `if` has been parsed, next thing to parse is the clause, if
-    /// any, of the `if` instruction.
+    /// Only the `if` instructoin has been parsed, next thing to parse is the
+    /// clause, if any, of the `if` instruction.
+    ///
+    /// This parse ends when `(then ...)` is encountered.
     Clause(Instruction<'a>),
-    /// Next thing to parse is the `then` block
-    Then(Instruction<'a>),
-    /// Next thing to parse is the `else` block
+    /// Currently parsing the `then` block, and afterwards a closing paren is
+    /// required or an `(else ...)` expression.
+    Then,
+    /// Parsing the `else` expression, nothing can come after.
     Else,
-    /// This `if` statement has finished parsing and if anything remains it's a
-    /// syntax error.
-    End,
 }
 
 /// Possible state of "what should be parsed next?" in a `try` expression.
@@ -216,9 +216,6 @@ impl<'a> ExpressionParser<'a> {
                     // items in the `if` statement. Otherwise we're just careful
                     // to terminate with an `end` instruction.
                     Level::If(If::Clause(_)) => {
-                        return Err(parser.error("previous `if` had no clause"));
-                    }
-                    Level::If(If::Then(_)) => {
                         return Err(parser.error("previous `if` had no `then`"));
                     }
                     Level::If(_) => {
@@ -281,23 +278,16 @@ impl<'a> ExpressionParser<'a> {
         })
     }
 
-    /// Handles all parsing of an `if` statement.
+    /// State transitions with parsing an `if` statement.
     ///
     /// The syntactical form of an `if` stament looks like:
     ///
     /// ```wat
-    /// (if $clause (then $then) (else $else))
+    /// (if ($clause)... (then $then) (else $else))
     /// ```
     ///
-    /// but it turns out we practically see a few things in the wild:
-    ///
-    /// * inside the `(if ...)` every sub-thing is surrounded by parens
-    /// * The `then` and `else` keywords are optional
-    /// * The `$then` and `$else` blocks don't need to be surrounded by parens
-    ///
-    /// That's all attempted to be handled here. The part about all sub-parts
-    /// being surrounded by `(` and `)` means that we hook into the `LParen`
-    /// parsing above to call this method there unconditionally.
+    /// THis method is called after a `(` is parsed within the `(if ...` block.
+    /// This determines what to do next.
     ///
     /// Returns `true` if the rest of the arm above should be skipped, or
     /// `false` if we should parse the next item as an instruction (because we
@@ -309,53 +299,37 @@ impl<'a> ExpressionParser<'a> {
             _ => return Ok(false),
         };
 
-        // The first thing parsed in an `if` statement is the clause. If the
-        // clause starts with `then`, however, then we know to skip the clause
-        // and fall through to below.
-        if let If::Clause(if_instr) = i {
-            let instr = mem::replace(if_instr, Instruction::End(None));
-            *i = If::Then(instr);
-            if !parser.peek::<kw::then>()? {
-                return Ok(false);
-            }
-        }
-
-        // All `if` statements are required to have a `then`. This is either the
-        // second s-expr (with or without a leading `then`) or the first s-expr
-        // with a leading `then`. The optionality of `then` isn't strictly what
-        // the text spec says but it matches wabt for now.
-        //
-        // Note that when we see the `then`, that's when we actually add the
-        // original `if` instruction to the stream.
-        if let If::Then(if_instr) = i {
-            let instr = mem::replace(if_instr, Instruction::End(None));
-            self.instrs.push(instr);
-            *i = If::Else;
-            if parser.parse::<Option<kw::then>>()?.is_some() {
+        match i {
+            // If the clause is still being parsed then interpret this `(` as a
+            // folded instruction unless it starts with `then`, in which case
+            // this transitions to the `Then` state and a new level has been
+            // reached.
+            If::Clause(if_instr) => {
+                if !parser.peek::<kw::then>()? {
+                    return Ok(false);
+                }
+                parser.parse::<kw::then>()?;
+                let instr = mem::replace(if_instr, Instruction::End(None));
+                self.instrs.push(instr);
+                *i = If::Then;
                 self.stack.push(Level::IfArm);
-                return Ok(true);
+                Ok(true)
             }
-            return Ok(false);
-        }
 
-        // effectively the same as the `then` parsing above
-        if let If::Else = i {
-            self.instrs.push(Instruction::Else(None));
-            if parser.parse::<Option<kw::r#else>>()?.is_some() {
-                if parser.is_empty() {
-                    self.instrs.pop();
-                }
+            // Previously we were parsing the `(then ...)` clause so this next
+            // `(` must be followed by `else`.
+            If::Then => {
+                parser.parse::<kw::r#else>()?;
+                self.instrs.push(Instruction::Else(None));
+                *i = If::Else;
                 self.stack.push(Level::IfArm);
-                return Ok(true);
+                Ok(true)
             }
-            *i = If::End;
-            return Ok(false);
-        }
 
-        // If we made it this far then we're at `If::End` which means that there
-        // were too many s-expressions inside the `(if)` and we don't want to
-        // parse anything else.
-        Err(parser.error("unexpected token: too many payloads inside of `(if)`"))
+            // If after a `(else ...` clause is parsed there's another `(` then
+            // that's not syntactically allowed.
+            If::Else => Err(parser.error("unexpected token: too many payloads inside of `(if)`")),
+        }
     }
 
     /// Handles parsing of a `try` statement. A `try` statement is simpler

crates/wit-component/tests/components/adapt-inject-stack-with-adapt-realloc/adapt-old.wat

@@ -25,7 +25,7 @@
 
     ;; `wit-component` should have injected a call to a function that allocates
     ;; the stack and sets $allocation_state to 2
-    (if (i32.ne (global.get $allocation_state) (i32.const 2)) (unreachable))
+    (if (i32.ne (global.get $allocation_state) (i32.const 2)) (then (unreachable)))
 
     ;; First, allocate a page using $cabi_realloc and write to it.  This tests
     ;; that we can use the main module's allocator if present (or else a

crates/wit-component/tests/components/adapt-inject-stack-with-realloc/adapt-old.wat

@@ -25,7 +25,7 @@
 
     ;; `wit-component` should have injected a call to a function that allocates
     ;; the stack and sets $allocation_state to 2
-    (if (i32.ne (global.get $allocation_state) (i32.const 2)) (unreachable))
+    (if (i32.ne (global.get $allocation_state) (i32.const 2)) (then (unreachable)))
 
     ;; First, allocate a page using $cabi_realloc and write to it.  This tests
     ;; that we can use the main module's allocator if present (or else a

crates/wit-component/tests/components/adapt-inject-stack-with-reallocing-adapter/adapt-old.wat

@@ -24,7 +24,7 @@
 
     ;; `wit-component` should have injected a call to a function that allocates
     ;; the stack and sets $allocation_state to 2
-    (if (i32.ne (global.get $allocation_state) (i32.const 2)) (unreachable))
+    (if (i32.ne (global.get $allocation_state) (i32.const 2)) (then (unreachable)))
 
     ;; Allocate 8 bytes of stack space for the two u32 return values. The
     ;; original stack pointer is saved in local 0 and the stack frame for this

tests/local/fuzz1.wat

@@ -260,9 +260,9 @@
     (i32.eqz
      (global.get $hangLimit)
     )
-    (return
+    (then (return
      (i64.const 4293531749)
-    )
+    ))
    )
    (global.set $hangLimit
     (i32.sub
@@ -279,8 +279,8 @@
       (local.tee $0
        (if (result i32)
         (i32.const -2147483647)
-        (local.get $0)
-        (block $label$4 (result i32)
+        (then (local.get $0))
+        (else (block $label$4 (result i32)
          (br_if $label$4
           (if (result i32)
            (block $label$5 (result i32)
@@ -293,9 +293,9 @@
                   (i32.eqz
                    (global.get $hangLimit)
                   )
-                  (return
+                  (then (return
                    (i64.const -32766)
-                  )
+                  ))
                  )
                  (global.set $hangLimit
                   (i32.sub
@@ -315,22 +315,22 @@
                 )
                )
               )
-              (block $label$7 (result f32)
+              (then (block $label$7 (result f32)
                (f32.const -nan:0x7fffda)
-              )
-              (f32.const 4294967296)
+              ))
+              (else (f32.const 4294967296))
              )
             )
             (i32.const -2147483648)
            )
-           (i32.const -1073741824)
-           (local.get $0)
+           (then (i32.const -1073741824))
+           (else (local.get $0))
           )
           (i32.eqz
            (global.get $global$0)
           )
          )
-        )
+        ))
        )
       )
      )

tests/local/if-else-parsing.wast

@@ -0,0 +1,42 @@
+(module
+  (func $a1
+    i32.const 0
+    (if (then) (else))
+  )
+  (func $a2
+    (if (i32.const 1) (i32.eqz) (then) (else))
+  )
+  (func $a3
+    (if (i32.const 1) (i32.eqz) (then))
+  )
+  (func $a4
+    (if (i32.const 1) (i32.eqz) (then nop))
+  )
+)
+(assert_invalid
+  (module quote
+    "(func (if))"
+  )
+  "no `then`")
+
+(assert_invalid
+  (module quote
+    "(func (if (else)))"
+  )
+  "no `then`")
+
+(assert_invalid
+  (module quote
+    "(func (if nop (else)))"
+  )
+  "expected `(`")
+(assert_invalid
+  (module quote
+    "(func (if (nop) (else)))"
+  )
+  "no `then`")
+(assert_invalid
+  (module quote
+    "(func (if (nop) nop (then)))"
+  )
+  "expected `(`")

tests/snapshots/local/if-else-parsing.wast/0.print

@@ -0,0 +1,29 @@
+(module
+  (type (;0;) (func))
+  (func $a1 (;0;) (type 0)
+    i32.const 0
+    if ;; label = @1
+    else
+    end
+  )
+  (func $a2 (;1;) (type 0)
+    i32.const 1
+    i32.eqz
+    if ;; label = @1
+    else
+    end
+  )
+  (func $a3 (;2;) (type 0)
+    i32.const 1
+    i32.eqz
+    if ;; label = @1
+    end
+  )
+  (func $a4 (;3;) (type 0)
+    i32.const 1
+    i32.eqz
+    if ;; label = @1
+      nop
+    end
+  )
+)

tests/snapshots/testsuite/if.wast/0.print

@@ -22,12 +22,14 @@
     end
     local.get 0
     if ;; label = @1
+    else
     end
     local.get 0
     if $l ;; label = @1
     end
     local.get 0
     if $l ;; label = @1
+    else
     end
   )
   (func (;2;) (type 5) (param i32) (result i32)

tests/snapshots/testsuite/proposals/function-references/if.wast/0.print

@@ -22,12 +22,14 @@
     end
     local.get 0
     if ;; label = @1
+    else
     end
     local.get 0
     if $l ;; label = @1
     end
     local.get 0
     if $l ;; label = @1
+    else
     end
   )
   (func (;2;) (type 5) (param i32) (result i32)

tests/snapshots/testsuite/proposals/gc/if.wast/0.print

@@ -22,12 +22,14 @@
     end
     local.get 0
     if ;; label = @1
+    else
     end
     local.get 0
     if $l ;; label = @1
     end
     local.get 0
     if $l ;; label = @1
+    else
     end
   )
   (func (;2;) (type 5) (param i32) (result i32)