IDL parser rewrite in Python

Source/bindings/scripts/idl_definitions_builder.py

Index: Source/bindings/scripts/idl_definitions_builder.py
diff --git a/Source/bindings/scripts/idl_definitions_builder.py b/Source/bindings/scripts/idl_definitions_builder.py
new file mode 100644
index 0000000000000000000000000000000000000000..946fff478517b3e0e4c8b6a530db09ac6e8789c8
--- /dev/null
+++ b/Source/bindings/scripts/idl_definitions_builder.py
@@ -0,0 +1,506 @@
+# Copyright (C) 2013 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#     * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+#     * Neither the name of Google Inc. nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+"""Read an IDL file or complete IDL interface, producing an IdlDefinitions object."""

[haraken, 2013/07/17 02:44:49]

Slightly better: "Build an IdlDefinitions object from AST (which is built by
idl_parser)".

[Nils Barth (inactive), 2013/07/17 12:05:09]

Oops, you’re right (this was the idl_reader comment). Fixed.

+
+from idl_definitions import IdlDefinitions, IdlInterface, IdlException, IdlOperation, IdlCallbackFunction, IdlArgument, IdlAttribute, IdlConstant, IdlEnum, IdlTypedef, IdlUnionType
+
+SPECIAL_KEYWORD_LIST = ['GETTER', 'SETTER', 'DELETER']
+
+
+def file_node_to_idl_definitions(node):

[haraken, 2013/07/17 02:44:49]

For readability, I might want to add one more helper method which is exported to
idl_compiler.py.

  def build_idl_definitions_from_ast(ast):
    file_node_to_idl_definitions(ast)

[Nils Barth (inactive), 2013/07/17 12:05:09]

You’re right, that’s clearer.
I actually had this earlier and removed it as part of removing “AST”
as much as possible, but for the interface it’s clearer this way.
Fixed, with slight refactoring too.

+    if node is None:
+        return None
+    node_class = node.GetClass()
+    if node_class != 'File':
+        raise ValueError('Unrecognized node class: %s' % node_class)
+    callback_functions = []
+    enumerations = []
+    exceptions = {}
+    file_name = node.GetName()  # FIXME: only needed for Perl, remove later
+    interfaces = {}
+    typedefs = {}
+
+    children = node.GetChildren()
+    for child in children:
+        child_class = child.GetClass()
+        if child_class == 'Interface':
+            interface = interface_node_to_idl_interface(child)
+            interfaces[interface.name] = interface
+        elif child_class == 'Exception':
+            exception = exception_node_to_idl_exception(child)
+            exceptions[exception.name] = exception
+        elif child_class == 'Typedef':
+            type_name = child.GetName()
+            typedefs[type_name] = typedef_node_to_idl_typedef(child)
+        elif child_class == 'Enum':
+            enumerations.append(enum_node_to_idl_enum(child))
+        elif child_class == 'Callback':
+            callback_functions.append(callback_node_to_idl_callback_function(child))
+        elif child_class == 'Implements':
+            # Implements is handled at the interface merging step
+            pass

[haraken, 2013/07/17 02:44:49]

Help me understand: Why can we skip 'Implements'? Even if we resolve the
dependency in the later phase, I guess we need to include the information of
'Implements' to IdlDefinitions.

[Nils Barth (inactive), 2013/07/17 12:05:09]

We should probably handle Implements differently (also Parents), but
that’s a bigger change, as part of fixing the messy dependency resolution.
What’s done now is that the code generator knows nothing about implements.
If we have “A implements B”, then the interface resolver parses A and B, puts
them both into interface “A”, and discards B.
So this is on my todo list, but parser ignores implements for now.

+        else:
+            raise ValueError('Unrecognized node class: %s' % child_class)
+
+    return IdlDefinitions(callback_functions=callback_functions, enumerations=enumerations, exceptions=exceptions,  file_name=file_name, interfaces=interfaces, typedefs=typedefs)

[haraken, 2013/07/17 02:44:49]

Nit: One unnecessary space before file_name.

[Nils Barth (inactive), 2013/07/17 12:05:09]

(>.<) Fixed.

+
+# Interface
+
+
+def interface_node_to_idl_interface(node):
+    attributes = []
+    constants = []
+    constructors = None
+    custom_constructors = None
+    extended_attributes = None
+    functions = []
+    is_callback = node.GetProperty('CALLBACK') or False
+    # FIXME: uppercase 'Partial' in base IDL parser
+    is_partial = node.GetProperty('Partial') or False
+    name = node.GetName()
+    parent = None
+
+    children = node.GetChildren()
+    for child in children:
+        child_class = child.GetClass()
+        if child_class == 'Attribute':
+            attribute = attribute_node_to_idl_attribute(child)
+            clear_constructor_attributes(attribute.extended_attributes)

[haraken, 2013/07/17 02:44:49]

Would you add the following FIXME?

  FIXME: This is a hack to support [CustomConstructor] for
window.HTMLImageElement. Remove the hack.

[Nils Barth (inactive), 2013/07/17 12:05:09]

Done.
Thanks for explaining why we have this!

+            attributes.append(attribute)
+        elif child_class == 'Const':
+            constants.append(constant_node_to_idl_constant(child))
+        elif child_class == 'ExtAttributes':
+            extended_attributes = ext_attributes_node_to_extended_attributes(child)

[haraken, 2013/07/17 02:44:49]

Don't you need to call clear_constructor_attributes(extended_attributes) ?

[Nils Barth (inactive), 2013/07/17 12:05:09]

The clearing is done as part of extended_attributes_to_constructors.
You’re right though, it’s clearer to explicitly call
clear_constructor_attributes.
Changed.

+            constructors, custom_constructors = extended_attributes_to_constructors(extended_attributes)
+        elif child_class == 'Operation':
+            functions.append(operation_node_to_idl_operation(child))
+        elif child_class == 'Inherit':
+            parent = child.GetName()
+        elif child_class == 'ExceptionFieldToString':

[haraken, 2013/07/17 02:44:49]

I don't fully understand why this is needed.

[Nils Barth (inactive), 2013/07/17 12:05:09]

(See above; this is a single special-case Operation.)

+            functions.append(exception_field_to_string_node_to_idl_operation(child))
+        else:
+            raise ValueError('Unrecognized node class: %s' % child_class)
+
+    return IdlInterface(name=name, attributes=attributes, constants=constants, constructors=constructors, custom_constructors=custom_constructors, extended_attributes=extended_attributes, functions=functions, is_callback=is_callback, is_partial=is_partial, parent=parent)
+
+
+def attribute_node_to_idl_attribute(node):
+    data_type = None
+    extended_attributes = {}
+    is_nullable = False
+    is_read_only = node.GetProperty('READONLY') or False
+    is_static = node.GetProperty('STATIC') or False
+    name = node.GetName()
+
+    children = node.GetChildren()
+    for child in children:
+        child_class = child.GetClass()
+        if child_class == 'Type':
+            data_type = type_node_to_type(child)
+            is_nullable = child.GetProperty('NULLABLE') or False
+        elif child_class == 'ExtAttributes':
+            extended_attributes = ext_attributes_node_to_extended_attributes(child)
+        else:
+            raise ValueError('Unrecognized node class: %s' % child_class)
+
+    return IdlAttribute(data_type=data_type, extended_attributes=extended_attributes, is_nullable=is_nullable, is_read_only=is_read_only, is_static=is_static, name=name)
+
+
+def constant_node_to_idl_constant(node):
+    name = node.GetName()
+
+    children = node.GetChildren()
+    num_children = len(children)

[haraken, 2013/07/17 02:44:49]

Shall we check that num_children > 1 ?

[Nils Barth (inactive), 2013/07/17 12:05:09]

Good point.
It would have thrown an error later, but better to make it explicit.
Done, and changed passim.

+    if num_children > 3:
+        raise ValueError('Expected at most 3 children, got %s' % num_children)
+
+    type_node = children[0]
+    data_type = type_node_inner_to_type(type_node)
+
+    value_node = children[1]
+    value_node_class = value_node.GetClass()
+    if value_node_class != 'Value':
+        raise ValueError('Expected Value node, got %s' % value_node_class)
+    value = value_node.GetName()
+
+    extended_attributes = None
+    if num_children == 3:
+        ext_attributes_node = children[2]
+        extended_attributes = ext_attributes_node_to_extended_attributes(ext_attributes_node)
+
+    return IdlConstant(data_type=data_type, extended_attributes=extended_attributes, name=name, value=value)
+
+
+def operation_node_to_idl_operation(node):
+    name = node.GetName()
+    # FIXME: AST should use None internally
+    if name == '_unnamed_':
+        name = None
+
+    is_static = node.GetProperty('STATIC') or False
+    specials = []
+    property_dictionary = node.GetProperties()
+    for special_keyword in SPECIAL_KEYWORD_LIST:

[haraken, 2013/07/17 02:44:49]

Why haven't the special keywords parsed in the parser? It seems a bit strange to
me that SPECIAL_KEYWORD_LIST appears not in blink_idl_parser but in
idl_definitions_buider.py.

[Nils Barth (inactive), 2013/07/17 12:05:09]

The special keywords are handled by the lexer and parser (the base Pepper lexer
and parser),
but we need this explicit checking because the parser puts all the special
keywords
as properties (in property_dictionary), rather than as a list.
I.e., the parser *flattens* the specials list into properties (along with static
etc.),
so we need to explicitly reconstruct the list.

It would be clearer if there were a specials_node whose children were the
specials,
so we could just loop over its children instead of hardcoding
a list of properties to extract.
I’ve added a FIXME.

Here’s where the base parser handles specials:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/idl_parser/i...

[haraken, 2013/07/21 14:31:50]

Got it. Given that static, const etc are stored in property_dictionary, it will
also make sense to store special-keywords in property_dictionary. Namely, your
current implementation looks good.

+        if special_keyword in property_dictionary:
+            specials.append(special_keyword.lower())
+
+    extended_attributes = None
+    arguments = []
+    return_type = None
+    children = node.GetChildren()
+    for child in children:
+        child_class = child.GetClass()
+        if child_class == 'Arguments':
+            arguments = arguments_node_to_arguments(child)
+        elif child_class == 'Type':
+            return_type = type_node_to_type(child)
+        elif child_class == 'ExtAttributes':
+            extended_attributes = ext_attributes_node_to_extended_attributes(child)
+        else:
+            raise ValueError('Unrecognized node class: %s' % child_class)
+
+    return IdlOperation(name=name, data_type=return_type, extended_attributes=extended_attributes, is_static=is_static, arguments=arguments, specials=specials)
+
+
+def arguments_node_to_arguments(arguments_node):

[haraken, 2013/07/17 02:44:49]

Nit: arguments_node => node (for consistency)

[Nils Barth (inactive), 2013/07/17 12:05:09]

(>.<) Done.

+    if arguments_node is None:

[haraken, 2013/07/17 02:44:49]

Is this check needed?

[Nils Barth (inactive), 2013/07/17 12:05:09]

It makes handling constructors much easier, since it saves us 3 checks
in extended_attributes_to_constructors.
This is because when the extended attribute [Constructor] (etc.) is parsed,
no Arguments node is created.
We could fix this in the IDLs by having [Constructor()] instead of
[Constructor],
which is perhaps clearer, but a bit long.
I've added a comment; is it useful?

[haraken, 2013/07/21 14:31:50]

Makes sense. Given that the spec allows both [Constructor] and [Constructor()],
we should support both as well.

[Nils Barth (inactive), 2013/07/22 06:32:01]

Got it. Didn’t realize that [Constructor] was one of the spec-specified extended
attributes, not one of the (many) Blink-only ones.

I've added a comment and link to spec:
http://dev.w3.org/2006/webapi/WebIDL/#es-extended-attributes

+        return None
+    arguments = []
+    argument_node_list = arguments_node.GetChildren()
+    for argument_node in argument_node_list:
+        arguments.append(argument_node_to_idl_argument(argument_node))
+    return arguments
+
+
+def argument_node_to_idl_argument(node):
+    name = node.GetName()
+
+    data_type = None
+    extended_attributes = {}
+    # FIXME: Boolean values are inconsistent due to Perl compatibility.
+    # Make all default to False once Perl removed.

[haraken, 2013/07/17 02:44:49]

You can remove this comment, as the same thing is written below.

[Nils Barth (inactive), 2013/07/17 12:05:09]

Done.
(Actual delete the nested comment, so it’s clear we want to change all cases.)

+    is_nullable = False
+    is_optional = node.GetProperty('OPTIONAL')
+    is_variadic = None
+    children = node.GetChildren()
+    for child in children:
+        child_class = child.GetClass()
+        if child_class == 'Type':
+            data_type = type_node_to_type(child)
+            # FIXME: Should end "or False", but use None instead for Perl
+            # compatibility.
+            is_nullable = child.GetProperty('NULLABLE')
+        elif child_class == 'ExtAttributes':
+            extended_attributes = ext_attributes_node_to_extended_attributes(child)
+        elif child_class == 'Argument':
+            child_name = child.GetName()
+            if child_name != '...':
+                raise ValueError('Unrecognized Argument node; expected "...", got "%s"' % child_name)
+            is_variadic = child.GetProperty('ELLIPSIS') or False
+        else:
+            raise ValueError('Unrecognized node class: %s' % child_class)
+
+    return IdlArgument(name=name, data_type=data_type, extended_attributes=extended_attributes, is_nullable=is_nullable, is_optional=is_optional, is_variadic=is_variadic)
+
+# Minor definitions
+
+
+def callback_node_to_idl_callback_function(node):
+    name = node.GetName()
+    children = node.GetChildren()
+    num_children = len(children)
+    if num_children != 2:
+        raise ValueError('Expected 2 children, got %s' % num_children)
+
+    type_node = children[0]
+    data_type = type_node_to_type(type_node)
+
+    arguments_node = children[1]
+    arguments_node_class = arguments_node.GetClass()
+    if arguments_node_class != 'Arguments':
+        raise ValueError('Expected Value node, got %s' % arguments_node_class)
+    arguments = arguments_node_to_arguments(arguments_node)
+
+    return IdlCallbackFunction(name=name, data_type=data_type, arguments=arguments)
+
+
+def enum_node_to_idl_enum(node):
+    name = node.GetName()
+    values = []
+    for child in node.GetChildren():
+        values.append(child.GetName())
+    return IdlEnum(name=name, values=values)
+
+
+def exception_field_to_string_node_to_idl_operation(node):
+    extended_attributes = {}
+    name = node.GetName()
+    children = node.GetChildren()

[haraken, 2013/07/17 02:44:49]

Shall we check that len(children) > 0 ?

[Nils Barth (inactive), 2013/07/17 12:05:09]

Done. (As above.)

+    if len(children) > 2:
+        raise ValueError('ExceptionFieldToString node with %s children, expected at most 2' % len(children))
+
+    type_node = children[0]
+    return_type = type_node_to_type(type_node)
+
+    if len(children) > 1:
+        ext_attributes_node = children[1]
+        extended_attributes = ext_attributes_node_to_extended_attributes(ext_attributes_node)
+
+    return IdlOperation(name=name, data_type=return_type, extended_attributes=extended_attributes)
+
+
+def exception_node_to_idl_exception(node):
+    # Exceptions are similar to Interfaces, but simpler
+    attributes = []
+    constants = []
+    extended_attributes = None
+    functions = []
+    name = node.GetName()
+
+    children = node.GetChildren()
+    for child in children:
+        child_class = child.GetClass()
+        if child_class == 'Attribute':
+            attribute = attribute_node_to_idl_attribute(child)
+            attributes.append(attribute)
+        elif child_class == 'Const':
+            constants.append(constant_node_to_idl_constant(child))
+        elif child_class == 'ExtAttributes':
+            extended_attributes = ext_attributes_node_to_extended_attributes(child)
+        elif child_class == 'Operation':
+            functions.append(operation_node_to_idl_operation(child))
+        elif child_class == 'ExceptionFieldToString':
+            functions.append(exception_field_to_string_node_to_idl_operation(child))
+        else:
+            raise ValueError('Unrecognized node class: %s' % child_class)
+
+    return IdlException(name=name, attributes=attributes, constants=constants, extended_attributes=extended_attributes, functions=functions)
+
+
+def typedef_node_to_idl_typedef(node):
+    data_type = None
+    extended_attributes = None
+
+    children = node.GetChildren()
+    for child in children:
+        child_class = child.GetClass()
+        if child_class == 'Type':
+            data_type = type_node_to_type(child)
+        elif child_class == 'ExtAttributes':
+            extended_attributes = ext_attributes_node_to_extended_attributes(child)
+            raise ValueError('Extended attributes in a typedef are untested!')
+        else:
+            raise ValueError('Unrecognized node class: %s' % child_class)
+
+    return IdlTypedef(data_type=data_type, extended_attributes=extended_attributes)
+
+# Extended attributes
+
+
+def ext_attributes_node_to_extended_attributes(node):
+    """
+    Returns:
+      Dictionary of {ExtAttributeName: ExtAttributeValue}.
+      Value is usually a string, with three exceptions:
+      Constructors: value is a list of Arguments nodes, corresponding to
+      possibly signatures of the constructor.
+      CustomConstructors: value is a list of Arguments nodes, corresponding to
+      possibly signatures of the custom constructor.
+      NamedConstructor: value is a Call node, corresponding to the single
+      signature of the named constructor.
+    """
+    # Primarily just make a dictionary from the children.
+    # The only complexity is handling various types of constructors:
+    # Constructors and Custom Constructors can have duplicate entries due to
+    # overloading, and thus are stored in temporary lists.
+    # However, Named Constructors cannot be overloaded, and thus do not have

[haraken, 2013/07/17 02:44:49]

Actually this should be a FIXME. We want to support overloading for named
constructors and remove custom bindings for HTMLImageElement. Given that it
looks straightforward to support the overloading in your beautiful parser, I
hope you'll fix it in a follow-up CL :)

[Nils Barth (inactive), 2013/07/17 12:05:09]

Added FIXME!

+    # a list.
+    constructors = []
+    custom_constructors = []
+    extended_attributes = {}
+
+    attribute_list = node.GetChildren()
+    for attribute in attribute_list:
+        name = attribute.GetName()
+        children = attribute.GetChildren()
+        if name in ['Constructor', 'CustomConstructor', 'NamedConstructor']:
+            child = None
+            child_class = None
+            if children:
+                if len(children) > 1:
+                    raise ValueError('ExtAttributes node with %s children, expected at most 1' % len(children))
+                child = children[0]
+                child_class = child.GetClass()
+            if name == 'Constructor':
+                if child_class and child_class != 'Arguments':
+                    raise ValueError('Constructor only supports Arguments as child, but has child of class: %s' % child_class)
+                constructors.append(child)
+            elif name == 'CustomConstructor':
+                if child_class and child_class != 'Arguments':
+                    raise ValueError('Custom Constructor only supports Arguments as child, but has child of class: %s' % child_class)
+                custom_constructors.append(child)
+            else:  # name == 'NamedConstructor'
+                if child_class and child_class != 'Call':
+                    raise ValueError('Named Constructor only supports Call as child, but has child of class: %s' % child_class)
+                extended_attributes[name] = child
+        elif children:
+            raise ValueError('Non-constructor ExtAttributes node with children: %s' % name)
+        else:
+            value = attribute.GetProperty('VALUE')
+            extended_attributes[name] = value
+
+    # Store constructors and custom constructors in special list attributes,
+    # which are deleted later. Note plural in key.
+    if constructors:
+        extended_attributes['Constructors'] = constructors
+    if custom_constructors:
+        extended_attributes['CustomConstructors'] = custom_constructors
+
+    return extended_attributes
+
+
+def extended_attributes_to_constructors(extended_attributes):
+    """Returns constructors and custom_constructors (lists of IdlOperations),
+    deletes the special list attributes, and puts dummy empty value in
+    Constructor and CustomConstructor extended attributes.
+    Auxiliary function for interface_node_to_idl_interface."""
+    constructors = []
+    custom_constructors = []
+    if 'Constructors' in extended_attributes:
+        constructor_list = extended_attributes['Constructors']
+        # If not overloaded, have index 0, otherwise index from 1
+        overloaded_index = 0 if len(constructor_list) == 1 else 1
+        for arguments_node in constructor_list:
+            name = 'Constructor'
+            arguments = arguments_node_to_arguments(arguments_node)
+            constructor = IdlOperation(name=name, extended_attributes=extended_attributes, overloaded_index=overloaded_index, arguments=arguments)
+            constructors.append(constructor)
+            overloaded_index += 1
+        del extended_attributes['Constructors']
+        extended_attributes['Constructor'] = None

[haraken, 2013/07/17 02:44:49]

You can use clear_constructor_attributes().

[Nils Barth (inactive), 2013/07/17 12:05:09]

Done. (As above.)

+
+        # Prefix 'CallWith' and 'RaisesException' with 'Constructor'
+        # FIXME: I have no idea why this is necessary.

[haraken, 2013/07/17 02:44:49]

Yeah, I don't understand why either. We shouldn't use [CallWith] and
[RaisesException] in constuctors and should just use [ConstructorCallWith] and
[ConstructorRaisesException]. Then we no longer need to handle these attributes
specially.

[Nils Barth (inactive), 2013/07/17 12:05:09]

Got it - thanks for the explanation!
Added FIXME.

+        if 'CallWith' in extended_attributes:
+            extended_attributes['ConstructorCallWith'] = extended_attributes['CallWith']
+            del extended_attributes['CallWith']
+        if 'RaisesException' in extended_attributes:
+            extended_attributes['ConstructorRaisesException'] = extended_attributes['RaisesException']
+            del extended_attributes['RaisesException']
+
+    if 'CustomConstructors' in extended_attributes:
+        custom_constructor_list = extended_attributes['CustomConstructors']
+        # If not overloaded, have index 0, otherwise index from 1
+        overloaded_index = 0 if len(custom_constructor_list) == 1 else 1
+        for arguments_node in custom_constructor_list:
+            name = 'CustomConstructor'
+            arguments = arguments_node_to_arguments(arguments_node)
+            custom_constructor = IdlOperation(name=name, extended_attributes=extended_attributes, overloaded_index=overloaded_index, arguments=arguments)
+            custom_constructors.append(custom_constructor)
+            overloaded_index += 1
+        del extended_attributes['CustomConstructors']
+        extended_attributes['CustomConstructor'] = None

[haraken, 2013/07/17 02:44:49]

You can use clear_constructor_attributes().

[Nils Barth (inactive), 2013/07/17 12:05:09]

Done. (As above.)

+
+    if 'NamedConstructor' in extended_attributes:
+        name = 'NamedConstructor'
+        call_node = extended_attributes['NamedConstructor']
+        extended_attributes['NamedConstructor'] = call_node.GetName()
+        overloaded_index = None  # named constructors are not overloaded
+        arguments_node = call_node.GetChildren()[0]

[haraken, 2013/07/17 02:44:49]

Do you want to check that len(children) > 0 ?

[Nils Barth (inactive), 2013/07/17 12:05:09]

Done.

+        arguments = arguments_node_to_arguments(arguments_node)
+        named_constructor = IdlOperation(name=name, extended_attributes=extended_attributes, overloaded_index=overloaded_index, arguments=arguments)
+        constructors.append(named_constructor)
+
+    return constructors, custom_constructors
+
+
+def clear_constructor_attributes(extended_attributes):
+    if 'Constructors' in extended_attributes:
+        del extended_attributes['Constructors']

[haraken, 2013/07/17 02:44:49]

This del won't be needed, because you're setting None in the next line.

[Nils Barth (inactive), 2013/07/17 12:05:09]

It deletes the plural Constructor*s* and sets the singular Constructor.
This is easy to miss, so I’ve added a comment.

+        extended_attributes['Constructor'] = None

[haraken, 2013/07/17 05:43:11]

If you set None, I'm afraid that we cannot distinguish whether [Constructor] is
specified or not. To avoid the problem, the current Perl flow uses
"VALUE_IS_MISSING" for this case. How can the new Python flow distinguish it?

[Nils Barth (inactive), 2013/07/17 12:05:09]

The keys of extended_attributes are the names of the extended attributes
that are specified, while their values are the values. Thus if Constructor
is not specified, then extended_attributes does not have a Constructor key.
If Constructor is specified without any parameters, then
extended_attributes['Constructor'] == None
(exactly the same as Perl's 'VALUE_IS_MISSING').
If Constructor is specified with parameters, then
extended_attributes['Constructor'] == the parameters.
(Actually stored in Constructors, but that’s the idea.)

+    if 'CustomConstructors' in extended_attributes:
+        del extended_attributes['CustomConstructors']

[haraken, 2013/07/17 02:44:49]

Ditto.

[Nils Barth (inactive), 2013/07/17 12:05:09]

Ditto.

+        extended_attributes['CustomConstructor'] = None
+
+
+# Types
+
+
+def type_node_to_type(node):
+    children = node.GetChildren()
+    if not children:

[haraken, 2013/07/17 02:44:49]

Instead, you can check that len(children) > 0

[Nils Barth (inactive), 2013/07/17 12:05:09]

In general “if not some_list:” is considered more Pythonic than
“if len(some_list) > 0:”, (it's in the style guide), but since we’re
explicitly checking maximum length too, it’s clearer to have a single
length check; done.

+        raise ValueError('Type node expects children, got none.')
+    if len(children) > 2:
+        raise ValueError('Type node expects at most 2 children (type + optional array []), got %s (multi-dimensional arrays are not supported).' % len(children))
+
+    type_node_child = children[0]
+    data_type = type_node_inner_to_type(type_node_child)
+
+    if len(children) == 2:
+        array_node = children[1]
+        array_node_class = array_node.GetClass()
+        if array_node_class != 'Array':
+            raise ValueError('Expected Array node as TypeSuffix, got %s node.' % array_node_class)
+        data_type += '[]'
+
+    return data_type
+
+
+def type_node_inner_to_type(node):
+    """Auxiliary."""

[haraken, 2013/07/17 02:44:49]

I'd remove this comment.

[Nils Barth (inactive), 2013/07/17 12:05:09]

Good point; not the world’s most useful comment.

+    node_class = node.GetClass()
+    if node_class in ['PrimitiveType', 'Typeref']:
+        return node.GetName()
+    elif node_class == 'Any':
+        return 'any'
+    elif node_class == 'Sequence':
+        return sequence_node_to_type(node)
+    elif node_class == 'UnionType':
+        return union_type_node_to_idl_union_type(node)
+    raise ValueError('Unrecognized node class: %s' % node_class)
+
+
+def sequence_node_to_type(node):
+    children = node.GetChildren()
+    if len(children) != 1:
+        raise ValueError('Sequence node expects exactly 1 child, got %s' % len(children))
+    sequence_child = children[0]
+    sequence_child_class = sequence_child.GetClass()
+    if sequence_child_class != 'Type':
+        raise ValueError('Unrecognized node class: %s' % sequence_child_class)
+    sequence_type = type_node_to_type(sequence_child)
+    return 'sequence<%s>' % sequence_type
+
+
+def union_type_node_to_idl_union_type(node):
+    union_member_types = []
+    for member_type_node in node.GetChildren():
+        member_type = type_node_inner_to_type(member_type_node)
+        union_member_types.append(member_type)
+    return IdlUnionType(union_member_types=union_member_types)