Overload resolution improvements (#377)

docs/reference/overloaded-methods.md

@@ -6,24 +6,65 @@ function by dynamically checking for different argument counts and/or types.
 
 ## Overload resolution
 
-The JS [marshaller](../features/js-dotnet-marshalling) has limited support for overload
-resolution. It can examine the count and types of arguments provided by the JavaScript caller and
-select the best matching .NET overload accordingly.
+The JS [marshaller](../features/js-dotnet-marshalling) has support for overload resolution. It can
+examine the count and types of arguments provided by the JavaScript caller and select the best
+matching .NET overload accordingly.
 
 ```C#
 [JSExport]
 public class OverloadsExample
 {
     public static void AddValue(string stringValue);
-    public static void AddValue(double numberValue);
+    public static void AddValue(int intValue);
+    public static void AddValue(double doubleValue);
 }
 ```
 ```JS
 OverloadsExample.addValue('test'); // Calls AddValue(string)
-OverloadsExample.addValue(77); // Calls AddValue(double)
+OverloadsExample.addValue(77); // Calls AddValue(int)
+OverloadsExample.addValue(0.5); // Calls AddValue(double)
 ```
 
-Currently the overload resolution is limited to examining the JavaScript type of each argument
-(`string`, `number`, `object`, etc), but that is not sufficient to select between overloads that
-differ only in the _type of object_.
-[More advanced overload resolution is planned.](https://github.com/microsoft/node-api-dotnet/issues/134)
+Overload resolution considers the following information when selecting the best match among method
+overloads:
+ - **Argument count** - Resolution eliminates any overloads that do not accept the number of
+   arguments that were supplied, taking into account when some .NET method parameters are
+   optional or have default values.
+ - **Argument JS value types** - Resolution initially does a quick filter by matching only on
+   the [JavaScript value type](./dotnet/Microsoft.JavaScript.NodeApi/JSValueType) of each argument,
+   e.g. JS `string` matches .NET `string`, JS `number` matches any .NET numeric type,
+   JS `object` matches any .NET class, interface, or struct type.
+ - **Nullability** - JS `null` or `undefined` arguments match with any method parameters that are
+   .NET reference types or `Nullable<T>` value types. (Non-nullable reference type annotations are
+   not considered.) 
+ - **Number argument properties** - If there are multiple overloads with different .NET numeric
+   types (e.g. `int` and `double`), the properties of the JS number value are used to select the
+   best overload, including whether it is negative, an integer, or outside the bounds of the .NET
+   numeric type.
+ - **Proxied .NET object types** - When a JS argument value is actually
+   [a proxy to a .NET object](./classes-interfaces.md#marshalling-net-classes-to-js),
+   then the .NET type is matched to the method parameter type.
+ - **JS collection types** - When an argument value is a JS collection, the JS collection type
+   such as `Array` or `Map` is matched to a corresponding .NET collection type. (Generic collection
+   _element_ types are not considered, since JS collections do not have specific element types.)
+ - **Other special types** - Types with special marshalling behavior including [dates](./dates),
+   [guids](./other-types), [Tasks/Promises](./async-promises), and [delegates](./delegates) are
+   matched accordingly during overload resolution.
+
+If overload resolution finds multiple matches, or does not find any valid matches, then a
+`TypeError` is thrown.
+
+### Performance considerations
+
+Unlike compiled languages where the compiler can bind to the appropriate overload at compile time,
+with JavaScript the overload resolution process must be repeated at every invocation of the method.
+It is not super expensive, but consider avoiding calls to overloaded methods in performance-critical
+code.
+
+### Limitations
+
+When calling .NET methods from JavaScript, the dynamic overload resolution is not 100% consistent
+with C#'s compile-time overload resolution. There are some unavoidable limitations due to the
+dynamic-typed nature of JavaScript, and likely some deficienceies in the implementation. While it
+should work sufficiently well for the majority of cases, if you find a situation where overload
+resolution is not working as expected, please [report a bug](../support).

src/NodeApi.DotNetHost/JSMarshaller.cs

@@ -1262,7 +1262,8 @@ public Expression<Func<JSCallbackDescriptor>> BuildConstructorOverloadDescriptor
             // TODO: Default parameter values
 
             Type[] parameterTypes = constructors[i].GetParameters()
-                .Select(p => p.ParameterType).ToArray();
+                .Select(p => EnsureObjectCollectionTypeForOverloadResolution(p.ParameterType))
+                .ToArray();
             statements[i + 1] = Expression.Assign(
                 Expression.ArrayAccess(overloadsVariable, Expression.Constant(i)),
                 Expression.New(
@@ -1353,7 +1354,8 @@ public Expression<Func<JSCallbackDescriptor>> BuildMethodOverloadDescriptorExpre
             // TODO: Default parameter values
 
             Type[] parameterTypes = methods[i].GetParameters()
-                .Select(p => p.ParameterType).ToArray();
+                .Select(p => EnsureObjectCollectionTypeForOverloadResolution(p.ParameterType))
+                .ToArray();
             statements[i + 1] = Expression.Assign(
                 Expression.ArrayAccess(overloadsVariable, Expression.Constant(i)),
                 Expression.New(
@@ -1381,6 +1383,77 @@ public Expression<Func<JSCallbackDescriptor>> BuildMethodOverloadDescriptorExpre
             Array.Empty<ParameterExpression>());
     }
 
+    /// <summary>
+    /// For purposes of overload resolution, convert non-generic collections and collections
+    /// with specific generic element types to collection interfaces with object element types.
+    /// This simplifies the checks required during overload resolution and avoids the need
+    /// for reflection at resolution time, which is not supported in AOT. (The JS collections
+    /// will still be marshalled to the more specific collection type after resolution when 
+    /// an overload is invoked.)
+    /// </summary>
+    private static Type EnsureObjectCollectionTypeForOverloadResolution(Type type)
+    {
+        if (typeof(System.Collections.IEnumerable).IsAssignableFrom(type) &&
+            !type.IsArray && type != typeof(string))
+        {
+            if (TypeImplementsGenericInterface(type, typeof(IDictionary<,>)) ||
+                TypeImplementsGenericInterface(type, typeof(IReadOnlyDictionary<,>)) ||
+                typeof(System.Collections.IDictionary).IsAssignableFrom(type))
+            {
+                type = typeof(IDictionary<object, object>);
+            }
+            else if (TypeImplementsGenericInterface(type, typeof(IList<>)) ||
+                TypeImplementsGenericInterface(type, typeof(IReadOnlyList<>)) ||
+                typeof(System.Collections.IList).IsAssignableFrom(type))
+            {
+                type = typeof(IList<object>);
+            }
+            else if (TypeImplementsGenericInterface(type, typeof(ISet<>))
+#if READONLY_SET
+                || TypeImplementsGenericInterface(type, typeof(IReadOnlySet<>))
+#endif
+            )
+            {
+                type = typeof(ISet<object>);
+            }
+            else if (TypeImplementsGenericInterface(type, typeof(ICollection<>)) ||
+                TypeImplementsGenericInterface(type, typeof(IReadOnlyCollection<>)))
+            {
+                type = typeof(ICollection<object>);
+            }
+            else
+            {
+                type = typeof(IEnumerable<object>);
+            }
+        }
+        else if (TypeImplementsGenericInterface(type, typeof(IAsyncEnumerable<>)))
+        {
+            type = typeof(IAsyncEnumerable<object>);
+        }
+
+        return type;
+    }
+
+    private static bool TypeImplementsGenericInterface(Type type, Type genericInterface)
+    {
+        if (type.IsInterface && type.IsGenericType &&
+            type.GetGenericTypeDefinition() == genericInterface)
+        {
+            return true;
+        }
+
+        foreach (Type interfaceType in type.GetInterfaces())
+        {
+            if (interfaceType.IsGenericType &&
+                interfaceType.GetGenericTypeDefinition() == genericInterface)
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     /// <summary>
     /// Gets overload information for a set of overloaded methods.
     /// </summary>
@@ -2968,71 +3041,114 @@ private IEnumerable<Expression> BuildFromJSToCollectionInterfaceExpressions(
         Type elementType = toType.GenericTypeArguments[0];
         Type typeDefinition = toType.GetGenericTypeDefinition();
 
-        if (typeDefinition == typeof(IList<>) ||
-            typeDefinition == typeof(ICollection<>) ||
+        if (typeDefinition == typeof(IEnumerable<>) ||
+            typeDefinition == typeof(IAsyncEnumerable<>) ||
+            typeDefinition == typeof(ISet<>) ||
 #if READONLY_SET
             typeDefinition == typeof(IReadOnlySet<>) ||
 #else
             // TODO: Support IReadOnlySet on .NET Framework / .NET Standard 2.0.
 #endif
-            typeDefinition == typeof(ISet<>))
+            typeDefinition == typeof(IList<>) ||
+            typeDefinition == typeof(IReadOnlyList<>))
         {
             /*
-             * value.TryUnwrap() as ICollection<T> ??
-             *     ((JSArray)value).AsCollection<T>(
-             *         (value) => (T)value,
-             *         (value) => (JSValue)value);
+             * value.TryUnwrap() as IList<T> ??
+             *     ((JSArray)value).AsList<T>((value) => (T)value, (value) => (JSValue)value);
              */
-            Type jsCollectionType = typeDefinition.Name.Contains("Set") ?
-                typeof(JSSet) : typeof(JSArray);
+            Type jsCollectionType =
+                typeDefinition == typeof(IEnumerable<>) ? typeof(JSIterable) :
+                typeDefinition == typeof(IAsyncEnumerable<>) ? typeof(JSAsyncIterable) :
+                typeDefinition.Name.Contains("Set") ? typeof(JSSet) :
+                typeof(JSArray);
+            bool isBidirectional = (typeDefinition == typeof(IList<>) ||
+#if READONLY_SET
+                typeDefinition == typeof(IReadOnlySet<>) ||
+#endif
+                typeDefinition == typeof(ISet<>));
             MethodInfo asCollectionMethod = typeof(JSCollectionExtensions).GetStaticMethod(
 #if !STRING_AS_SPAN
                 "As" + typeDefinition.Name.Substring(1, typeDefinition.Name.IndexOf('`') - 1),
 #else
                 string.Concat("As",
                     typeDefinition.Name.AsSpan(1, typeDefinition.Name.IndexOf('`') - 1)),
 #endif
-                new[] { jsCollectionType, typeof(JSValue.To<>), typeof(JSValue.From<>) },
+                isBidirectional ?
+                    new[] { jsCollectionType, typeof(JSValue.To<>), typeof(JSValue.From<>) } :
+                    new[] { jsCollectionType, typeof(JSValue.To<>) },
                 elementType);
             MethodInfo asJSCollectionMethod = jsCollectionType.GetExplicitConversion(
                 typeof(JSValue), jsCollectionType);
+            Expression valueAsCollectionExpression =
+                Expression.Convert(valueExpression, jsCollectionType, asJSCollectionMethod);
+            Expression fromJSExpression = GetFromJSValueExpression(elementType);
+            Expression toJSExpression = GetToJSValueExpression(elementType);
             yield return Expression.Coalesce(
                 Expression.TypeAs(Expression.Call(valueExpression, s_tryUnwrap), toType),
-                Expression.Call(
-                    asCollectionMethod,
-                    Expression.Convert(valueExpression, jsCollectionType, asJSCollectionMethod),
-                    GetFromJSValueExpression(elementType),
-                    GetToJSValueExpression(elementType)));
+                isBidirectional ?
+                    Expression.Call(
+                        asCollectionMethod,
+                        valueAsCollectionExpression,
+                        fromJSExpression,
+                        toJSExpression) :
+                    Expression.Call(
+                        asCollectionMethod,
+                        valueAsCollectionExpression,
+                        fromJSExpression));
         }
-        else if (typeDefinition == typeof(IReadOnlyList<>) ||
-            typeDefinition == typeof(IReadOnlyCollection<>) ||
-            typeDefinition == typeof(IEnumerable<>) ||
-            typeDefinition == typeof(IAsyncEnumerable<>))
+        else if (typeDefinition == typeof(ICollection<>) ||
+            typeDefinition == typeof(IReadOnlyCollection<>))
         {
             /*
-             * value.TryUnwrap() as IReadOnlyCollection<T> ??
-             *     ((JSArray)value).AsReadOnlyCollection<T>((value) => (T)value);
+             * value.TryUnwrap() as ICollection<T> ?? value.IsArray() ?
+             *     ((JSArray)value).AsCollection<T>((value) => (T)value) :
+             *     ((JSSet)value).AsCollection<T>((value) => (T)value, (value) => (JSValue)value);
              */
-            Type jsCollectionType = typeDefinition == typeof(IEnumerable<>) ?
-                typeof(JSIterable) : typeDefinition == typeof(IAsyncEnumerable<>) ?
-                typeof(JSAsyncIterable) : typeof(JSArray);
-            MethodInfo asCollectionMethod = typeof(JSCollectionExtensions).GetStaticMethod(
-#if !STRING_AS_SPAN
-                "As" + typeDefinition.Name.Substring(1, typeDefinition.Name.IndexOf('`') - 1),
-#else
-                string.Concat("As",
-                    typeDefinition.Name.AsSpan(1, typeDefinition.Name.IndexOf('`') - 1)),
-#endif
-                new[] { jsCollectionType, typeof(JSValue.To<>) },
+            MethodInfo isArrayMethod = typeof(JSValue).GetInstanceMethod(nameof(JSValue.IsArray));
+            bool isReadOnlyCollection = typeDefinition == typeof(IReadOnlyCollection<>);
+            string asCollectionMethodName = isReadOnlyCollection ?
+                    nameof(JSCollectionExtensions.AsReadOnlyCollection) :
+                    nameof(JSCollectionExtensions.AsCollection);
+            MethodInfo arrayAsCollectionMethod = typeof(JSCollectionExtensions).GetStaticMethod(
+                asCollectionMethodName,
+                isReadOnlyCollection ?
+                    new[] { typeof(JSArray), typeof(JSValue.To<>) } :
+                    new[] { typeof(JSArray), typeof(JSValue.To<>), typeof(JSValue.From<>) },
                 elementType);
-            MethodInfo asJSCollectionMethod = jsCollectionType.GetExplicitConversion(
-                typeof(JSValue), jsCollectionType);
+            MethodInfo setAsCollectionMethod = typeof(JSCollectionExtensions).GetStaticMethod(
+                asCollectionMethodName,
+                isReadOnlyCollection ?
+                    new[] { typeof(JSSet), typeof(JSValue.To<>) } :
+                    new[] { typeof(JSSet), typeof(JSValue.To<>), typeof(JSValue.From<>) },
+                elementType);
+            MethodInfo asJSArrayMethod = typeof(JSArray).GetExplicitConversion(
+                typeof(JSValue), typeof(JSArray));
+            MethodInfo asJSSetMethod = typeof(JSSet).GetExplicitConversion(
+                typeof(JSValue), typeof(JSSet));
             yield return Expression.Coalesce(
                 Expression.TypeAs(Expression.Call(valueExpression, s_tryUnwrap), toType),
-                Expression.Call(
-                    asCollectionMethod,
-                    Expression.Convert(valueExpression, jsCollectionType, asJSCollectionMethod),
-                    GetFromJSValueExpression(elementType)));
+                Expression.Condition(
+                    Expression.Call(valueExpression, isArrayMethod),
+                    isReadOnlyCollection ?
+                        Expression.Call(
+                            arrayAsCollectionMethod,
+                            Expression.Convert(valueExpression, typeof(JSArray), asJSArrayMethod),
+                            GetFromJSValueExpression(elementType)) :
+                        Expression.Call(
+                            arrayAsCollectionMethod,
+                            Expression.Convert(valueExpression, typeof(JSArray), asJSArrayMethod),
+                            GetFromJSValueExpression(elementType),
+                            GetToJSValueExpression(elementType)),
+                    isReadOnlyCollection ?
+                        Expression.Call(
+                            setAsCollectionMethod,
+                            Expression.Convert(valueExpression, typeof(JSSet), asJSSetMethod),
+                            GetFromJSValueExpression(elementType)) :
+                        Expression.Call(
+                            setAsCollectionMethod,
+                            Expression.Convert(valueExpression, typeof(JSSet), asJSSetMethod),
+                            GetFromJSValueExpression(elementType),
+                            GetToJSValueExpression(elementType))));
         }
         else if (typeDefinition == typeof(IDictionary<,>))
         {

src/NodeApi.Generator/ExpressionExtensions.cs

@@ -102,9 +102,9 @@ private static string ToCS(
                       ") { " + ToCS(conditional.IfTrue, path, variables) + "; }" +
                       (conditional.IfFalse is DefaultExpression ? string.Empty :
                       " else { " + ToCS(conditional.IfFalse, path, variables) + "; }")
-                    : ToCS(conditional.Test, path, variables) + " ?\n" +
+                    : '(' + ToCS(conditional.Test, path, variables) + " ?\n" +
                       ToCS(conditional.IfTrue, path, variables) + " :\n" +
-                      ToCS(conditional.IfFalse, path, variables),
+                      ToCS(conditional.IfFalse, path, variables) + ')',
 
             MemberExpression { NodeType: ExpressionType.MemberAccess } member =>
                 member.Expression is ParameterExpression parameterExpression &&
@@ -375,6 +375,10 @@ internal static string FormatType(Type type)
         {
             return "string";
         }
+        else if (type == typeof(object))
+        {
+            return "object";
+        }
         else if (type == typeof(void))
         {
             return "void";