Fix DeadArgumentElimination + TrapsNeverHappen to not leave stale types (#4910)

DAE will normally not remove an unreachable parameter, because it checks for
effects there. But in TrapsNeverHappen mode, we assume that an unreachable
is an effect we can remove, so we are willing to remove it:

(func $foo (param $unused i32)
  ;; never use $unused
)
(func $bar
  (call $foo
    (unreachable)))

;;=> dae+tnh

(func $foo
)
(func $bar
  (call $foo))

But that transformation is invalid: the call's type was unreachable before but
no longer is. What went wrong here is that, yes, it is valid to remove an
unreachable, but we may need to update types while doing so, which we
were not doing.

This wasn't noticed before due to a combination of unfortunate factors:

The main reason is that this only happens in TrapsNeverHappens mode. We
don't fuzz that, because it's difficult: that mode can assume a trap never happens,
so a trap is undefined behavior really. On real-world code this is great, but in the
fuzzer it means that the output can seem to change after optimizations.

The validator happened to be missing an error for a call that has type unreachable
but shouldn't: Validator: Validate unreachable calls more carefully #4909 . Without
that, we'd only get an error if the bad type influenced a subsequent pass in a confusing
way - which is possible, but difficult to achieve (what ended up happening in practice is
that SignatureRefining on J2Wasm relied on the unreachable and refined a type too much).

Even with that fix, for the problem to be detected we'd need for the validation error to
happen in the final output, after running all the passes. In practice, though, that's not
likely, since other passes tend to remove unreachables etc. Pass-debug mode is very
useful for finding stuff like this, as it validates after every individual pass. Sadly it turns
out that global validation was off there: Validator: Validate globally by default #4906
(so it was catching the 99% of validation errors that are local, but this particular error
was in the remaining 1%...).

As a fix, simply ignore this case. It's not really worth the effort to optimize it, since DCE
will just remove unreachables like that anyhow. So if we run again after a DCE we'd get
a chance to optimize.

This updates some existing tests to avoid (unreachable). That was used as an example
of something with effects, but after this change it is treated more carefully. Replace those
things with something else that has effects (a call).

1 files changed, 25 insertions, 5 deletions

diff --git a/src/passes/param-utils.cpp b/src/passes/param-utils.cpp
index cf7873dc0..3b0721173 100644
--- a/src/passes/param-utils.cpp
+++ b/src/passes/param-utils.cpp
@@ -62,20 +62,40 @@ bool removeParameter(const std::vector<Function*>& funcs,
   // Check if none of the calls has a param with side effects that we cannot
   // remove (as if we can remove them, we will simply do that when we remove the
   // parameter). Note: flattening the IR beforehand can help here.
-  auto hasBadEffects = [&](ExpressionList& operands) {
-    return EffectAnalyzer(runner->options, *module, operands[index])
-      .hasUnremovableSideEffects();
+  //
+  // It would also be bad if we remove a parameter that causes the type of the
+  // call to change, like this:
+  //
+  //  (call $foo
+  //    (unreachable))
+  //
+  // After removing the parameter the type should change from unreachable to
+  // something concrete. We could handle this by updating the type and then
+  // propagating that out, or by appending an unreachable after the call, but
+  // for simplicity just ignore such cases; if we are called again later then
+  // if DCE ran meanwhile then we could optimize.
+  auto hasBadEffects = [&](auto* call) {
+    auto& operands = call->operands;
+    bool hasUnremovable =
+      EffectAnalyzer(runner->options, *module, operands[index])
+        .hasUnremovableSideEffects();
+    bool wouldChangeType =
+      call->type == Type::unreachable && !call->isReturn &&
+      std::any_of(operands.begin(), operands.end(), [](Expression* operand) {
+        return operand->type == Type::unreachable;
+      });
+    return hasUnremovable || wouldChangeType;
   };
   bool callParamsAreValid =
     std::none_of(calls.begin(), calls.end(), [&](Call* call) {
-      return hasBadEffects(call->operands);
+      return hasBadEffects(call);
     });
   if (!callParamsAreValid) {
     return false;
   }
   bool callRefParamsAreValid =
     std::none_of(callRefs.begin(), callRefs.end(), [&](CallRef* call) {
-      return hasBadEffects(call->operands);
+      return hasBadEffects(call);
     });
   if (!callRefParamsAreValid) {
     return false;