1505774 - Implement nullref type, change ref.null syntax & encoding

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P2)

Description

[Lars T Hansen [:lth]]

When the first prototype of anyref landed, it dealt with null values in a conservative way: null expressions carry their type, so type checking was not complicated by inferring the type of a null.

The current reftypes proposal instead says that `ref.null` has the type `nullref`, and defines how nullref is used by the type calculus (effectively, it upcasts to anyref and funcref).  This requires us to introduce the nullref type, and to change the type calculus in a number of places.

It'll be useful to remember that in the future, nullref will not upcast to all ref types.

Comment 1

[Lars T Hansen [:lth]]

Ran into a minor bug in existing code:

new WebAssembly.Module(wasmTextToBinary(
    `(module
       (gc_feature_opt_in 1)
       (type $T (struct (field i32)))
       (type $U (struct (field i32) (field f64)))
       (global $g (ref $T) (ref.null (ref $U))))`));

The bug is that the null value of type (ref $U) should upcast to (ref $T) as it does in other contexts.  I'm bringing it up here and not in its own ticket because the bug boils away with the work on nullref.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1505774-nullref.patch

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1505774-test-cases-for-nullref.patch

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 9026149 [details] [diff] [review]
bug1505774-nullref.patch

Review of attachment 9026149 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, good point.  r+ assuming this nits make sense:

::: js/src/wasm/WasmOpIter.h
@@ +649,5 @@
>      }
>  
> +    if (env_.gcTypesEnabled() == HasGcTypes::True &&
> +        observed.isReference() && expected.isReference() &&
> +        env_.refTypesAreUnifiable(NonAnyToValType(observed), NonAnyToValType(expected)))

Unification is, as I understand it, a symmetric operation which asks whether there is a substitution for type variables in two type expressions to make the two type expressions the same.  However, this lone <: is asymmetric, so a pre-existing issue I failed to point out last time around is that there shouldn't be a lone IsSubtypeOf() test inside a function named Unify().  I don't think the fix is to just rename Unify() to IsSubtypeOf() either, since what does 'result' even mean in that context; you have to go read how IsSubtypeOf() is used and see that only topWithType() uses and only then you can reason about the meaning.

So I'd suggest:
- killing refTypesAreUnifiable(), using isRefSubtypeOf() instead
- killing Unify(), inlining it into its two uses, where popWithType()'s use collapses down to just a conditional

That way the isRefSubtypeOf() makes sense in the context of popWithType()/topWithType(), allowing topWithType() to do its special thing for ValType::Any which is quite independent of the subtype query.

(On a side note, renaming ValType::Any to ValType::TVar seems attractive to avoid confusion with anyref and the unlikely-future "any" wasm value type, which would be completely different.)

::: js/src/wasm/WasmTypes.h
@@ +367,4 @@
>          return UnpackTypeCodeType(tc_) == TypeCode::Ref;
>      }
>  
> +    bool isReference() const {

the straw that broke the camel's back ;)

::: js/src/wasm/WasmValidate.cpp
@@ +2050,5 @@
>          if (!globals[i].isImport() || globals[i].isMutable()) {
>              return d.fail("initializer expression must reference a global immutable import");
>          }
> +        if (expected.isReference()) {
> +            if (!env->typesAreUnifiable(globals[i].type(), expected)) {

I would think this should be env->isSubtypeOf() since we're not unifying, just assigning a value of one type to a global of another.  Since this is the only use of typesAreUnifiable(), I'd just rename it to isSubtypeOf() and move it to be under isRefSubtypeOf().

Comment 5

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #4)
> Comment on attachment 9026149 [details] [diff] [review]
> bug1505774-nullref.patch
> 
> 
> ::: js/src/wasm/WasmOpIter.h
> @@ +649,5 @@
> >      }
> >  
> > +    if (env_.gcTypesEnabled() == HasGcTypes::True &&
> > +        observed.isReference() && expected.isReference() &&
> > +        env_.refTypesAreUnifiable(NonAnyToValType(observed), NonAnyToValType(expected)))
> 
> Unification is, as I understand it, a symmetric operation which asks whether
> there is a substitution for type variables in two type expressions to make
> the two type expressions the same.  However, this lone <: is asymmetric, so
> a pre-existing issue I failed to point out last time around is that there
> shouldn't be a lone IsSubtypeOf() test inside a function named Unify().  I
> don't think the fix is to just rename Unify() to IsSubtypeOf() either, since
> what does 'result' even mean in that context; you have to go read how
> IsSubtypeOf() is used and see that only topWithType() uses and only then you
> can reason about the meaning.
> 
> So I'd suggest:
> - killing refTypesAreUnifiable(), using isRefSubtypeOf() instead
> - killing Unify(), inlining it into its two uses, where popWithType()'s use
> collapses down to just a conditional
> 
> That way the isRefSubtypeOf() makes sense in the context of
> popWithType()/topWithType(), allowing topWithType() to do its special thing
> for ValType::Any which is quite independent of the subtype query.

That worked out pretty well (ditto in WasmValidate.cpp).

> (On a side note, renaming ValType::Any to ValType::TVar seems attractive to
> avoid confusion with anyref and the unlikely-future "any" wasm value type,
> which would be completely different.)

Did this too, small amount of work.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f03c9e85b401
Update test cases for NullRef. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/5219f57277c4
Introduce nullref type. r=luke

Comment 7

[Luke Wagner [:luke]]

(In reply to Lars T Hansen [:lth] from comment #5)
> That worked out pretty well (ditto in WasmValidate.cpp).

Excellent; thanks!  As a follow-up to be rolled into some other random patch, could you also rename NonAnyToValType() to NonTVarToValType()?

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f03c9e85b401
https://hg.mozilla.org/mozilla-central/rev/5219f57277c4