Assertion failed: isRef() (/src/src/wasm/wasm-type.cpp: getHeapType: 930)

[nth10sd]

(module
  (func (param i32)
    loop
      block
        local.get 0
        ref.as_non_null
      end
    end
  )
)

shows an assertion failure in wasm-as at Assertion failed: isRef() (/src/src/wasm/wasm-type.cpp: getHeapType: 930).

This assertion failure does not seem to show up in version 117, only in 118.

I haven't been able to check the latest comment on the main branch and I haven't checked if #6741 will fix this.

@kripken, any thoughts? Perhaps it's the same as #6738?

[kripken]

Hmm, that looks different. The error there is in the validator, but the error here, based on the stack trace, is from WATParser->IRBuilder->RefAs::finalize().

We can't really fix this in that finalize method because it is not an error-reporting function. So the callers need to handle it. @tlively you probably know the best place for that in WATParser/IRBuilder?

Though, another option might be for finalize to set the type to unreachable. Then we'd keep on truckin' til the validator, which would be able to show a proper error. A downside is the unreachable type might be confusing during debugging, but OTOH the IR is not valid anyhow, so maybe that's ok?

[tlively]

We could add a one-off check in IRBuilder that the child of a RefAs is a reference, but there might be arbitrarily many similar assumptions in the finalization methods of other instructions. We might want to consider a more systematic approach.

We already have code that checks whether children meet the type requirements of their parents in IRBuilder, but we only use it when parsing unreachable code. We intentionally skip these checks for normal code, preferring to leave error detection and reporting to the validator. One option would be to enable these checks for all code, which would let us produce high-quality error messages with source location information during parsing, which would be a good usability improvement over the status quo.

The downside of this approach would be that it would make testing the validator implementation of type checking much harder, since errors would not get past the parser. This would be less of a concern if we used the same code to do type checking in both the parser and validator, as suggested in #6613.

[kripken]

@tlively Good points. Yeah, if it makes testing the validator harder then it seems too risky to me, so it should wait on #6613.

Meanwhile, what do you think about RefAs::finalize() as making the type unreachable when it is invalid? I am coming around to liking that actually. It defers the full error handling to the validator, and unreachable is reasonable in the sense that the code is nonsensical / cannot be executed.

[tlively]

Yeah, I'm fine with that as a short term solution, as long as it comes with a comment explaining that it's a bit of a hack.