742144 - Need to support typedefs in the WebIDL parser

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Minimal webidl file that needs to work without throwing:

  typedef long foopy;
  interface Foo {
    const foopy X = 5;
  };

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 2

[Justin Lebar (not reading bugmail)]

> -    def unroll(self):
> -        return self.inner.unroll()
> +    def unroll(self, onlyTypedefs = False):
> +        return self.inner.unroll() if not onlyTypedefs else self

It seems to me that unroll(onlyTypedefs=True) is a logically separate operation from unroll(): The latter takes sequence(long?) --> long, which is a lot different from unwrapping a typedef.  It would make more sense to me if they were separate methods.

Also, what's supposed to happen when I have:

  typedef long? type1;
  typedef type1 type2;

  type2.unroll(onlyTypedefs=True)

As it stands, type2's inner is nullable(type1), which unroll(onlyTypedefs=True)'s to itself.  It seems to me that nullable(long) would be much more useful, in general.

>  class IDLNullValue(IDLObject):
>      def __init__(self, location):
>          IDLObject.__init__(self, location)
>          self.type = None
>          self.value = None
>  
>      def coerceToType(self, type, location):
> -        if not isinstance(type, IDLNullableType):
> +        unrolledType = type.unroll(onlyTypedefs = True)
> +        if not isinstance(unrolledType, IDLNullableType):
>              raise WebIDLError("Cannot coerce null value to type %s." % type,
>                                location)

Why isinstance(IDLNullableType) instead of nullable()? That seems to obviate the need for unroll(onlyTypedefs=True) here (although if implemented as suggested above, it seems useful in general).  But maybe I'm missing something...

> @@ -1266,25 +1269,31 @@ class IDLValue(IDLObject):
>          if type == self.type:
>              return self # Nothing to do
>  
>          # If the type allows null, rerun this matching on the inner type
>          if type.nullable():
>              innerValue = self.coerceToType(type.inner, location)
>              return IDLValue(self.location, type, innerValue.value)
>  
> -        # Else, see if we can coerce to 'type'.
> -        if self.type.isInteger():
> +        # We check for nullability before unrolling because unrolling will
> +        # discard nullability.
> +        unrolledType = type.unroll()
> +        if unrolledType == self.type:
> +            return self # Still nothing to do
> +
> +        # Else, see if we can coerce to 'unrolledType'.
> +        if unrolledType.isInteger():

unroll() strips off sequence and array.  So this seems wrong.

> +    def p_ConstTypeIdentifier(self, p):
> +        """
> +            ConstType : IDENTIFIER Null
> +        """
> +        identifier = IDLUnresolvedIdentifier(self.getLocation(p, 1), p[1])
> +
> +        try:
> +            obj = self.globalScope()._lookupIdentifier(identifier)
> +        except:
> +            assert False # Not yet implemented
> +
> +        assert isinstance(obj, IDLTypedefType)

Shouldn't this be a true exception?  It's certainly not an ICE if obj is not an IDLTypedefType, right?  I guess you're counting on catching the error further down the line, when we assign into the const?  You could spit out a warning here, then.

> +++ b/tests/test_type_equality.py

What is this file testing?  (Maybe you forgot to finish the test?)

> +            interface TestConstDisallowed3 {
> +              const long l = 12345678900;
> +            };

I presume this is supposed to throw because the value is too large?  If so, that probably deserves a suite of boundary checks, for the different numeric types and their signed/unsigned flavors.  You can do that separately, of course.

Comment 3

[Peter Van der Beken [:peterv]]

Can we get some traction here? HTML5 has:

[TreatNonCallableAsNull]
callback EventHandlerNonNull = any (Event event);
typedef EventHandlerNonNull? EventHandler;

Not having typedefs means replacing about 60 instances of EventHandler in HTMLElement.webidl with EventHandlerNonNull? (and back once this is fixed).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Implement support for typedefs in WebIDL.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Some general notes:

1)  I didn't implement extended attributes on the typedef.  I plan to raise a spec issue
    about it being totally underdefined.
2)  The basic idea is to never expose the typedef type to anyone outside the parser, ever,
    more or less.
3)  I didn't implement storing of the original name, because for builtins I don't know how
    to do it: it would have to be stored on the singleton or something.  We should consider
    not using singleton type objects for builtins to fix that.  Then we could also give
    them sane locations, which would have been useful a few times.

Comment 6

[Peter Van der Beken [:peterv]]

I get

  File "dom/bindings/parser/WebIDL.py", line 1886, in finish
    assert not isinstance(t, IDLTypedefType)

if in a .webidl file I use a type that's a typedef from a different .webidl file.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hrm.  Let me try that and see what's going on.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: With that fixed, and a test added

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 657498 [details] [diff] [review]
With that fixed, and a test added

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

::: dom/bindings/parser/WebIDL.py
@@ +935,5 @@
>          assert obj
>          if obj.isType():
> +            # obj itself might not be complete; deal with that.
> +            if not obj.isComplete():
> +                obj = obj.complete(scope)

Can we assert obj != self to prevent an iloop or something?

@@ +1352,5 @@
> +
> +    def complete(self, parentScope):
> +        if not self.inner.isComplete():
> +            self.inner = self.inner.complete(parentScope)
> +        return self.inner

assert self.inner.isComplete()?

@@ +1357,5 @@
> +
> +    def finish(self, parentScope):
> +        # Maybe the IDLObjectWithIdentifier for the typedef should be
> +        # a separate thing from the type?
> +        self.complete(parentScope)

Is this actually getting called?  Other types don't have a finish function.

@@ +3433,5 @@
> +
> +        try:
> +            obj = self.globalScope()._lookupIdentifier(identifier)
> +        except:
> +            assert False # Not yet implemented

You should be able to implement this, I think.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Implement support for typedefs in WebIDL.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Can we assert obj != self to prevent an iloop or something?
> assert self.inner.isComplete()?

Absolutely.  Done.

> Is this actually getting called?  Other types don't have a finish function.

Yes, it's called.  The reason is that this object is both a type and an IDLObjectWithIdentifier.  And we call finish() on the toplevel IDLObjectWithIdentifiers.  Hence the comment about splitting up the type and the thing that gets finish()ed...

> You should be able to implement this, I think.

Done in attachment 658624 [details] [diff] [review].

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Boris Zbarsky (:bz) from comment #11)
> > Is this actually getting called?  Other types don't have a finish function.
> 
> Yes, it's called.  The reason is that this object is both a type and an
> IDLObjectWithIdentifier.  And we call finish() on the toplevel
> IDLObjectWithIdentifiers.  Hence the comment about splitting up the type and
> the thing that gets finish()ed...

Aha.  Yeah, maybe we should do that in the future.  This is fine for now though.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 658624 [details] [diff] [review]
Implement support for typedefs in WebIDL.

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

::: dom/bindings/parser/tests/test_typedef.py
@@ +23,5 @@
> +          interface Foo {
> +            const mynullablelong? X = 5;
> +            void foo(mynullablelong? Y);
> +          };
> +        """)

Please add a test for

interface Foo {
  const mynullablelong? X = 5;
  ...
};

typedef long? mynullablelong;

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe317c21b749

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fe317c21b749