IDL: overload resolution for string and enumeration arguments

Source/bindings/scripts/v8_interface.py

 # Copyright (C) 2013 Google Inc. All rights reserved.
 # coding=utf-8
 #
 # 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
@@ skipping 362 matching lines @@
         'minimum_number_of_required_arguments': minimum_number_of_required_arguments,
         'name': name,
     }
 
 
 def is_use_spec_algorithm(effective_overloads_by_length):
     # FIXME: temporary function so can switch incrementally
     # Use spec algorithm if:
     # * method is not variadic,
     # * no distinguishing type is unsupported:
-    #   non-wrapper, array, callback interface, enumeration, boolean, string, nullable,
+    #   non-wrapper, array, callback interface, boolean, nullable,
     # * all distinguishing types are distinct, and
     # * all distinguishing arguments are required (not optional).
     def is_unsupported_type(idl_type):
         return ((idl_type.is_interface_type and not idl_type.is_wrapper_type) or
                 idl_type.array_or_sequence_type or
                 idl_type.is_callback_interface or
-                idl_type.is_enum or
-                idl_type.name in ('Boolean', 'String') or
+                idl_type.name == 'Boolean' or
                 idl_type.is_nullable)
 
     for _, effective_overloads in effective_overloads_by_length:
         methods = [effective_overload[0]
                    for effective_overload in effective_overloads]
         if any(method['is_variadic'] for method in methods):
             return False
 
         if len(effective_overloads) == 1:
             # No distinguishing type, since resolved by length
@@ skipping 227 matching lines @@
     #     Note: This is the argument that will be used to resolve which
     #           overload is selected.
     cpp_value = 'info[%s]' % index
 
     # Extract argument and IDL type to simplify accessing these in each loop.
     # FIXME: mostly just need type, but need argument itself for optionality
     arguments = [method['arguments'][index] for method in methods]
     idl_types = [argument['idl_type_object'] for argument in arguments]
     idl_types_methods = zip(idl_types, methods)
 
-    # We can't do a single loop through all methods or simply sort them, because
+    # We can’t do a single loop through all methods or simply sort them, because
     # a method may be listed in multiple steps of the resolution algorithm, and
     # which test to apply differs depending on the step.
-    # Instead, we need to loop through all methods at each step, filtering to
-    # matches and generating an appropriate test.
-    for idl_type, method in idl_types_methods:
-        # 4. Otherwise: if V is a platform object – but not a platform array
-        # object – and there is an entry in S that has one of the following
-        # types at position i of its type list,
-        # • an interface type that V implements
-        # (We distinguish wrapper types from built-in interface types.)
-        if idl_type.is_wrapper_type:
-            test = 'V8{idl_type}::hasInstance({cpp_value}, info.GetIsolate())'.format(idl_type=idl_type.base_type, cpp_value=cpp_value)
+    #
+    # Instead, we need to go through all methods at each step, either finding
+    # first match (if only one test is allowed) or filtering to matches (if
+    # multiple tests are allowed), and generating an appropriate tests.
+
+    # 4. Otherwise: if V is a platform object – but not a platform array
+    # object – and there is an entry in S that has one of the following
+    # types at position i of its type list,
+    # • an interface type that V implements
+    # (We distinguish wrapper types from built-in interface types.)
+    for idl_type, method in ((idl_type, method)
+                             for idl_type, method in idl_types_methods
+                             if idl_type.is_wrapper_type):
+        test = 'V8{idl_type}::hasInstance({cpp_value}, info.GetIsolate())'.format(idl_type=idl_type.base_type, cpp_value=cpp_value)
+        yield test, method

[haraken, 2014/05/14 06:09:03]

Why do you not need a try-catch block here?

[Nils Barth (inactive), 2014/05/14 06:28:15]

Because this can return multiple matches.
Added comment to clarify.

+
+    # (Check for exact type matches before performing automatic type conversion;
+    # only needed if distinguishing between primitive types.)
+
+    # 10. Otherwise: if V is a Number value, and there is an entry in S that has
+    # one of the following types at position i of its type list,
+    # • a numeric type
+    if any(idl_type.name == 'String' or idl_type.is_enum

[haraken, 2014/05/14 06:09:03]

I wonder why you need to check string or enum here.

[Nils Barth (inactive), 2014/05/14 06:28:15]

It's only needed if there's a match in step 11, otherwise it's redundant.
Added comment to address this.

+           for idl_type in idl_types):
+        try:
+            method = next(method for idl_type, method in idl_types_methods
+                          if idl_type.is_numeric_type)
+            test = '%s->IsNumber()' % cpp_value
             yield test, method
+        except StopIteration:
+            pass
 
-    # FIXME: not needed yet
-    # for idl_type, method in idl_types_methods:
-    #     # 10. Otherwise: if V is a Number value, and there is an entry in S
-    #     # that has one of the following types at position i of its type
-    #     # list,
-    #     # • a numeric type
-    #     if idl_type.is_numeric_type:
-    #         test = '%s->IsNumber()' % cpp_value
-    #         yield test, method
+    # (Perform automatic type conversion, in order. If any of these match,
+    # that’s the end, and no other tests are needed.)
 
-    for idl_type, method in idl_types_methods:
-        # 12. Otherwise: if there is an entry in S that has one of the
-        # following types at position i of its type list,
-        # • a numeric type
-        if idl_type.is_numeric_type:
-            yield 'true', method
+    # 11. Otherwise: if there is an entry in S that has one of the following
+    # types at position i of its type list,
+    # • DOMString
+    # • an enumeration type
+    try:

[Jens Widell, 2014/05/14 05:19:31]

You could skip the 'try' and "move" the controlled block into the preceding
'if', which checks for the same condition. That would avoid checking for a
string-or-enum argument twice using two different types of checks.

[Nils Barth (inactive), 2014/05/14 06:28:15]

While that seems simpler, I'm following the order in the spec;
the added |if| check above is to remove redundancy in the CG.
It gets trickier when we also have booleans.

+        method = next(method for idl_type, method in idl_types_methods
+                      if idl_type.name == 'String' or idl_type.is_enum)
+        yield 'true', method
+        return
+    except StopIteration:
+        pass
+
+    # 12. Otherwise: if there is an entry in S that has one of the following
+    # types at position i of its type list,
+    # • a numeric type

[haraken, 2014/05/14 06:09:03]

I'm curious why the step 11 and 12 are separated. Aren't they doing the same
thing? (just generating "true"). Probably you're planning to make it different
in a follow-up.

[Nils Barth (inactive), 2014/05/14 06:28:15]

Because they're different in the spec, and splitting makes a difference:
it means we *first* resolve to DOMString, *then* to numeric, *regardless*
of the order in the IDL file.
(This is to handle automatic type conversion.)

This is explained in the comment above:
# (Perform automatic type conversion, in order. If any of these match,
# that’s the end, and no other tests are needed.)

+    try:
+        method = next(method for idl_type, method in idl_types_methods
+                      if idl_type.is_numeric_type)
+        yield 'true', method
+        return
+    except StopIteration:
+        pass
 
 
 def overload_resolution_expression(method):
     # Expression is an OR of ANDs: each term in the OR corresponds to a
     # possible argument count for a given method, with type checks.
     # FIXME: Blink's overload resolution algorithm is incorrect, per:
     # Implement WebIDL overload resolution algorithm.  http://crbug.com/293561
     #
     # Currently if distinguishing non-primitive type from primitive type,
     # (e.g., sequence<DOMString> from DOMString or Dictionary from double)
@@ skipping 379 matching lines @@
         deleter = next(
             method
             for method in interface.operations
             if ('deleter' in method.specials and
                 len(method.arguments) == 1 and
                 str(method.arguments[0].idl_type) == 'DOMString'))
     except StopIteration:
         return None
 
     return property_deleter(deleter)