dotnet / runtimelab

This repo is for experimentation and exploring new ideas that may or may not make it into the main dotnet/runtime repo.
MIT License
1.37k stars 189 forks source link

Review UX and bindings for .NET Swift interop tooling in simple P/Invoke void cases #2518

Closed kotlarmilos closed 4 months ago

kotlarmilos commented 5 months ago

Description 

This issue is intended to discuss the user experience and bindings generated by the Swift bindings tooling available at https://github.com/dotnet/runtimelab/pull/2517. The focus is on evaluating the quality of the generated C# code, scalability and trimming, and the overall UX experience.

Usage

Example usage:

dotnet SwiftBindings.dll -d "./libHelloLibrary.dylib" -s "./HelloLibrary.swiftinterface" -o "./"

Options:

  -d, --dylibs             Required. Paths to the dynamic libraries (dylibs), separated by commas.
  -s, --swiftinterfaces    Required. Paths to the Swift interface files, separated by commas.
  -o, --output             Required. Output directory for generated bindings.
  -h, --help               Display this help message.
  --version                Display version information.

When an unknown type is encountered, the tooling will report the error and proceed with bindings generation.

Simple binding

The supported scenario involves simple P/Invoke calls without arguments, with a void return type and no exceptions.

Swift library:

public func sayHello() {
    print("Hello world")
}

Generated C# bindings:


using System;
using System.Runtime.InteropServices;
namespace HelloLibrary
{
    public class TopLevelEntities
    {
        public static void SayHello()
        {
            NativeMethodsForTopLevelEntities.PIfunc_SayHello();
        }
    }
    internal class NativeMethodsForTopLevelEntities
    {
        [DllImport("libHelloLibrary.dylib", EntryPoint = "$s12HelloLibrary03sayA0yyF")]
        internal static extern void PIfunc_SayHello();
    }
}

User's C# code:

using System;
using HelloLibrary;

namespace HelloWorld
{
    public class Program
    {
        public static void Main(string[] args)
        {
            TopLevelEntities.SayHello();
        }
    }
}

/cc: @stephen-hawley @AaronRobinsonMSFT @vitek-karas

rolfbjarne commented 5 months ago

The supported scenario involves simple P/Invoke calls without arguments and with a void return type.

The supported scenario involves simple P/Invoke calls without arguments, with a void return type and no exceptions.

AaronRobinsonMSFT commented 5 months ago
  -d, --dylibs             Required. Paths to the dynamic libraries (dylibs), separated by commas.
  -s, --swiftinterfaces    Required. Paths to the Swift interface files, separated by commas.

This seems like a cumbersome parsing argument. I would instead allow multiple -d and -s flags so each one is independent and adding or removing is easier than altering a string that is then parsed by another process. Since this will eventually be done in MSBuild permitting more than one instance will make the command line creation easier.

AaronRobinsonMSFT commented 5 months ago

Any sort of logging/verbosity mechanism that we need? I would reference some of the existing tooling, e.g., crossgen, we have for other options. Perhaps @jkoritzinsky knows of others to reference.

jkoritzinsky commented 5 months ago

Is there a way we could enable users to specify an Apple framework name and that would automatically discover the dylibs and swiftinterface files? Something like --framework CryptoKit?

On the generated code: I'd recommend using either a local function for the inner DllImport or at least making the NativeMethodsForTopLevelEntities class file-scoped to reduce the emitted API surface area.

rolfbjarne commented 5 months ago
  -d, --dylibs             Required. Paths to the dynamic libraries (dylibs), separated by commas.
  -s, --swiftinterfaces    Required. Paths to the Swift interface files, separated by commas.

This seems like a cumbersome parsing argument. I would instead allow multiple -d and -s flags so each one is independent and adding or removing is easier than altering a string that is then parsed by another process. Since this will eventually be done in MSBuild permitting more than one instance will make the command line creation easier.

Someone will inevitably end up trying to bind a dylib with a name containing a comma at some point... so yes, one file per -d/-s and then allowing multiple arguments sounds like the better option.

vitek-karas commented 5 months ago

Naming nit:

        public static void Main(string[] args)
        {
            TopLevelEntities.SayHello();
        }

I assume the TopLevelEntities name is "hardcoded". Personally not a fan, because if I have two such libraries they will both have TopLevelEntities and it will be a mess. Also, it's harder to read, since it doesn't say where the SayHello came from. It's not a nice API design. Did we consider reusing the library name, so that it would be for example HelloLibrary.SayHello()?.

kotlarmilos commented 4 months ago

Any sort of logging/verbosity mechanism that we need? I would reference some of the existing tooling, e.g., crossgen, we have for other options. Perhaps @jkoritzinsky knows of others to reference.

Are you referring to https://github.com/dotnet/runtime/blob/7185df8eda6c25d101170a2809094adc9f493b51/src/coreclr/tools/aot/crossgen2/Crossgen2RootCommand.cs#L69-L70? There is a similar mechanism in the existing tooling that we can rely on https://github.com/xamarin/binding-tools-for-swift/blob/59d29705abe31066651a79ef900bd0c0e0fa4133/tom-swifty/SwiftyOptions.cs#L98-L100. I will update the CLI to include verbosity.

Is there a way we could enable users to specify an Apple framework name and that would automatically discover the dylibs and swiftinterface files? Something like --framework CryptoKit?

It will be essential to include this feature. Users should also have the option to specify the framework along with platform and SDK version as optional parameters. There is a text-based stub library file used for linking against frameworks and the .swiftinterface file. Here is a proposal:

if (options.Help)
{
    Console.WriteLine("Usage:");
    Console.WriteLine("  -d, --dylib            Paths to the dynamic libraries (dylibs)");
    Console.WriteLine("  -s, --swiftinterface   Paths to the Swift interface files");
    Console.WriteLine("  -o, --output            Output directory for generated bindings.");
    Console.WriteLine("  --framework             Framework name.");
    Console.WriteLine("  --framework-platform    Platform for the framework.");
    Console.WriteLine("  --framework-sdk-version SDK version for the framework. ");
}

Did we consider reusing the library name, so that it would be for example HelloLibrary.SayHello()?.

Good point. It is more intuitive to reuse the library name instead of a hardcoded name.

kotlarmilos commented 4 months ago

An alternative option is to utilize the .abi.json file generated by the Swift compiler. It can be generated even from a Swift interface using the xcrun swift-frontend -compile-module-from-interface command. This approach simplifies the UX by accepting only ABI files, whether it is generated from user code or a framework. Additionally, json is convenient for readability and maintenance within the tooling. For reference, here is the generated ABI for the HelloWorld sample: https://gist.github.com/kotlarmilos/90fe104de7722a1c0a3271b1f5d00968.

@stephen-hawley Do you know of any limitations with the ABI file compared to the .dylib + .swiftinterface?

stephen-hawley commented 4 months ago

I don't know of limitations WRT to the ABI compared to a .swiftinterface. What it looks like to me is a json representation of the the abstract syntax tree of the .swiftinterface.

I do not, however, recommend that we go this route. Where this is problematic is that we immediately lose a lot of the benefits from using an antlr based parser of the .swiftinterface: type-strong parsing. This means that we would have to write something to first turn the parsed json into another AST then write code to walk the AST to generate the types we need to represent the ABI. In other words more technical debt. In looking at Apple's own use of this tool, it appears that they're using it for doing API diffs, not for actual compilation. I would be worried about that, especially if they made changes to the layout of the json or decided they didn't need it anymore. The .swiftinterface file at least follows the language which has a complete specification.

stephen-hawley commented 4 months ago

Naming nit:

        public static void Main(string[] args)
        {
            TopLevelEntities.SayHello();
        }

I assume the TopLevelEntities name is "hardcoded". Personally not a fan, because if I have two such libraries they will both have TopLevelEntities and it will be a mess. Also, it's harder to read, since it doesn't say where the SayHello came from. It's not a nice API design. Did we consider reusing the library name, so that it would be for example HelloLibrary.SayHello()?.

"TopLevelEntities" in BTfS is overridable as an option (--GlobalClassName). The name isn't important so long as it makes it clear what it represents. Using the library name is confusing and could cause problems because frequently the library name and the module name are identical. When SayHello is fully qualified, it's really HelloLibrary.HelloLibrary.SayHello which opens up possibilities for ambiguities. Personally, I'd prefer something that's clear as to how it represents the swift projection. For example Top, TopLevel, Globals, GlobalFuncs (not really accurate since you can make global variables, but those are discouraged).

kotlarmilos commented 4 months ago

I don't know of limitations WRT to the ABI compared to a .swiftinterface. What it looks like to me is a json representation of the the abstract syntax tree of the .swiftinterface.

I do not, however, recommend that we go this route. Where this is problematic is that we immediately lose a lot of the benefits from using an antlr based parser of the .swiftinterface: type-strong parsing. This means that we would have to write something to first turn the parsed json into another AST then write code to walk the AST to generate the types we need to represent the ABI. In other words more technical debt. In looking at Apple's own use of this tool, it appears that they're using it for doing API diffs, not for actual compilation. I would be worried about that, especially if they made changes to the layout of the json or decided they didn't need it anymore. The .swiftinterface file at least follows the language which has a complete specification.

I am looking to lock the UX design, and using .swiftinterface seems to be the best approach. The ABI file generated from the .swiftinterface could be an alternative but it is not the source of truth and might lead to breaking changes if Apple decides to abandon it. Ultimately, the ABI aggregation should adhere to the SwiftReflector declarations.

However, consuming the .dylib appears to be redundant and limits us from integrating frameworks where only .swiftinterface is available, like in CryptoKit case. @stephen-hawley, could you share scenarios where the use of .dylib is necessary?

"TopLevelEntities" in BTfS is overridable as an option (--GlobalClassName). The name isn't important so long as it makes it clear what it represents. Using the library name is confusing and could cause problems because frequently the library name and the module name are identical. When SayHello is fully qualified, it's really HelloLibrary.HelloLibrary.SayHello which opens up possibilities for ambiguities. Personally, I'd prefer something that's clear as to how it represents the swift projection. For example Top, TopLevel, Globals, GlobalFuncs (not really accurate since you can make global variables, but those are discouraged).

This is great to be parametrized. Here's an alternative proposal that should avoid naming complexity at this stage:


using System;
using System.Runtime.InteropServices;
namespace HelloLibraryBindings
{
    public class HelloLibrary
    {

    }
}
kotlarmilos commented 4 months ago

Thank you for your feedback, below is the summary.

Usage

The tooling is designed to process both .swiftinterface files and .dylib files for API aggregation. Temporarily, the Swift interface parser has been removed from the project to narrow the scope of https://github.com/dotnet/runtimelab/pull/2517. It is planned to introduce this parser in subsequent PRs. Currently, the tooling consumes Swift ABI file that is generated from the .swiftinterface file by the Swift compiler. We will explore its usability long-term.

Current tooling options:

Description:
  Swift bindings generator.
Usage:
  SwiftBindings [options]
Options:
  -a, --swiftabi <swiftabi> (REQUIRED)  Path to the Swift ABI file.
  -o, --output <output> (REQUIRED)      Output directory for generated bindings.
  -v, --verbose <verbose>               Prints information about work in process.
  -h, --help                            Display a help message.
  --version                             Show version information
  -?, -h, --help                        Show help and usage information

Target tooling options:

Description:
  Swift bindings generator.
Usage:
  SwiftBindings [options]
Options:
  -i, --swiftinterface <swiftinterface> (REQUIRED)  Path to the Swift interface file.
  -d, --dylib <swiftinterface> (REQUIRED)           Path to the dynamic library.
  -o, --output <output> (REQUIRED)                  Output directory for generated bindings.
  -v, --verbose <verbose>                           Prints information about work in process.
  -h, --help                                        Display a help message.
  --version                                         Show version information
  -?, -h, --help           

Projections

Each provided module is projected into a separate assembly. The assembly contains a namespace named <ModuleName>Bindings and includes a class named <ModuleName>. For each public function, a P/Invoke signature and a corresponding method are generated. Below is an example to illustrate this process.

using System;
using System.Runtime.InteropServices;
namespace HelloLibraryBindings
{
    public class HelloLibrary
    {
        [DllImport("libHelloLibrary.dylib", EntryPoint = "$s12HelloLibrary03sayA0yyF")]
        internal static extern void PIfunc_sayHello();
        public static void sayHello()
        {
            PIfunc_sayHello();
        }
    }
}