Closed Bug 742144 Opened 8 years ago Closed 7 years ago

Need to support typedefs in the WebIDL parser

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Minimal webidl file that needs to work without throwing:

  typedef long foopy;
  interface Foo {
    const foopy X = 5;
  };
Attached patch Patch (obsolete) — Splinter Review
Attachment #617613 - Flags: review?(justin.lebar+bug)
> -    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.
Attachment #617613 - Flags: review?(justin.lebar+bug) → review-
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).
Target Milestone: --- → mozilla16
Version: Trunk → 18 Branch
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.
Assignee: khuey → bzbarsky
Whiteboard: [need review]
Target Milestone: mozilla16 → ---
Version: 18 Branch → Trunk
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.
Hrm.  Let me try that and see what's going on.
Attachment #657417 - Attachment is obsolete: true
Attachment #657417 - Flags: review?(khuey)
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.
Attachment #657498 - Attachment is obsolete: true
Attachment #657498 - Flags: review?(khuey)
> 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].
(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 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;
Attachment #658624 - Flags: review?(khuey) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe317c21b749
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/fe317c21b749
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.