Wouldn't it be simple to just assume Type::i31ref is nullable and return Type::i31ref here, and later fix Type::i31ref itself? This way it looks internally Type::i31ref is still non-nullable but when we parse i31ref we treat it as something other than i31ref which is nullable.

The same for the similar usages in wasm-s-parser.cpp and wasm-validator.cpp.

I'm not sure this would be any simpler because all the other code needs to be prepared to deal with either (ref null i31) or (ref i31). Which version is considered "Basic" because it has the Type::i31ref shorthand should really only matter for printing, unless I'm missing something.

I think it is slightly simpler in the current approach, since this is more consistent with our other behavior, which is to turn all inputs immediately into nullable types. It's true that it looks a little more awkward here, since it's no longer "basic", but it is still applying that simple rule.

Is there any case in which t.isRef() and we don't enable FeatureSet::ReferenceTypes? If it is always enabled for any reference types, can we enable it by default for t.isRef() and only additionally handle GC related stuff?

Yes, good point. I pushed a commit with that simplification now.

I'm not very sure what is this method exactly for anymore.. It seems it was added in #2675 to support multivalue printing for local types without commas, like (i32 i64), but it has since grown and its name also changed from LocalType to SExprType... Can you help me on when we just use printType and when we use this method?

Sorry, yes, it is a little confusing. I've added comments in this PR which might help. But probably we should rename things too.

• printTypeName prints a "serialized" name, as a list of characters. It has no spaces, in particular. We use this to emit a name for any type, no matter what it contains. Say an array contains (ref eq) then we'd print some serialization of that into [ref_eq] or such.
• SExprType prints out a type in normal s-format. For (ref eq) it would be exactly that (with spaces etc.). But that can recursively call printTypeName for the sub-types that it is declared from, when it needs their "serialized" name.

It would be good to standardize on either using printXXX functions or using wrapper structs with bespoke operator<< implementations, but I don't think that needs to happen here. (I prefer the latter fwiw.)

I assume eqref is short for (ref null eq) and that there is also a (ref eq). In that case this code probably should compare with rightHeap == HeapType::eq rather than with right == Type::eqref. It will have to handle differing nullability between left and right as well.

Thanks, yes. Pushed a fix now.

Just noticed that the right == Type::funcref check below should probably be changed in the same way.

Good catch, fixed.