[GC] Add Type::IsDefaultable and use that to do more validation #3456
Conversation
src/wasm/wasm-type.cpp
Outdated
| // that it is nullable. | ||
| // FIXME when we have proper non-nullability. | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I thought primitive types are defaultable too, because they can be set to 0, but this does not allow it. This seems to imply all basic types are defaultable.
There was a problem hiding this comment.
The GC MVP doc says i31ref is not, at least unless I'm misreading it
anyref == (ref null any)
eqref == (ref null eq)
dataref == (ref data)
i31ref == (ref i31)
So apparently dataref and i31ref are non-nullable?
There was a problem hiding this comment.
I wonder if i31ref is meant to default to 0? I assume (ref null i31) defaults to null regardless.
There was a problem hiding this comment.
Oh sorry nevermind, I thought this method was isDefaultable, but it was isNullable....
There was a problem hiding this comment.
Not sure if it helps, but I got this response when asking about i31ref == (ref i31) specifically.
Having a different nullability default for the i31ref shorthand was a pragmatic choice based on expected use cases, but it is a bit irregular. We can switch it around if folks think that's preferable. Opinions?
Potentially related, i31.new has recently been promoted to a constant instruction, which may or may not indicate that i31ref can now default to (i31.new (i31.const 0)).
There was a problem hiding this comment.
Thanks @dcodeIO , interesting.
Sounds like not everything is figured out in the spec about this, so we'll revisit as necessary...
src/wasm/wasm-type.cpp
Outdated
| // i31ref is always non-nullable - there is no way to declare a nullable | ||
| // i31 reference, unlike other reference types. For now, allow it in vars |
There was a problem hiding this comment.
Good question... the binaryen parser doesn't allow it. But maybe we should, as it seems like i31 is a heap type...?
In that case, we could just remove i31ref entirely, if it's just a wat shorthand for (ref i31)? That is, we could remove i31ref from the entire codebase, and whenever we need one, do Type(HeapType::i31, NonNullable).
There was a problem hiding this comment.
Well, I wouldn't go as far as removing i31ref from the codebase entirely, unless we also want to remove anyref, externref, etc. for the same reasons. But we should be using (ref null i31) rather than i31ref everywhere we are currently using e.g. externref in place of (ref extern).
There was a problem hiding this comment.
@tlively I'm not sure I follow your last sentence. What is the analogy with externref saying?
My point maybe wasn't clear before. What I am saying is this: right now we have i31ref, and we cannot read (ref null i31) (with or without null). If I add the ability to read that, then now we have two internal representations of the same type, Type::i31ref and Type(HeapType::BasicHeapType::i31, NonNullable).
The latter would not count as Type::isBasic() right now, so we'd need to fix that. I think we'd need to fix equality checking by making sure the two are interned in the same way, but I'm not sure. Instead, it seems simpler to me to have a single internal representation (the second of the two above), and to basically turn i31ref into sugar in the text format, which is immediately lowered in the s-parser. Thoughts?
(That all seems like annoying work, so I'd prefer if there's a better option!)
There was a problem hiding this comment.
Due to canonicalization, Type(HeapType::BasicHeapType::i31, NonNullable) == Type::i31ref, just like Type(HeapType::BasicHeapType::any, Nullable) == Type::anyref. That means that Type::isBasic() returns true either way.
i31ref, anyref, externref, etc. are all just shorthands for (ref i31), (ref null any), (ref null extern), etc. We could remove all these shorthands from the codebase, but it wouldn't make sense to remove just one of them.
I'm suggesting that we parse i31ref (i.e. (ref i31)) as (ref null i31) just like we currently parse (ref any) as (ref null any) (i.e. anyref).
|
Ok, after recent heap type improvements this PR does not require any hacks for i31, and just adds |
| // A variable can get a default value if its type is concrete (unreachable | ||
| // and none have no values, hence no default), and if it's a reference, it | ||
| // must be nullable. | ||
| return isConcrete() && (!isRef() || isNullable()); |
There was a problem hiding this comment.
I think so, and binaryen gives them the natural default value (rtt.canon). I admit I can't find an explanation of that on the GC docs, though. I opened WebAssembly/gc#175
kripken
changed the title
The extra validation noticed that we need to do something for i31ref. For
refs in general we just make them nullable, but we can't do that for a built-in
type like i31ref. So treat it as nullable for now, until we handle nullability
properly.