1289315 - Union types in iterable<> declarations fail in WebIDL parser

Status: RESOLVED FIXED

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

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

According to the spec, Union types are valid in iterable<> declarations on an interface, and are used in things like the CSS Typed OM spec. However, using union types in an iterable fails in the WebIDL parser. 

Example backtrace:


  File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/share/code/mozbuild/1201590-webmidi-dom/python/mozbuild/mozbuild/action/webidl.py", line 19, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/share/code/mozbuild/1201590-webmidi-dom/python/mozbuild/mozbuild/action/webidl.py", line 15, in main
    manager.generate_build_files()
  File "/share/code/mozbuild/1201590-webmidi-dom/dom/bindings/mozwebidlcodegen/__init__.py", line 271, in generate_build_files
    written, deps = self._generate_build_files_for_webidl(filename)
  File "/share/code/mozbuild/1201590-webmidi-dom/dom/bindings/mozwebidlcodegen/__init__.py", line 473, in _generate_build_files_for_webidl
    root = CGBindingRoot(self._config, binding_stem, filename)
  File "/share/code/mozbuild/1201590-webmidi-dom/dom/bindings/Codegen.py", line 13480, in __init__
    jsImplemented)
  File "/share/code/mozbuild/1201590-webmidi-dom/dom/bindings/Codegen.py", line 1165, in __init__
    None))
  File "/share/code/mozbuild/1201590-webmidi-dom/dom/bindings/Codegen.py", line 1060, in addHeadersForType
    headerSet.add(self.getUnionDeclarationFilename(config, unrolled))
  File "/share/code/mozbuild/1201590-webmidi-dom/dom/bindings/Codegen.py", line 1231, in getUnionDeclarationFilename
    assert len(config.filenamesPerUnion[unionType.name]) == 1
AssertionError

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

From email thread between :jyc and :bz, sounds like this is due to the way header includes are created during maplike/setlike creation.

Quoting :bz, fix is:

"Change getTypesFromDescriptor in Configuration.py so that if descriptor.interface has a maplikeOrSetlikeOrIterable its key and value type (whichever exists) are added to the list of types returned from that function."

This probably means that union types will fail in maplike/setlike too. We don't have tests for unions on any of these interface types yet, so I'm also curious whether some of our autogenerated functions will fail with union types. 

I'll attach a quick test patch I wrote up for this specific issue, but we should probably land this with tests for all types added to the bindings test set.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Test Patch for union types in iterators

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

bz, not sure where you were at with this in the email chain. If you've got a quick fix, lemme know, otherwise feel free to assign to me and I'll take a look.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Go for it.  I don't have a fix written past what comment 1 says.  But I think Jonathan might have one.

Comment 5

[Jonathan Chan [:jyc]]

This is what I applied following bz's comments -- it lets me compile with the WebIDL snippet I sent. Don't know if more is needed.

diff --git a/dom/bindings/Configuration.py b/dom/bindings/Configuration.py
--- a/dom/bindings/Configuration.py
+++ b/dom/bindings/Configuration.py
@@ -706,16 +706,24 @@ def getTypesFromDescriptor(descriptor):
     types = []
     for s in signatures:
         assert len(s) == 2
         (returnType, arguments) = s
         types.append(returnType)
         types.extend(a.type for a in arguments)
 
     types.extend(a.type for a in members if a.isAttr())
+
+    if descriptor.interface.maplikeOrSetlikeOrIterable:
+        maplikeOrSetlikeOrIterable = descriptor.interface.maplikeOrSetlikeOrIterable
+        if maplikeOrSetlikeOrIterable.hasKeyType():
+            types.append(maplikeOrSetlikeOrIterable.keyType)
+        if maplikeOrSetlikeOrIterable.hasValueType():
+            types.append(maplikeOrSetlikeOrIterable.valueType)
+
     return types
 
 
 def getFlatTypes(types):
     retval = set()
     for type in types:
         type = type.unroll()
         if type.isUnion():

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, I tested this fix and it works. I also didn't realize how union types were generated by the bindings, so I think generated functions will be ok (though we still need documentation, I'll file a followup for that). 

Once I get the test fleshed out a little more, I'll go ahead and land the patch (which will look like a self review but isn't :) ).

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Test Patch for union types in iterators (Updates)

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Fix Union Types for Maplike/Setlike/Iterable in WebIDL; r=qdot

I'm marking this as self review, but it's actually bz/jyc's code I'm r+'ing. However, if anything does go wrong it'd probably land on me anyways. :)

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Mochitest for Union Type WebIDL Maplike/Setlike/Iterable fix; r=qdot

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8774993 [details] [diff] [review]
Patch 2 (v1) - Mochitest for Union Type WebIDL Maplike/Setlike/Iterable fix; r=qdot

Just need a quick r+ on the webidl change from a DOM peer.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> but it's actually bz/jyc's code I'm r+'ing

Just set the user in the patch to jyc?  He wrote the patch, after all.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8774993 [details] [diff] [review]
Patch 2 (v1) - Mochitest for Union Type WebIDL Maplike/Setlike/Iterable fix; r=qdot

>+TestInterfaceIterableDoubleUnion::Constructor(const GlobalObject& aGlobal,
>+                                         ErrorResult& aRv)

Please fix the indent.

>+      // Simple dual type iterable creation and functionality test
>+       info("IterableDoubleUnion: Testing simple iterable creation and functionality");

And here.

r=me

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae6fac030deb
Fix union types for maplike/setlike/iterable in WebIDL parser; r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/44d0ef641fa7
Tests for Iterable<> Union Types in WebIDL Interfaces; r=bz

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> > but it's actually bz/jyc's code I'm r+'ing
> 
> Just set the user in the patch to jyc?  He wrote the patch, after all.

Ah, ok, did that. Sorry about that, misunderstood code trail between bug comments and email.

All patched up and pushed to inbound.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae6fac030deb
https://hg.mozilla.org/mozilla-central/rev/44d0ef641fa7