Implement nullref type, change ref.null syntax & encoding

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P2
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

7 months ago
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.
Assignee

Updated

6 months ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Assignee

Comment 1

6 months ago
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.
Assignee

Comment 2

6 months ago
Attachment #9026149 - Flags: review?(luke)
Assignee

Comment 3

6 months ago
Attachment #9026150 - Flags: review?(luke)
Assignee

Updated

6 months ago
Assignee

Updated

6 months ago
No longer blocks: 1488199
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().
Attachment #9026149 - Flags: review?(luke) → review+

Updated

6 months ago
Attachment #9026150 - Flags: review?(luke) → review+
Assignee

Comment 5

6 months ago
(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.
(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

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f03c9e85b401
https://hg.mozilla.org/mozilla-central/rev/5219f57277c4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.