Skip to content

[Swift Bindings] Ensure projection tooling generate bindings that compile for selected frameworks#2900

Merged
kotlarmilos merged 12 commits intodotnet:feature/swift-bindingsfrom
kotlarmilos:swift-bindings/unknown-types
Dec 20, 2024
Merged

[Swift Bindings] Ensure projection tooling generate bindings that compile for selected frameworks#2900
kotlarmilos merged 12 commits intodotnet:feature/swift-bindingsfrom
kotlarmilos:swift-bindings/unknown-types

Conversation

@kotlarmilos
Copy link
Copy Markdown
Member

@kotlarmilos kotlarmilos commented Dec 19, 2024

Description

This PR:

  1. Ensures that the generated bindings for frameworks compile successfully
  2. Handles unknown types gracefully
  3. Simplifies the projection tooling UX
  4. Adds projection tooling requirements

Generated bindings that compile for StoreKit2: https://gist.github.com/kotlarmilos/907cde8ce6af5f2730f8f33e432a503e

Changes

This PR handles unknown types gracefully by introducing AnyType as a Swift placeholder type. Additionally, it simplifying the projection tooling UX by removing unnecessary args and unifies the Swift and Swift.Runtime namespaces into a single directory

This PR does not change the layout of projections.

Validation

Both official and PR pipelines now include a bindings compilation check for frameworks to ensure that generated code can compile.

Copy link
Copy Markdown
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

I think it would be a good idea to zero out _payload after freeing it, which would avoid any accidental access of freed memory:

https://gist.github.com/kotlarmilos/907cde8ce6af5f2730f8f33e432a503e#file-swift-storekit-cs-L105

https://gist.github.com/kotlarmilos/907cde8ce6af5f2730f8f33e432a503e#file-swift-storekit-cs-L112

Copy link
Copy Markdown

@jkurdek jkurdek left a comment

Choose a reason for hiding this comment

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

I think that gracefully handling unknown types will be tricky. The complexity is in the fact that given an argument we need to make an ABI decision (indirect result, opaque pointer, etc). We wont be able to make those decision if we dont find the type in the TypeDatabase.

If compilation is our goal for now maybe we can do the following approach. Do AnyType subtitution in wrappers signatures - this should work as this does not touch ABI. And then just not generate pinvokes when argument is not known.

var returnType = MethodDecl.CSSignature.First().CSTypeIdentifier.Name;
var argument = MethodDecl.CSSignature.First();
var returnType = argument.CSTypeIdentifier.Name;
var typeRecord = MarshallingHelpers.GetType(argument, TypeDatabase.Registrar);
Copy link
Copy Markdown

@jkurdek jkurdek Dec 20, 2024

Choose a reason for hiding this comment

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

I feel that we now operate on two abstraction layers in this call. We have high-level MethodRequiresIndirectResult which calls type database and low level MarshallingHelpers.GetType which can lead to confusing behaviour as discussed in previous comment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I feel that some marshalling should be handled before emitting. However, I would prefer implementing more support before structuring it.

When replacing unsupported types with AnyType placeholders, method conflicts might occur (same name and signature). To address this, I completely skipped generating wrappers to ensure compilation succeeds. The next step would be to reduce these methods during marshalling and emit them with a NotImplementedException body.

@kotlarmilos
Copy link
Copy Markdown
Member Author

If compilation is our goal for now maybe we can do the following approach. Do AnyType subtitution in wrappers signatures - this should work as this does not touch ABI. And then just not generate pinvokes when argument is not known.

I think this is a good start -- it is safe to assume that AnyType signatures will not be used. Additionally, the AnyType placeholder highlights missing language features that we may want to focus on.

@kotlarmilos kotlarmilos merged commit 92290ae into dotnet:feature/swift-bindings Dec 20, 2024
@kotlarmilos
Copy link
Copy Markdown
Member Author

Thanks for prompt review. If there is any additional feedback later, we will address it as a follow-up.

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