JSExport method overload support (#221)

jasongin · web-flow · commit 80cb8095d3e4 · 2024-03-10T16:56:27.000-10:00
diff --git a/src/NodeApi.DotNetHost/JSMarshaller.cs b/src/NodeApi.DotNetHost/JSMarshaller.cs
@@ -43,6 +43,12 @@ public class JSMarshaller
     /// </summary>
     public const string ResultPropertyName = "result";
 
+    /// <summary>
+    /// Keeps track of the names of all generated lambda expressions in order to automatically
+    /// avoid collisions, which can occur with overloaded methods.
+    /// </summary>
+    private readonly HashSet<string> _expressionNames = new();
+
     [ThreadStatic]
     private static JSMarshaller? s_current;
 
@@ -805,7 +811,7 @@ Expression ParameterToJSValue(int index) => InlineOrInvoke(
             return Expression.Lambda(
                 _delegates.Value.GetToJSDelegateType(method.ReturnType, parameters),
                 Expression.Block(method.ReturnType, new[] { resultVariable }, statements),
-                $"to_{FullMethodName(method)}",
+                FullMethodName(method, "to_"),
                 parameters);
         }
         catch (Exception ex)
@@ -874,7 +880,7 @@ public LambdaExpression BuildFromJSFunctionExpression(MethodInfo method)
             return Expression.Lambda(
                 JSMarshallerDelegates.GetFromJSDelegateType(method.DeclaringType!),
                 body: Expression.Block(typeof(JSValue), variables, statements),
-                $"from_{FullMethodName(method)}",
+                FullMethodName(method, "from_"),
                 parameters: new[] { thisParameter, s_argsParameter });
         }
         catch (Exception ex)
@@ -1265,6 +1271,7 @@ public Expression<Func<JSCallbackDescriptor>> BuildMethodOverloadDescriptorExpre
          * return JSCallbackOverload.CreateDescriptor(methodName, overloads);
          */
 
+        string name = FullMethodName(methods[0]);
         ParameterExpression overloadsVariable =
             Expression.Variable(typeof(JSCallbackOverload[]), "overloads");
         var statements = new Expression[methods.Length + 2];
@@ -1304,7 +1311,7 @@ public Expression<Func<JSCallbackDescriptor>> BuildMethodOverloadDescriptorExpre
                 typeof(JSCallbackDescriptor),
                 new[] { overloadsVariable },
                 statements),
-            name: FullMethodName(methods[0]),
+            name,
             Array.Empty<ParameterExpression>());
     }
 
@@ -3015,17 +3022,30 @@ private static bool IsTypedArrayType(Type elementType)
             || elementType == typeof(double);
     }
 
-    private static string FullMethodName(MethodInfo method)
+    private string FullMethodName(MethodInfo method, string? prefix = null)
     {
-        string prefix = string.Empty;
         string name = method.Name;
         if (name.StartsWith("get_") || name.StartsWith("set_"))
         {
-            prefix = name.Substring(0, 4);
+            prefix ??= name.Substring(0, 4);
             name = name.Substring(4);
         }
+        else
+        {
+            prefix ??= string.Empty;
+        }
+
+        // Ensure the generated name is unique by appending a counter suffix if necessary.
+        string fullName = $"{prefix}{FullTypeName(method.DeclaringType!)}_{name}";
+        string suffix = string.Empty;
+        for (int i = 2; _expressionNames.Contains(fullName + suffix); i++)
+        {
+            suffix = $"_{i}";
+        }
 
-        return $"{prefix}{FullTypeName(method.DeclaringType!)}_{name}";
+        fullName += suffix;
+        _expressionNames.Add(fullName);
+        return fullName;
     }
 
     internal static string FullTypeName(Type type)
diff --git a/src/NodeApi.Generator/ModuleGenerator.cs b/src/NodeApi.Generator/ModuleGenerator.cs
@@ -368,18 +368,20 @@ private void ExportModule(
             s += $"exportsValue = new JSModuleBuilder<{ns}.{moduleType.Name}>()";
             s.IncreaseIndent();
 
-            // Export non-static members of the module class.
-            foreach (ISymbol? member in moduleType.GetMembers()
-              .Where((m) => m.DeclaredAccessibility == Accessibility.Public && !m.IsStatic))
+            // Export public non-static members of the module class.
+            IEnumerable<ISymbol> members = moduleType.GetMembers()
+                .Where((m) => m.DeclaredAccessibility == Accessibility.Public && !m.IsStatic);
+
+            foreach (IPropertySymbol property in members.OfType<IPropertySymbol>())
             {
-                if (member is IMethodSymbol method && method.MethodKind == MethodKind.Ordinary)
-                {
-                    ExportMethod(ref s, method);
-                }
-                else if (member is IPropertySymbol property)
-                {
-                    ExportProperty(ref s, property);
-                }
+                ExportProperty(ref s, property, GetExportName(property));
+            }
+
+            foreach (IGrouping<string, IMethodSymbol> methodGroup in members.OfType<IMethodSymbol>()
+                .Where((m) => m.MethodKind == MethodKind.Ordinary)
+                .GroupBy(GetExportName))
+            {
+                ExportMethod(ref s, methodGroup, methodGroup.Key);
             }
         }
         else
@@ -401,18 +403,20 @@ private void ExportModule(
                 // Export tagged static properties as properties on the module.
                 ExportProperty(ref s, exportProperty, exportName);
             }
-            else if (exportItem is IMethodSymbol exportMethod)
-            {
-                // Export tagged static methods as top-level functions on the module.
-                ExportMethod(ref s, exportMethod, exportName);
-            }
             else if (exportItem is ITypeSymbol exportDelegate &&
                 exportDelegate.TypeKind == TypeKind.Delegate)
             {
                 ExportDelegate(exportDelegate);
             }
         }
 
+        // Export tagged static methods as top-level functions on the module.
+        foreach (IGrouping<string, IMethodSymbol> methodGroup in exportItems.OfType<IMethodSymbol>()
+            .GroupBy(GetExportName))
+        {
+            ExportMethod(ref s, methodGroup, methodGroup.Key);
+        }
+
         if (moduleType != null)
         {
             // Construct an instance of the custom module class when the module is initialized.
@@ -434,10 +438,8 @@ private void ExportModule(
     private void ExportType(
         ref SourceBuilder s,
         ITypeSymbol type,
-        string? exportName = null)
+        string exportName)
     {
-        exportName ??= type.Name;
-
         string propertyAttributes = string.Empty;
         if (type.ContainingType != null)
         {
@@ -547,22 +549,15 @@ private void ExportMembers(
     {
         bool isStreamClass = typeof(System.IO.Stream).IsAssignableFrom(type.AsType());
 
-        foreach (ISymbol member in type.GetMembers()
-          .Where((m) => m.DeclaredAccessibility == Accessibility.Public))
-        {
-            if (isStreamClass && !member.IsStatic)
-            {
-                // Only static members on stream subclasses are exported to JS.
-                continue;
-            }
+        IEnumerable<ISymbol> members = type.GetMembers()
+            .Where((m) => m.DeclaredAccessibility == Accessibility.Public)
+            .Where((m) => !isStreamClass || m.IsStatic);
 
-            if (member is IMethodSymbol method && method.MethodKind == MethodKind.Ordinary)
-            {
-                ExportMethod(ref s, method);
-            }
-            else if (member is IPropertySymbol property)
+        foreach (ISymbol member in members)
+        {
+            if (member is IPropertySymbol property)
             {
-                ExportProperty(ref s, property);
+                ExportProperty(ref s, property, GetExportName(member));
             }
             else if (type.TypeKind == TypeKind.Enum && member is IFieldSymbol field)
             {
@@ -571,42 +566,61 @@ private void ExportMembers(
             }
             else if (member is INamedTypeSymbol nestedType)
             {
-                ExportType(ref s, nestedType);
+                ExportType(ref s, nestedType, GetExportName(member));
             }
         }
+
+        foreach (IGrouping<string, IMethodSymbol> methodGroup in members
+            .OfType<IMethodSymbol>().Where((m) => m.MethodKind == MethodKind.Ordinary)
+            .GroupBy(GetExportName))
+        {
+            ExportMethod(ref s, methodGroup, methodGroup.Key);
+        }
     }
 
     /// <summary>
     /// Generate code for a method exported on a class, struct, or module.
     /// </summary>
     private void ExportMethod(
       ref SourceBuilder s,
-      IMethodSymbol method,
-      string? exportName = null)
+      IEnumerable<IMethodSymbol> methods,
+      string exportName)
     {
-        exportName ??= ToCamelCase(method.Name);
+        // TODO: Support exporting generic methods.
+        methods = methods.Where((m) => !m.IsGenericMethod);
+
+        IMethodSymbol? method = methods.FirstOrDefault();
+        if (method == null)
+        {
+            return;
+        }
 
-        // An adapter method may be used to support marshalling arbitrary parameters,
-        // if the method does not match the `JSCallback` signature.
         string attributes = "JSPropertyAttributes.DefaultMethod" +
             (method.IsStatic ? " | JSPropertyAttributes.Static" : string.Empty);
-        if (method.IsGenericMethod)
+
+        if (methods.Count() == 1 && !IsMethodCallbackAdapterRequired(method))
         {
-            // TODO: Export generic method.
+            // No adapter is needed for a method with a JSCallback signature.
+            string ns = GetNamespace(method);
+            string className = method.ContainingType.Name;
+            s += $".AddMethod(\"{exportName}\", " +
+                $"{ns}.{className}.{method.Name},\n\t{attributes})";
         }
-        else if (IsMethodCallbackAdapterRequired(method))
+        else if (methods.Count() == 1)
         {
+            // An adapter method supports marshalling arbitrary parameters.
             Expression<JSCallback> adapter =
                 _marshaller.BuildFromJSMethodExpression(method.AsMethodInfo());
             _callbackAdapters.Add(adapter.Name!, adapter);
             s += $".AddMethod(\"{exportName}\", {adapter.Name},\n\t{attributes})";
         }
         else
         {
-            string ns = GetNamespace(method);
-            string className = method.ContainingType.Name;
-            s += $".AddMethod(\"{exportName}\", " +
-                $"{ns}.{className}.{method.Name},\n\t{attributes})";
+            // An adapter method provides overload resolution.
+            LambdaExpression adapter = _marshaller.BuildMethodOverloadDescriptorExpression(
+                methods.Select((m) => m.AsMethodInfo()).ToArray());
+            _callbackAdapters.Add(adapter.Name!, adapter);
+            s += $".AddMethod(\"{exportName}\", {adapter.Name}(),\n\t{attributes})";
         }
     }
 
@@ -616,10 +630,8 @@ private void ExportMethod(
     private void ExportProperty(
       ref SourceBuilder s,
       IPropertySymbol property,
-      string? exportName = null)
+      string exportName)
     {
-        exportName ??= ToCamelCase(property.Name);
-
         bool writable = property.SetMethod != null ||
             (!property.IsStatic && property.ContainingType.TypeKind == TypeKind.Struct);
         string attributes = "JSPropertyAttributes.Enumerable | JSPropertyAttributes.Configurable" +
diff --git a/src/NodeApi.Generator/SymbolExtensions.cs b/src/NodeApi.Generator/SymbolExtensions.cs
@@ -414,7 +414,8 @@ private static ConstructorBuilder BuildSymbolicConstructor(
         IReadOnlyList<IParameterSymbol> parameters = constructorSymbol.Parameters;
         for (int i = 0; i < parameters.Count; i++)
         {
-            constructorBuilder.DefineParameter(i, ParameterAttributes.None, parameters[i].Name);
+            // The parameter index is offset by 1.
+            constructorBuilder.DefineParameter(i + 1, ParameterAttributes.None, parameters[i].Name);
         }
 
         if (isDelegateConstructor)
@@ -556,8 +557,10 @@ public static ConstructorInfo AsConstructorInfo(this IMethodSymbol methodSymbol)
             parameter.Type.AsType(type.GenericTypeArguments, buildType: true);
         }
 
-        ConstructorInfo? constructorInfo = type.GetConstructor(
-            methodSymbol.Parameters.Select((p) => p.Type.AsType()).ToArray());
+        BindingFlags bindingFlags = BindingFlags.Public | BindingFlags.Instance;
+        ConstructorInfo? constructorInfo = type.GetConstructors(bindingFlags)
+            .FirstOrDefault((c) => c.GetParameters().Select((p) => p.Name).SequenceEqual(
+                methodSymbol.Parameters.Select((p) => p.Name)));
         return constructorInfo ?? throw new InvalidOperationException(
                 $"Constructor not found for type: {type.Name}");
     }
diff --git a/src/NodeApi/Interop/JSPropertyDescriptorListOfT.cs b/src/NodeApi/Interop/JSPropertyDescriptorListOfT.cs
@@ -215,4 +215,14 @@ public TDerived AddMethod(
           attributes,
           data);
     }
+
+    public TDerived AddMethod(
+        string name,
+        JSCallbackDescriptor callbackDescriptor,
+        JSPropertyAttributes attributes = JSPropertyAttributes.DefaultMethod)
+    {
+        Properties.Add(JSPropertyDescriptor.Function(
+            name, callbackDescriptor.Callback, attributes, callbackDescriptor.Data));
+        return (TDerived)(object)this;
+    }
 }
diff --git a/test/TestCases/napi-dotnet/Overloads.cs b/test/TestCases/napi-dotnet/Overloads.cs
@@ -0,0 +1,65 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+namespace Microsoft.JavaScript.NodeApi.TestCases;
+
+[JSExport]
+public class Overloads
+{
+    public Overloads()
+    {
+    }
+
+    public Overloads(int intValue)
+    {
+        IntValue = intValue;
+    }
+
+    public Overloads(string stringValue)
+    {
+        StringValue = stringValue;
+    }
+
+    public Overloads(int intValue, string stringValue)
+    {
+        IntValue = intValue;
+        StringValue = stringValue;
+    }
+
+    public Overloads(ITestInterface obj)
+    {
+        StringValue = obj.Value;
+    }
+
+    public int? IntValue { get; private set; }
+
+    public string? StringValue { get; private set; }
+
+    public void SetValue(int intValue)
+    {
+        IntValue = intValue;
+    }
+
+    public void SetValue(string stringValue)
+    {
+        StringValue = stringValue;
+    }
+
+    public void SetValue(int intValue, string stringValue)
+    {
+        IntValue = intValue;
+        StringValue = stringValue;
+    }
+
+    public void SetValue(ITestInterface obj)
+    {
+        StringValue = obj.Value;
+    }
+
+    // Method with overloaded name in C# is given a non-overloaded export name.
+    [JSExport("setDoubleValue")]
+    public void SetValue(double doubleValue)
+    {
+        IntValue = (int)doubleValue;
+    }
+}
diff --git a/test/TestCases/napi-dotnet/overloads.js b/test/TestCases/napi-dotnet/overloads.js

Original file line number	Diff line number	Diff line change
`@@ -414,7 +414,8 @@ private static ConstructorBuilder BuildSymbolicConstructor(`
`414`	`414`	`IReadOnlyList<IParameterSymbol> parameters = constructorSymbol.Parameters;`
`415`	`415`	`for (int i = 0; i < parameters.Count; i++)`
`416`	`416`	`{`
`417`		`- constructorBuilder.DefineParameter(i, ParameterAttributes.None, parameters[i].Name);`
	`417`	`+ // The parameter index is offset by 1.`
	`418`	`+ constructorBuilder.DefineParameter(i + 1, ParameterAttributes.None, parameters[i].Name);`
`418`	`419`	`}`
`419`	`420`
`420`	`421`	`if (isDelegateConstructor)`
`@@ -556,8 +557,10 @@ public static ConstructorInfo AsConstructorInfo(this IMethodSymbol methodSymbol)`
`556`	`557`	`parameter.Type.AsType(type.GenericTypeArguments, buildType: true);`
`557`	`558`	`}`
`558`	`559`
`559`		`- ConstructorInfo? constructorInfo = type.GetConstructor(`
`560`		`- methodSymbol.Parameters.Select((p) => p.Type.AsType()).ToArray());`
	`560`	`+ BindingFlags bindingFlags = BindingFlags.Public \| BindingFlags.Instance;`
	`561`	`+ ConstructorInfo? constructorInfo = type.GetConstructors(bindingFlags)`
	`562`	`+ .FirstOrDefault((c) => c.GetParameters().Select((p) => p.Name).SequenceEqual(`
	`563`	`+ methodSymbol.Parameters.Select((p) => p.Name)));`
`561`	`564`	`return constructorInfo ?? throw new InvalidOperationException(`
`562`	`565`	`$"Constructor not found for type: {type.Name}");`
`563`	`566`	`}`