[SwiftBindings] Add support for non-frozen structs #2870

jkurdek · 2024-12-11T17:43:35Z

Contributes to #2822

This PR brings some improvements to projecting structs

Adds basic projections of non frozen structs
Adds usage of runtimes SwiftSelf
Adds usage of runtimes SwiftIndirectResult

TODO:
Simplify testing

jkurdek · 2024-12-11T17:45:42Z

src/Swift.Bindings/src/Parser/SwiftABIParser.cs

-                        decl = CreateClassDecl(node, parentDecl, moduleDecl);
-                    }
+                    // TODO: Find better place for this
+                    typeRecord.IsBlittable = !swiftTypeInfo.ValueWitnessTable->IsNonPOD || !swiftTypeInfo.ValueWitnessTable->IsNonBitwiseTakable; //TODO: This is not the full picture.


Deciding on whether a struct should be projected as a class or struct on c# side is more complex than that. We will have to implement a correct algorithm for that. I guess for now the question is where this should live. ABIParsing?

Since it's not part of the actual parsing, my inclination is no.
Measuring the size, stride, and alignment and determining blittability should be a post processing step and should end up in the type database entry for the type.

I agree that metadata info should live in the type database. I suggest to retrieve it using MachO reader, but I think it is important to have it at parsing time. Do we want to read .dylib before the ABI parsing to consume information for projections from type database?

I agree that this information should end up in TypeDatabase. But I think it also needs to be present on a TypeDecl, otherwise handling each TypeDecl would require reading TypeDatabase entry.

Do we want to read .dylib before the ABI parsing to consume information for projections from type database?

I am not sure I understand

Populate type database using MachO reader before ABI parsing, so metadata information can be consumed without calling into dylib from HandleTypeDecl.

I was looking into how to do this and it's a little more complicated than you think. The problem is that the value witness table for any particular type might not live in the dylib that you're reading. For example, if I define a swift type like this:

public struct NoInThisDylib { public let contents: Int public init(with: Int) { contents = with } }

Then the value witness table for this type is an external reference to the value witness table for Int64 from libswiftCore.dylib. IIRC all heap allocated types (classes) share the same value witness table.

I created the following issues to track the outcomes of the discussion:
#2892
#2886

jkurdek · 2024-12-11T17:50:03Z

src/Swift.Bindings/src/Emitter/StringCSharpEmitter/Handler/MethodHandler.cs

+        /// <param name="env">The environment.</param>
+        /// <param name="conductor">The conductor instance.</param>
+        /// <param name="typeDatabase">The type database instance.</param>
+        public void Emit(IndentedTextWriter writer, IEnvironment env, Conductor conductor, TypeDatabase typeDatabase)


TypeDatabase will be most likely used in majority of the handlers. Maybe it would make sense to inject it into every handler during their construction

It should live in the environment, I suspect.

Is TypeDatabase a singleton?

I don't see any reason to not have TypeDatabase as singleton. Ideally it should be immutable.

Added TypeDatabase to env

stephen-hawley

This is a great start. I will come back and look at this more closely later. I wanted to provide immediate feedback so you could start thinking about changes you might need.

src/Swift.Bindings/src/Emitter/StringCSharpEmitter/Handler/MethodHandler.cs

stephen-hawley · 2024-12-11T19:11:57Z

src/Swift.Bindings/src/Emitter/StringCSharpEmitter/Handler/MethodHandler.cs

+            writer.WriteLine("{");
+            writer.Indent++;
+
+            string PInvokeName = $"{methodEnv.PInvokePrefix}{methodDecl.Name}";


Naming methods is going to be very common. I'd like to see naming centralized.

I agree, especially with PInvoke methods (i.e. multiple constructors)

I think there is (at least) two types of naming we need to handle:

Naming things projected from Swift - some things in Swift are unpronouncable (e.g. emoji names) in csharp. We need to do the translation.

Generating names for extra things we need - pinvokes, proxies etc.

When talking about centralizing naming do we want to have one thing doing 1. and 2. ?

Edit: Number one could probably be done at parsing stage and encoded in the TypeDatabase. So this should leave only number 2

Naming things projected from Swift - some things in Swift are unpronouncable (e.g. emoji names) in csharp. We need to do the translation.

I suggest creating a tracking issue and deferring this to a later stage. Do we want to support operator overloading?

There's a third case of naming, which is local variables used in marshaling.

src/Swift.Bindings/src/Emitter/StringCSharpEmitter/Handler/MethodHandler.cs

stephen-hawley · 2024-12-11T19:17:27Z

src/Swift.Bindings/src/Emitter/StringCSharpEmitter/Handler/MethodHandler.cs

-            List<ArgumentDecl> tempDecl = new(methodDecl.CSSignature);
+            foreach (var argument in MethodDecl.CSSignature.Skip(1))
+            {
+                var typeRecord = TypeDatabase.Registrar.GetType(argument);


This should be a TryGet pattern since the type may not exist. In addition, if the type database entry comes from an external module, you will either need to use the full type name of inject a using statement.

We should always use fully qualified names - to avoid unintentional collisions. In fact we should use the global: prefix as well to make sure we don't pick up some other random namespace item. Just like Roslyn source generator.

@stephen-hawley, I'm thinking about the behaviour on TryGet method failure. Should we just skip this methods projection? Or we should risk getting it wrong?

In BTfS for this class of failure it was treated as a warning and a skip for the method.

I think in order to properly skip the method we need to check all the conditions before we write any code. (this is particularly important if code generation is non-linear - If I encounter problem writing the pinvoke I want to cancel writing the wrapper as well)

We could do this by creating something like "transactions" - when we're about to start projecting a method, we "start a transaction" which will reset all output buffers. Then we project/write and everything goes into output buffers, only when we're successfully done, we "commit" and appends the buffers to the actual output.
This would be more robust than trying to check for every error condition upfront - it's easy to miss something, and that doesn't count actual unexpected exceptions during the writing process (our bugs and so on). It would be really nice if the tool was resilient to its own bugs to some degree.

if the type database entry comes from an external module, you will either need to use the full type name of inject a using statement.

I agree. I think we should add some mechanism which allows us to resolve a fully qualified c# name from any Decl and any TypeDatabase entry. This should probably be bundled with the translation of unpronouncable names is csharp. So lets track it together. Out of scope of this PR. #2889

This should be a TryGet pattern since the type may not exist.

I like the "transactional" idea. Again I think it will require some separate discussion. Lets do it separately. Tracking issue: #2890.

.gitignore

kotlarmilos · 2024-12-12T09:36:30Z

src/Swift.Bindings/src/Emitter/StringCSharpEmitter/Handler/MethodHandler.cs

+        /// <param name="env">The environment.</param>
+        /// <param name="conductor">The conductor instance.</param>
+        /// <param name="typeDatabase">The type database instance.</param>
+        public void Emit(IndentedTextWriter writer, IEnvironment env, Conductor conductor, TypeDatabase typeDatabase)


Is TypeDatabase a singleton?

src/Swift.Bindings/src/Emitter/StringCSharpEmitter/Handler/MethodHandler.cs

kotlarmilos · 2024-12-12T09:42:07Z

src/Swift.Bindings/src/Emitter/StringCSharpEmitter/Handler/MethodHandler.cs

+            writer.WriteLine("{");
+            writer.Indent++;
+
+            string PInvokeName = $"{methodEnv.PInvokePrefix}{methodDecl.Name}";


I agree, especially with PInvoke methods (i.e. multiple constructors)

src/Swift.Bindings/src/Marshaler/MarshallingExtensions.cs

kotlarmilos · 2024-12-12T09:57:41Z

src/Swift.Bindings/src/Marshaler/MarshallingExtensions.cs

+            return decl is StructDecl structDecl && structDecl.IsFrozen && structDecl.IsBlittable;
+        }
+
+        public static TypeRecord GetType(this TypeRegistrar typeRegistrar, ArgumentDecl argumentDecl)


Ideally, this should be part of type database. I think we should create a clear type database interface as it will be used across all components of the tooling.

I agree. Currently type database accepts queries for types using module name and type name. I believe a lot of the queries will be driven by looking at some typeSpec, so maybe we can include that in the interface

Here's something to put a pin in: Swift is simple in that there is only one namespace: the module name. C# not so much. In either case, it might be handy to have something like this:

public class ClassName { // implement IEquatable<> etc. public ClassName(string namespace, string name) { Namespace = namespace; Name = name; FullName = Namespace + "." + Name; } public string Namespace { get; init; } public string Name { get; init; } // may include inner classes as a path public string FullName { get; init } }

And use this wherever you want to refer to either a C# class or a Swift class.
When writing code non-linearly, we can do namespaces.AddIfNotPresent(clname.Namespace) or imports.AddIfNotPresent(clname.Namespace).

src/Swift.Bindings/src/Emitter/StringCSharpEmitter/Handler/TypeHandler.cs

src/Swift.Bindings/src/Parser/SwiftABIParser.cs

kotlarmilos · 2024-12-12T10:06:53Z

src/Swift.Bindings/src/Parser/SwiftABIParser.cs

-                        decl = CreateClassDecl(node, parentDecl, moduleDecl);
-                    }
+                    // TODO: Find better place for this
+                    typeRecord.IsBlittable = !swiftTypeInfo.ValueWitnessTable->IsNonPOD || !swiftTypeInfo.ValueWitnessTable->IsNonBitwiseTakable; //TODO: This is not the full picture.


I agree that metadata info should live in the type database. I suggest to retrieve it using MachO reader, but I think it is important to have it at parsing time. Do we want to read .dylib before the ABI parsing to consume information for projections from type database?

src/Swift.Runtime/src/TypeDatabase.xml

vitek-karas · 2024-12-12T10:32:03Z

src/Swift.Bindings/tests/NonFrozenStructs/NonFrozenStructsTests.cs

+                _assemblyPath = TestsHelper.Compile(
+                new string[] { "NonFrozenStructs/*.cs" },
+                new string[] { },
+                new string[] { });


Nit: Indentation

kotlarmilos

LGTM! Thanks!

jkurdek added the area-SwiftBindings Swift bindings for .NET label Dec 11, 2024

jkurdek requested review from kotlarmilos, stephen-hawley and matouskozak December 11, 2024 17:43

jkurdek self-assigned this Dec 11, 2024

jkurdek commented Dec 11, 2024

View reviewed changes

stephen-hawley requested changes Dec 11, 2024

View reviewed changes

kotlarmilos reviewed Dec 12, 2024

View reviewed changes

vitek-karas reviewed Dec 12, 2024

View reviewed changes

kotlarmilos mentioned this pull request Dec 13, 2024

[Swift language features] Implement Swift structs in C# #2822

Open

7 tasks

jkurdek force-pushed the swift/non-frozen-structs branch from 34b4a19 to 462b0a1 Compare December 17, 2024 00:34

This was referenced Dec 17, 2024

[SwiftBindings] Revert emitting Swift projection on error #2890

Open

[SwiftBindings] Retrieve metadata information #2892

Open

stephen-hawley approved these changes Dec 17, 2024

View reviewed changes

jkurdek added 7 commits December 17, 2024 22:51

Added SwiftSelf to function signatures

0d11977

Added projections of non-frozen functions

b54ced7

TypeDatabase on env

1b8a52f

Rework signature handling

bba6348

Add centralized naming

974893c

Reverted extension methods

17e2109

Fix tests after rebase

5fd8181

jkurdek force-pushed the swift/non-frozen-structs branch from 462b0a1 to 5fd8181 Compare December 17, 2024 23:48

kotlarmilos self-requested a review December 18, 2024 11:02

kotlarmilos approved these changes Dec 18, 2024

View reviewed changes

jkurdek merged commit a9ce1b5 into feature/swift-bindings Dec 18, 2024
6 checks passed

jkurdek deleted the swift/non-frozen-structs branch December 18, 2024 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SwiftBindings] Add support for non-frozen structs #2870

[SwiftBindings] Add support for non-frozen structs #2870

jkurdek commented Dec 11, 2024 •

edited

Loading

jkurdek Dec 11, 2024

stephen-hawley Dec 11, 2024

kotlarmilos Dec 12, 2024

jkurdek Dec 12, 2024 •

edited

Loading

kotlarmilos Dec 12, 2024

stephen-hawley Dec 12, 2024

jkurdek Dec 17, 2024

jkurdek Dec 11, 2024

stephen-hawley Dec 11, 2024 •

edited

Loading

kotlarmilos Dec 12, 2024

jkurdek Dec 12, 2024

jkurdek Dec 17, 2024

stephen-hawley left a comment

stephen-hawley Dec 11, 2024

kotlarmilos Dec 12, 2024

jkurdek Dec 12, 2024 •

edited

Loading

kotlarmilos Dec 13, 2024

stephen-hawley Dec 16, 2024

jkurdek Dec 17, 2024

stephen-hawley Dec 11, 2024

vitek-karas Dec 12, 2024

jkurdek Dec 12, 2024

stephen-hawley Dec 12, 2024

jkurdek Dec 12, 2024

vitek-karas Dec 17, 2024

jkurdek Dec 17, 2024

kotlarmilos Dec 12, 2024

kotlarmilos Dec 12, 2024

kotlarmilos Dec 12, 2024

jkurdek Dec 12, 2024

stephen-hawley Dec 17, 2024

kotlarmilos Dec 12, 2024

vitek-karas Dec 12, 2024

kotlarmilos left a comment

[SwiftBindings] Add support for non-frozen structs #2870

[SwiftBindings] Add support for non-frozen structs #2870

Conversation

jkurdek commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkurdek Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephen-hawley Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephen-hawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkurdek Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotlarmilos left a comment

Choose a reason for hiding this comment

jkurdek commented Dec 11, 2024 •

edited

Loading

jkurdek Dec 12, 2024 •

edited

Loading

stephen-hawley Dec 11, 2024 •

edited

Loading

jkurdek Dec 12, 2024 •

edited

Loading