-
Notifications
You must be signed in to change notification settings - Fork 200
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
Add experimental Swift bindings for CryptoKit #2704
Conversation
[InlineArray(16)] | ||
public unsafe struct Data : ISwiftObject | ||
{ | ||
private byte _payload; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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:
SwiftSelf<T>
runtime#107946Resolves #2580