Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add experimental Swift bindings for CryptoKit #2704

Merged

Conversation

kotlarmilos
Copy link
Member

Description

This PR introduces experimental Swift bindings for CryptoKit. It validates the Swift interop and demonstrates the Swift bindings surface for end-users.

The bindings are manually created as a proof of concept. In the future, the bindings should be reduced, and they should be automatically generated by the projection tooling

Current limitations:

Resolves #2580

[InlineArray(16)]
public unsafe struct Data : ISwiftObject
{
private byte _payload;
Copy link
Member

@jkurdek jkurdek Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until those bits flow dotnet/runtime#108483, this will fail on mono

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we test only CoreCLR.

public bool HasExtraInhabitants => ExtraInhabitantCount != 0;
}

public static unsafe void* GetMetadata<T>(T type) where T: ISwiftObject

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be generic?

public static unsafe void* GetMetadata<T>(ISwiftObject obj) {
    return obj.Metadata;
}

While you don't use it here, we will need a more general version of this that takes the C# Type as an argument and returns the type metadata from that so that we can get the type metadata for the full scope of Swift projectable types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is convenient for retrieval. Alternatively, it can be retrieved using reflection:

        public static unsafe void* GetMetadata(Type t)
        {
            if (typeof(ISwiftObject).IsAssignableFrom(t))
            {
                var metadataProperty = t.GetProperty("Metadata", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.Public);
                if (metadataProperty != null)
                {
                    var metadataValue = metadataProperty.GetValue(null);
                    if (metadataValue is IntPtr ptr)
                    {
                        return ptr.ToPointer();
                    }
                }
                throw new InvalidOperationException($"Type {t.Name} does not have a 'Metadata' property.");
            }
            throw new ArgumentException($"Type {t.Name} does not implement ISwiftObject.");
        }

I believe we should consider all use cases before making such changes, but I'm also open to refactoring it now. What do you prefer?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to take some thought, but I think the main entry point should be something like:

public static bool TryGetMetadata (Type t, out Metadata metadata)
{
    // one catch - t must not be an unbound generic.
    if (typeof(ISwiftObject).IsAssignableFrom(t)) {
        metadata = GetMetadataFromISwiftObject(t);
        return true;
    }
    // over time we add the cases we need for tuples, nint, nuint, nfloat, etc.
    // I'd like to see this ultimately be a ladder of `else if` constructs for each major type ordered by
    // most common and/or cheapest predicates. For example, identifying certain scalars should be
    // a matter of looking at `t.GetTypeCode()`
    return false;
}

static Metadata GetMetadataFromISwiftObject(Type t) // t is guaranteed to be ISwiftObject compatible 
{
    // your reflection code - no try/get pattern needed, this is a private API
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's talk more about this. Created a tracking issue: #2705

{
if (handle != IntPtr.Zero)
{
NativeLibrary.Free(handle);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:
1 - will this free an already loaded library?
2 - you don't need an IntPtr.Zero check, this is documented as doing nothing on IntPtr.Zero

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention is to free the loaded library only if NativeLibrary.GetExport fails, which is why handle != IntPtr.Zero is used.

IntPtr handle = IntPtr.Zero;
try
{
handle = NativeLibrary.Load(Foundation.Path);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only work for conformance descriptors that are in Foundation - there will be other modules that have conformance descriptors. We should open an issue on this so it doesn't get lost.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a tracking issue: #2705

/// </summary>
public unsafe interface ISwiftObject
{
public static abstract void* Metadata { get; }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the future - I'd really like to see Metadata be its own type - a struct containing either an IntPtr, a NativeHandle, or a void*. There will be sufficient funcs that will have Metadata as arguments that we are likely to have signature conflicts. Let's open up an issue for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a tracking issue: #2705

// <summary>
// Represents Swift UnsafeMutablePointer in C#.
// </summary>
public readonly unsafe struct UnsafeMutablePointer<T> where T : unmanaged

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to correct this now, but this code will cause memory leaks without it honoring the Swift memory model. Please open an issue for this.

Copy link
Member Author

@kotlarmilos kotlarmilos Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as a task Review memory management of the projected pointer types in dotnet/runtime#95633. Plan to collect more details and open a tracking issue.

// </summary>
[StructLayout(LayoutKind.Sequential, Size = 16)]
[InlineArray(16)]
public unsafe struct Data : ISwiftObject

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not important right now, but in the future we will also want Data to implement friendlier C# interfaces (IEnumerable or ICollection etc). Let's open an issue for that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as a task Implement common C# interfaces (IEnumerable, ICollection, etc) in dotnet/runtime#95633. Plan to collect more details and open a tracking issue.

@kotlarmilos kotlarmilos merged commit 855a05b into feature/swift-bindings Oct 9, 2024
6 checks passed
@kotlarmilos kotlarmilos deleted the swift-bindings/cryptokit-example branch October 9, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-SwiftBindings Swift bindings for .NET
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants