Make call_ref type annotations mandatory (#5246)

They were optional for a while to allow users to gracefully transition to using
them, but now make them mandatory to match the upstream WasmGC spec.

Author: tlively

CHANGELOG.md

@@ -18,8 +18,7 @@ Current Trunk
 - Add extra `memory64` argument for `BinaryenSetMemory` and new
   `BinaryenMemoryIs64` C-API method to determine 64-bit memory. (#4963)
 - `TypeBuilderSetSubType` now takes a supertype as the second argument.
-- `call_ref` can now take a signature type immediate in the text format. The
-  type immediate will become mandatory in the future.
+- `call_ref` now takes a mandatory signature type immediate.
 - If `THROW_ON_FATAL` is defined at compile-time, then fatal errors will throw a
   `std::runtime_error` instead of terminating the process. This may be used by
   embedders of Binaryen to recover from errors.

src/wasm-binary.h

@@ -1094,8 +1094,7 @@ enum ASTNodes {
 
   // typed function references opcodes
 
-  CallRefUnannotated = 0x14,
-  CallRef = 0x17,
+  CallRef = 0x14,
   RetCallRef = 0x15,
 
   // gc opcodes
@@ -1743,8 +1742,7 @@ class WasmBinaryBuilder {
   void visitTryOrTryInBlock(Expression*& out);
   void visitThrow(Throw* curr);
   void visitRethrow(Rethrow* curr);
-  void visitCallRef(CallRef* curr,
-                    std::optional<HeapType> maybeType = std::nullopt);
+  void visitCallRef(CallRef* curr);
   void visitRefAs(RefAs* curr, uint8_t code);
 
   [[noreturn]] void throwError(std::string text);

src/wasm-builder.h

@@ -1342,35 +1342,6 @@ class ValidatingBuilder : public Builder {
     }
     return makeBrOn(op, name, ref);
   }
-
-  template<typename T>
-  Expression* validateAndMakeCallRef(Expression* target,
-                                     const T& args,
-                                     bool isReturn = false) {
-    if (target->type != Type::unreachable && !target->type.isRef()) {
-      throw ParseException("Non-reference type for a call_ref", line, col);
-    }
-    // TODO: This won't be necessary once type annotations are mandatory on
-    // call_ref.
-    if (target->type == Type::unreachable ||
-        target->type.getHeapType() == HeapType::nofunc) {
-      // An unreachable target is not supported. Similiar to br_on_cast, just
-      // emit an unreachable sequence, since we don't have enough information
-      // to create a full call_ref.
-      std::vector<Expression*> children;
-      for (auto* arg : args) {
-        children.push_back(makeDrop(arg));
-      }
-      children.push_back(makeDrop(target));
-      children.push_back(makeUnreachable());
-      return makeBlock(children, Type::unreachable);
-    }
-    auto heapType = target->type.getHeapType();
-    if (!heapType.isSignature()) {
-      throw ParseException("Invalid reference type for a call_ref", line, col);
-    }
-    return makeCallRef(target, args, heapType.getSignature().results, isReturn);
-  }
 };
 
 } // namespace wasm

src/wasm/wasm-binary.cpp

@@ -3846,15 +3846,12 @@ BinaryConsts::ASTNodes WasmBinaryBuilder::readExpression(Expression*& curr) {
       visitMemoryGrow(grow);
       break;
     }
-    case BinaryConsts::CallRefUnannotated:
-      visitCallRef((curr = allocator.alloc<CallRef>())->cast<CallRef>());
-      break;
     case BinaryConsts::CallRef:
     case BinaryConsts::RetCallRef: {
       auto call = allocator.alloc<CallRef>();
       call->isReturn = code == BinaryConsts::RetCallRef;
       curr = call;
-      visitCallRef(call, getTypeByIndex(getU32LEB()));
+      visitCallRef(call);
       break;
     }
     case BinaryConsts::AtomicPrefix: {
@@ -6851,29 +6848,13 @@ void WasmBinaryBuilder::visitRethrow(Rethrow* curr) {
   curr->finalize();
 }
 
-void WasmBinaryBuilder::visitCallRef(CallRef* curr,
-                                     std::optional<HeapType> maybeType) {
+void WasmBinaryBuilder::visitCallRef(CallRef* curr) {
   BYN_TRACE("zz node: CallRef\n");
   curr->target = popNonVoidExpression();
-  HeapType heapType;
-  if (maybeType) {
-    heapType = *maybeType;
-    if (!Type::isSubType(curr->target->type, Type(heapType, Nullable))) {
-      throwError("Call target has invalid type: " +
-                 curr->target->type.toString());
-    }
-  } else {
-    auto type = curr->target->type;
-    if (type == Type::unreachable) {
-      // If our input is unreachable, then we cannot even find out how many
-      // inputs we have, and just set ourselves to unreachable as well.
-      curr->finalize(type);
-      return;
-    }
-    if (!type.isRef()) {
-      throwError("Non-ref type for a call_ref: " + type.toString());
-    }
-    heapType = type.getHeapType();
+  HeapType heapType = getTypeByIndex(getU32LEB());
+  if (!Type::isSubType(curr->target->type, Type(heapType, Nullable))) {
+    throwError("Call target has invalid type: " +
+               curr->target->type.toString());
   }
   if (!heapType.isSignature()) {
     throwError("Invalid reference type for a call_ref: " + heapType.toString());

src/wasm/wasm-s-parser.cpp

@@ -2744,35 +2744,20 @@ Expression* SExpressionWasmBuilder::makeTupleExtract(Element& s) {
 }
 
 Expression* SExpressionWasmBuilder::makeCallRef(Element& s, bool isReturn) {
-  Index operandsStart = 1;
-  std::optional<HeapType> sigType;
-  try {
-    sigType = parseHeapType(*s[1]);
-    operandsStart = 2;
-  } catch (ParseException& p) {
-    // The type annotation is required for return_call_ref but temporarily
-    // optional for call_ref.
-    if (isReturn) {
-      throw;
-    }
-  }
+  HeapType sigType = parseHeapType(*s[1]);
   std::vector<Expression*> operands;
-  parseOperands(s, operandsStart, s.size() - 1, operands);
+  parseOperands(s, 2, s.size() - 1, operands);
   auto* target = parseExpression(s[s.size() - 1]);
 
-  if (sigType) {
-    if (!sigType->isSignature()) {
-      throw ParseException(
-        std::string(isReturn ? "return_call_ref" : "call_ref") +
-          " type annotation should be a signature",
-        s.line,
-        s.col);
-    }
-    return Builder(wasm).makeCallRef(
-      target, operands, sigType->getSignature().results, isReturn);
+  if (!sigType.isSignature()) {
+    throw ParseException(
+      std::string(isReturn ? "return_call_ref" : "call_ref") +
+        " type annotation should be a signature",
+      s.line,
+      s.col);
   }
-  return ValidatingBuilder(wasm, s.line, s.col)
-    .validateAndMakeCallRef(target, operands, isReturn);
+  return Builder(wasm).makeCallRef(
+    target, operands, sigType.getSignature().results, isReturn);
 }
 
 Expression* SExpressionWasmBuilder::makeI31New(Element& s) {

test/lit/passes/dae_all-features.wast

@@ -510,7 +510,7 @@
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $1
-  (call_ref
+  (call_ref $i64
    (i64.const 0)
    (global.get $global$0)
   )

test/lit/passes/gufa-refs.wast

@@ -888,7 +888,7 @@
       (local.get $y)
     )
     ;; Send a non-null value only to the second param.
-    (call_ref
+    (call_ref $two-params
       (ref.as_non_null
         (ref.null $struct)
       )
@@ -4592,19 +4592,19 @@
     ;; Call $i1 twice with the same value, and $i2 twice with different values.
     ;; Note that structurally the types are identical, but we still
     ;; differentiate them, allowing us to optimize.
-    (call_ref
+    (call_ref $i1
       (i32.const 42)
       (ref.func $reffed1)
     )
-    (call_ref
+    (call_ref $i1
       (i32.const 42)
       (ref.func $reffed1)
     )
-    (call_ref
+    (call_ref $i2
       (i32.const 1337)
       (ref.func $reffed2)
     )
-    (call_ref
+    (call_ref $i2
       (i32.const 99999)
       (ref.func $reffed2)
     )

test/lit/passes/inlining-optimizing.wast

@@ -32,7 +32,7 @@
   ;; unreachable.)
   (call $0)
   (drop
-   (call_ref
+   (call_ref $none_=>_i32
     (ref.cast_static $none_=>_i32
      (ref.func $0)
     )

test/lit/passes/local-cse_all-features.wast

@@ -4,21 +4,22 @@
 ;; RUN: foreach %s %t wasm-opt --local-cse --all-features -S -o - | filecheck %s
 
 (module
-  ;; CHECK:      (type $i32_=>_i32 (func (param i32) (result i32)))
+  ;; CHECK:      (type $f (func (param i32) (result i32)))
+  (type $f (func (param i32) (result i32)))
 
   ;; CHECK:      (type $none_=>_none (func))
 
   ;; CHECK:      (elem declare func $calls $ref.func)
 
   ;; CHECK:      (func $calls (param $x i32) (result i32)
   ;; CHECK-NEXT:  (drop
-  ;; CHECK-NEXT:   (call_ref $i32_=>_i32
+  ;; CHECK-NEXT:   (call_ref $f
   ;; CHECK-NEXT:    (i32.const 10)
   ;; CHECK-NEXT:    (ref.func $calls)
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT:  (drop
-  ;; CHECK-NEXT:   (call_ref $i32_=>_i32
+  ;; CHECK-NEXT:   (call_ref $f
   ;; CHECK-NEXT:    (i32.const 10)
   ;; CHECK-NEXT:    (ref.func $calls)
   ;; CHECK-NEXT:   )
@@ -28,10 +29,10 @@
   (func $calls (param $x i32) (result i32)
     ;; The side effects of calls prevent optimization.
     (drop
-      (call_ref (i32.const 10) (ref.func $calls))
+      (call_ref $f (i32.const 10) (ref.func $calls))
     )
     (drop
-      (call_ref (i32.const 10) (ref.func $calls))
+      (call_ref $f (i32.const 10) (ref.func $calls))
     )
     (i32.const 20)
   )

test/lit/passes/optimize-instructions-call_ref-roundtrip.wast

@@ -69,17 +69,17 @@
  (func $call-table-get (param $x i32)
   ;; The heap type of the call_indirects that we emit here should be the
   ;; identical one as on the table that they correspond to.
-  (call_ref
+  (call_ref $v1
    (table.get $table-1
     (local.get $x)
    )
   )
-  (call_ref
+  (call_ref $v2
    (table.get $table-2
     (local.get $x)
    )
   )
-  (call_ref
+  (call_ref $v3
    (table.get $table-3
     (local.get $x)
    )

test/lit/passes/optimize-instructions-call_ref.wast

@@ -51,7 +51,7 @@
  ;; CHECK-NEXT: )
  (func $call_ref-to-direct (param $x i32) (param $y i32)
   ;; This call_ref should become a direct call.
-  (call_ref
+  (call_ref $i32_i32_=>_none
    (local.get $x)
    (local.get $y)
    (ref.func $foo)
@@ -84,7 +84,7 @@
   ;; This call_ref should become a direct call, even though it doesn't have a
   ;; simple ref.func as the target - we need to look into the fallthrough, and
   ;; handle things with locals.
-  (call_ref
+  (call_ref $i32_i32_=>_none
    ;; Write to $x before the block, and write to it in the block; we should not
    ;; reorder these things as the side effects could alter what value appears
    ;; in the get of $x. (There is a risk of reordering here if we naively moved
@@ -116,7 +116,7 @@
  (func $fallthrough-no-params (result i32)
   ;; A fallthrough appears here, but there are no operands so this is easier to
   ;; optimize: we can just drop the call_ref's target before the call.
-  (call_ref
+  (call_ref $none_=>_i32
    (block (result (ref $none_=>_i32))
     (nop)
     (ref.func $fallthrough-no-params)
@@ -148,7 +148,7 @@
   ;; nullable, which means we must be careful when we create a temp local for
   ;; it: the local should be nullable, and gets of it should use a
   ;; ref.as_non_null so that we validate.
-  (call_ref
+  (call_ref $data_=>_none
    (local.get $x)
    (block (result (ref $data_=>_none))
     (nop)
@@ -174,7 +174,7 @@
   ;; emit non-validating code here, which would happen if we replace the
   ;; call_ref that returns nothing with a call that returns an i32. In fact, we
   ;; end up optimizing the cast into an unreachable.
-  (call_ref
+  (call_ref $none_=>_i32
    (ref.cast_static $none_=>_i32
     (ref.func $return-nothing)
    )
@@ -199,7 +199,7 @@
  ;; CHECK-NEXT: )
  (func $fallthrough-unreachable
   ;; If the call is not reached, do not optimize it.
-  (call_ref
+  (call_ref $i32_i32_=>_none
    (unreachable)
    (unreachable)
    (block (result (ref $i32_i32_=>_none))
@@ -210,14 +210,16 @@
  )
 
  ;; CHECK:      (func $ignore-unreachable
- ;; CHECK-NEXT:  (drop
+ ;; CHECK-NEXT:  (block ;; (replaces something unreachable we can't emit)
+ ;; CHECK-NEXT:   (drop
+ ;; CHECK-NEXT:    (unreachable)
+ ;; CHECK-NEXT:   )
  ;; CHECK-NEXT:   (unreachable)
  ;; CHECK-NEXT:  )
- ;; CHECK-NEXT:  (unreachable)
  ;; CHECK-NEXT: )
  (func $ignore-unreachable
   ;; Ignore an unreachable call_ref target entirely.
-  (call_ref
+  (call_ref $i32_i32_=>_none
    (unreachable)
   )
  )
@@ -230,7 +232,7 @@
  ;; CHECK-NEXT:  )
  ;; CHECK-NEXT: )
  (func $call-table-get (param $x i32)
-  (call_ref
+  (call_ref $i32_i32_=>_none
    (i32.const 1)
    (i32.const 2)
    (table.get $table-1
@@ -273,7 +275,7 @@
  ;; CHECK-NEXT: )
  (func $call_ref-to-select (param $x i32) (param $y i32) (param $z i32) (param $f (ref $i32_i32_=>_none))
   ;; This call_ref should become an if over two direct calls.
-  (call_ref
+  (call_ref $i32_i32_=>_none
    (local.get $x)
    (local.get $y)
    (select
@@ -284,7 +286,7 @@
   )
 
   ;; But here one arm is not constant, so we do not optimize.
-  (call_ref
+  (call_ref $i32_i32_=>_none
    (local.get $x)
    (local.get $y)
    (select

test/lit/passes/precompute-gc.wast

@@ -1198,7 +1198,7 @@
   (drop
    ;; Read from the local, checking whether precompute set a value there (it
    ;; should not, as the cast fails).
-   (call_ref
+   (call_ref $func-return-i32
     (local.get $temp)
    )
   )