Last Comment Bug 742145 - Need support for the various typed arrays types in WebIDL parser
: Need support for the various typed arrays types in WebIDL parser
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
:
Mentors:
Depends on:
Blocks: ParisBindings 748266 749864 750264
  Show dependency treegraph
 
Reported: 2012-04-03 19:32 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-05-24 21:42 PDT (History)
5 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (19.11 KB, patch)
2012-04-27 21:21 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Patch (2.28 KB, patch)
2012-05-23 11:02 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (18.94 KB, patch)
2012-05-23 11:07 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-04-03 19:32:37 PDT
We need ArrayBufferView and the various subtypes: Int8Array, Uint8Array, Uint8ClampedArray, Int16Array, Uint16Array, Int32Array, Uint32Array, Int64Array, Uint64Array, Float32Array, Float64Array.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-27 21:21:41 PDT
Created attachment 619247 [details] [diff] [review]
Patch
Comment 2 Justin Lebar (not reading bugmail) 2012-04-30 13:21:18 PDT
Comment on attachment 619247 [details] [diff] [review]
Patch

>@@ -638,16 +638,32 @@ class IDLType(IDLObject):
>         return False
> 
>     def isArray(self):
>         return False
> 
>     def isArrayBuffer(self):
>         return False
> 
>+    def isArrayBufferView(self):
>+        return False
>+
>+    def isTypedArray(self):
>+        return False
>+
>+    def isGeckoInterface(self):
>+        assert self.isInterface()
>+        return not self.isSpiderMonkeyInterface()
>+
>+    def isSpiderMonkeyInterface(self):
>+        assert self.isInterface()
>+        return self.isArrayBuffer() or \
>+               self.isArrayBufferView() or \
>+               self.isTypedArray()

I don't get why it's an error to call these without first checking that the type is an interface.  But in any case, please explain what these methods are about.

>     def isDictionary(self):
>         return False
> 
>     def isInterface(self):
>         return False
> 
>     def isAny(self):
>         return self.tag() == IDLType.Tags.any
>@@ -758,16 +774,22 @@ class IDLNullableType(IDLType):
>         return self.inner.isSequence()
> 
>     def isArray(self):
>         return self.inner.isArray()
> 
>     def isArrayBuffer(self):
>         return self.inner.isArrayBuffer()
> 
>+    def isArrayBufferView(self):
>+        return self.inner.isArrayBufferView()
>+
>+    def isTypedArray(self):
>+        return self.inner.isTypedArray()
>+
>     def isDictionary(self):
>         return self.inner.isDictionary()
> 
>     def isInterface(self):
>         return self.inner.isInterface()
> 
>     def isEnum(self):
>         return self.inner.isEnum()

Why doesn't IDLNullableType get isGeckoInterface and isSpiderMonkeyInterface?  Does this even pass the test which iterates over the "public" properties of X and nullable(X) and ensures that they're the same?  The only way I'd expect it to pass is if we never hand an interface to the test, so is{Gecko,SpiderMonkey}Interface always throws, and we always ignore the property.  But in that case, you just got lucky.  :)

I really wish we had a cleaner way of doing this...

>@@ -988,24 +1010,25 @@ class IDLTypedefType(IDLType, IDLObjectW
> 
>     def isDistinguishableFrom(self, other):
>         return self.inner.isDistinguishableFrom(other)
> 
> class IDLWrapperType(IDLType):
>     def __init__(self, location, inner):
>         IDLType.__init__(self, location, inner.identifier.name)
>         self.inner = inner
>-        self.name = inner.identifier
>+        self._identifier = inner.identifier
>         self.builtin = False
> 
>     def __eq__(self, other):
>-        return other and self.name == other.name and self.builtin == other.builtin
>+        return other and self._identifier == other._identifier and \
>+               self.builtin == other.builtin
> 
>     def __str__(self):
>-        return str(self.name.name) + " (Wrapper)"
>+        return str(self.name) + " (Wrapper)"
> 
>     def nullable(self):
>         return False
> 
>     def isPrimitive(self):
>         return False
> 
>     def isString(self):

>@@ -1122,21 +1165,30 @@ class IDLBuiltinType(IDLType):
>         return self._typeTag == IDLBuiltinType.Types.domstring
> 
>     def isInteger(self):
>         return self._typeTag <= IDLBuiltinType.Types.unsigned_long_long
> 
>     def isArrayBuffer(self):
>         return self._typeTag == IDLBuiltinType.Types.ArrayBuffer
> 
>+    def isArrayBufferView(self):
>+        return self._typeTag == IDLBuiltinType.Types.ArrayBufferView

Typed arrays inherit from ArrayBufferView, so it's kind of weird to say Uint8Array is not an ArrayBufferView.  One could always do |if isArrayBufferView and not isTypedArray| -- is |isArrayBufferView| called so often that this would be annoying?

>@@ -1758,17 +1840,27 @@ class Tokenizer(object):
>         ")": "RPAREN",
>         "[": "LBRACKET",
>         "]": "RBRACKET",
>         "?": "QUESTIONMARK",
>         ",": "COMMA",
>         "=": "EQUALS",
>         "<": "LT",
>         ">": "GT",
>-        "ArrayBuffer": "ARRAYBUFFER"
>+        "ArrayBuffer": "ARRAYBUFFER",
>+        "ArrayBufferView": "ARRAYBUFFERVIEW",
>+        "Int8Array": "INT8ARRAY",
>+        "Uint8Array": "UINT8ARRAY",
>+        "Uint8ClampedArray": "UINT8CLAMPEDARRAY",
>+        "Int16Array": "INT16ARRAY",
>+        "Uint16Array": "UINT16ARRAY",
>+        "Int32Array": "INT32ARRAY",
>+        "Uint32Array": "UINT32ARRAY",
>+        "Float32Array": "FLOAT32ARRAY",
>+        "Float64Array": "FLOAT64ARRAY"
>         }
> 
>     tokens.extend(keywords.values())
> 
>     def t_error(self, t):
>         raise WebIDLError("Unrecognized Input",
>                Location(lexer=self.lexer,
>                         lineno=self.lexer.lineno,
>@@ -2540,25 +2632,55 @@ class Parser(Tokenizer):
>         if p[5]:
>             type = IDLNullableType(self.getLocation(p, 5), type)
>         p[0] = type
> 
>     def p_AttributeTypePrimitive(self, p):
>         """
>             AttributeType : PrimitiveOrStringType TypeSuffix
>                           | ARRAYBUFFER TypeSuffix
>+                          | ARRAYBUFFERVIEW TypeSuffix
>+                          | INT8ARRAY TypeSuffix
>+                          | UINT8ARRAY TypeSuffix
>+                          | UINT8CLAMPEDARRAY TypeSuffix
>+                          | INT16ARRAY TypeSuffix
>+                          | UINT16ARRAY TypeSuffix
>+                          | INT32ARRAY TypeSuffix
>+                          | UINT32ARRAY TypeSuffix
>+                          | FLOAT32ARRAY TypeSuffix
>+                          | FLOAT64ARRAY TypeSuffix
>                           | OBJECT TypeSuffix
>                           | ANY TypeSuffixStartingWithArray
>         """
>         if p[1] == "object":
>             type = BuiltinTypes[IDLBuiltinType.Types.object]
>         elif p[1] == "any":
>             type = BuiltinTypes[IDLBuiltinType.Types.any]
>         elif p[1] == "ArrayBuffer":
>             type = BuiltinTypes[IDLBuiltinType.Types.ArrayBuffer]
>+        elif p[1] == "ArrayBufferView":
>+            type = BuiltinTypes[IDLBuiltinType.Types.ArrayBufferView]
>+        elif p[1] == "Int8Array":
>+            type = BuiltinTypes[IDLBuiltinType.Types.Int8Array]
>+        elif p[1] == "Uint8Array":
>+            type = BuiltinTypes[IDLBuiltinType.Types.Uint8Array]
>+        elif p[1] == "Uint8ClampedArray":
>+            type = BuiltinTypes[IDLBuiltinType.Types.Uint8ClampedArray]
>+        elif p[1] == "Int16Array":
>+            type = BuiltinTypes[IDLBuiltinType.Types.Int16Array]
>+        elif p[1] == "Uint16Array":
>+            type = BuiltinTypes[IDLBuiltinType.Types.Uint16Array]
>+        elif p[1] == "Int32Array":
>+            type = BuiltinTypes[IDLBuiltinType.Types.Int32Array]
>+        elif p[1] == "Uint32Array":
>+            type = BuiltinTypes[IDLBuiltinType.Types.Uint32Array]
>+        elif p[1] == "Float32Array":
>+            type = BuiltinTypes[IDLBuiltinType.Types.Float32Array]
>+        elif p[1] == "Float64Array":
>+            type = BuiltinTypes[IDLBuiltinType.Types.Float64Array]
>         else:
>             type = BuiltinTypes[p[1]]

This is a table lookup disguised as an 'if' statement.  It should probably be a true table lookup.

But more generally, I don't think adding a built-in type should require modifying the lexer.  I guess I might be OK with it if you thought this was absolutely the last time we'd ever add another built-in type like this, but I doubt that's true.  :)

Logically, shouldn't we treat Int8Array just like any other WebIDL type?  What makes a type built-in is simply that the type is considered defined whenever we're parsing another script.  It's as though there's a magic #include at the top of every file we parse.  Right?

I'll grant that "deep" built-in types like "int" may require lexer support.  And that we may want to make codegen's life easier with methods like isArrayBufferView.  But neither of those preclude what I'm suggesting.

What would you think of having something like:

 builtinTypes = {'ArrayBuffer': ['arrayBuffer', 'interface'],
                 'ArrayBufferView': ['arrayBufferView', 'interface'],
                 'Uint8Array': ['arrayBufferView', 'typedArray', 'interface'],
                 ...}

and building up the relevant IDLType objects out of this?

I don't like modifying the lexer for this, but the r- is mostly for the nullable issue; particularly, we should make sure there's a test to catch this, if our current ones don't.  I haven't looked at the tests, but I will if you convince me that this patch doesn't need code changes.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-04-30 13:45:50 PDT
> I don't get why it's an error to call these without first checking that the type is an
> interface. 

Fwiw, I nuked those asserts in my import of the patch and made them into checks instead, and asked khuey to upstream that.

> is |isArrayBufferView| called so often that this would be annoying?

No, but the _only_ time it's called what the caller wants to know is "is this an argument that accepts all array buffer views?"

In particular, the codegen needs to be able to test for the following things:

1)  This type is one of the typed array things (covered by isSpiderMonkeyInterface,
    used for landing in the right part of the codegen and for deciding which headers to
    include).
2)  This argument should allow passing an ArrayBuffer (isArrayBuffer()).
3)  This argument should allow passing any ArrayBufferView (isArrayBufferView()).
4)  This argument should allow passing a specific kind of ArrayBufferView (isTypedArray()
    plus .name to see which kind)
5-7) as 2-4, but with s/argument/return value/ and s/passing returning/.
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-23 11:02:47 PDT
Created attachment 626506 [details] [diff] [review]
Patch
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-23 11:07:15 PDT
Created attachment 626510 [details] [diff] [review]
Patch
Comment 6 Justin Lebar (not reading bugmail) 2012-05-23 13:00:47 PDT
Comment on attachment 626510 [details] [diff] [review]
Patch

>@@ -988,24 +1024,25 @@ class IDLTypedefType(IDLType, IDLObjectW
> 
>     def isDistinguishableFrom(self, other):
>         return self.inner.isDistinguishableFrom(other)
> 
> class IDLWrapperType(IDLType):
>     def __init__(self, location, inner):
>         IDLType.__init__(self, location, inner.identifier.name)
>         self.inner = inner
>-        self.name = inner.identifier
>+        self._identifier = inner.identifier
>         self.builtin = False
> 
>     def __eq__(self, other):
>-        return other and self.name == other.name and self.builtin == other.builtin
>+        return other and self._identifier == other._identifier and \
>+               self.builtin == other.builtin
> 
>     def __str__(self):
>-        return str(self.name.name) + " (Wrapper)"
>+        return str(self.name) + " (Wrapper)"
> 
>     def nullable(self):
>         return False
> 
>     def isPrimitive(self):
>         return False
> 
>     def isString(self):

I don't completely understand this change, probably because I can't figure out what IDLWrapperType is for...  But okay, this seems reasonable.  :)

>@@ -1212,17 +1290,47 @@ BuiltinTypes = {
>+      IDLBuiltinType.Types.ArrayBufferView:
>+          IDLBuiltinType(BuiltinLocation("<builtin type>"), "ArrayBufferView",
>+                         IDLBuiltinType.Types.ArrayBufferView),
>+      IDLBuiltinType.Types.Int8Array:
>+          IDLBuiltinType(BuiltinLocation("<builtin type>"), "Int8Array",
>+                         IDLBuiltinType.Types.Int8Array),
>+      IDLBuiltinType.Types.Uint8Array:
>+          IDLBuiltinType(BuiltinLocation("<builtin type>"), "Uint8Array",
>+                         IDLBuiltinType.Types.Uint8Array),
>+      IDLBuiltinType.Types.Uint8ClampedArray:
>+          IDLBuiltinType(BuiltinLocation("<builtin type>"), "Uint8ClampedArray",
>+                         IDLBuiltinType.Types.Uint8ClampedArray),
>+      IDLBuiltinType.Types.Int16Array:
>+          IDLBuiltinType(BuiltinLocation("<builtin type>"), "Int16Array",
>+                         IDLBuiltinType.Types.Int16Array),
>+      IDLBuiltinType.Types.Uint16Array:
>+          IDLBuiltinType(BuiltinLocation("<builtin type>"), "Uint16Array",
>+                         IDLBuiltinType.Types.Uint16Array),
>+      IDLBuiltinType.Types.Int32Array:
>+          IDLBuiltinType(BuiltinLocation("<builtin type>"), "Int32Array",
>+                         IDLBuiltinType.Types.Int32Array),
>+      IDLBuiltinType.Types.Uint32Array:
>+          IDLBuiltinType(BuiltinLocation("<builtin type>"), "Uint32Array",
>+                         IDLBuiltinType.Types.Uint32Array),
>+      IDLBuiltinType.Types.Float32Array:
>+          IDLBuiltinType(BuiltinLocation("<builtin type>"), "Float32Array",
>+                         IDLBuiltinType.Types.Float32Array),
>+      IDLBuiltinType.Types.Float64Array:
>+          IDLBuiltinType(BuiltinLocation("<builtin type>"), "Float64Array",
>+                         IDLBuiltinType.Types.Float64Array)
>     }

I think we could make this less of a mouthful, but let's not worry about it in this bug.

>@@ -2828,24 +2936,36 @@ class Parser(Tokenizer):
>             raise WebIDLError("invalid syntax", Location(self.lexer, p.lineno, p.lexpos, self._filename))
> 
>     def __init__(self, outputdir=''):
>         Tokenizer.__init__(self, outputdir)
>         self.parser = yacc.yacc(module=self,
>                                 outputdir=outputdir,
>                                 tabmodule='webidlyacc')
>         self._globalScope = IDLScope(BuiltinLocation("<Global Scope>"), None, None)
>+        self._installBuiltins(self._globalScope)
>         self._productions = []
> 
>         self._filename = "<builtin>"
>         self.lexer.input(Parser._builtins)
>         self._filename = None
> 
>         self.parser.parse(lexer=self.lexer)
> 
>+    def _installBuiltins(self, scope):
>+        assert isinstance(scope, IDLScope)
>+
>+        # xrange omits the last value.
>+        for x in xrange(IDLBuiltinType.Types.ArrayBuffer, IDLBuiltinType.Types.Float64Array + 1):

Gah, more hardcoding of element placement in these magic arrays.  Could we at
least do

  for t in IDLBuiltinType.Types[IDLBuiltinType.Types.ArrayBuffer:]:

so we only hardcode the beginning, rather than the beginning and the end?

>diff --git a/tests/test_incomplete_types.py b/tests/test_incomplete_types.py
>--- a/tests/test_incomplete_types.py
>+++ b/tests/test_incomplete_types.py
>@@ -27,18 +27,18 @@ def WebIDLTest(parser, harness):
>     harness.ok(isinstance(attr, WebIDL.IDLAttribute),
>                "Should be an IDLAttribute")
>     method = iface.members[1]
>     harness.ok(isinstance(method, WebIDL.IDLMethod),
>                "Should be an IDLMethod")
> 
>     harness.check(attr.identifier.QName(), "::TestIncompleteTypes::attr1",
>                   "Attribute has the right QName")
>-    harness.check(attr.type.name.QName(), "::FooInterface",
>+    harness.check(attr.type.name, "FooInterface",
>                   "Previously unresolved type has the right name")
> 
>     harness.check(method.identifier.QName(), "::TestIncompleteTypes::method1",
>                   "Attribute has the right QName")
>     (returnType, args) = method.signatures()[0]
>-    harness.check(returnType.name.QName(), "::FooInterface",
>+    harness.check(returnType.name, "FooInterface",
>                   "Previously unresolved type has the right name")
>-    harness.check(args[0].type.name.QName(), "::FooInterface",
>+    harness.check(args[0].type.name, "FooInterface",
>                   "Previously unresolved type has the right name")

Why isn't args[0].type.identifier available?

I think this is basically right, in any case, so r=me.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-23 13:30:51 PDT
> I don't completely understand this change, probably because I can't figure
> out what IDLWrapperType is for...  But okay, this seems reasonable.  :)

IDLWrapperType is for interfaces/etc that are actual IDLObjects and also types.  This change keeps IDLWrapperType from overwriting IDLType's name field (python ftl :-/)

> I think we could make this less of a mouthful, but let's not worry about it
> in this bug.

Yeah, probably.

> Gah, more hardcoding of element placement in these magic arrays.  Could we at
> least do
> 
>   for t in IDLBuiltinType.Types[IDLBuiltinType.Types.ArrayBuffer:]:
> 
> so we only hardcode the beginning, rather than the beginning and the end?

We could, but I worry about the future when someone adds another type.

> Why isn't args[0].type.identifier available?

This was unnecessary and has been removed.

> I think this is basically right, in any case, so r=me.

Woot.
Comment 8 Justin Lebar (not reading bugmail) 2012-05-23 14:02:17 PDT
> This change keeps IDLWrapperType from overwriting IDLType's name field (python ftl :-/)

Shoulda used self.__name, apparently.

> We could, but I worry about the future when someone adds another type.

I worry about that either way.  Certainly the right solution would be to have two separate arrays (perhaps one is a subset of the other).

> This was unnecessary and has been removed.

So all the changes to test_incomplete_types.py are gone?
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-23 14:06:24 PDT
> > This was unnecessary and has been removed.
> 
> So all the changes to test_incomplete_types.py are gone?

Yes.
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-05-24 21:42:45 PDT
https://hg.mozilla.org/mozilla-central/rev/7a9a2341db11

Note You need to log in before you can comment on or make changes to this bug.