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

[Swift Binding] Projecting frozen structs #2797

Open
Tracked by #2822
jkurdek opened this issue Nov 21, 2024 · 7 comments
Open
Tracked by #2822

[Swift Binding] Projecting frozen structs #2797

jkurdek opened this issue Nov 21, 2024 · 7 comments
Assignees
Labels
area-SwiftBindings Swift bindings for .NET

Comments

@jkurdek
Copy link
Member

jkurdek commented Nov 21, 2024

Note

This was edited to contain correct code. The previous version of the code contained incorrect LLVM-IR (did not account for crossing the module boundary).

So far we have considered frozen structs to have known layout at compile time and thus we projected them into c# as structs. However not every frozen struct's layout will be know at compile time. (this also applies to generic types to #2796)

Lets consider the following Swift code:

//Module A
@frozen
public struct FrozenStruct {
    let x: Int
    let b: NonFrozenStruct
}

public struct NonFrozenStruct
{
    let a: Double
    let b : Double
}

// Module B
public func returnData(data: FrozenStruct) -> FrozenStruct {
    return data
}

FrozenStruct contains a field of type NonFrozenStruct. This makes its layout and size unknown at compile time.
Swift will pass this struct as if it was non-frozen.

Below we can see the llvm-ir for the returnData struct. Frozen struct will be passed usinga a pointer, and the function will utilise IndirectResult.

define hidden swiftcc void @"$s7ModuleB10returnData4data0A1A12FrozenStructVAF_tF"(ptr noalias sret(%swift.opaque) %0, ptr noalias %1) #0 {
entry:
  %data.debug = alloca ptr, align 8
  call void @llvm.memset.p0.i64(ptr align 8 %data.debug, i8 0, i64 8, i1 false)
  store ptr %1, ptr %data.debug, align 8
  %2 = call ptr @"$s7ModuleA12FrozenStructVWOc"(ptr %1, ptr %0)
  ret void
}

As the layout is unknown at compile time, the correct way to handle this on C# side would be to represent FrozenStruct as a class.
This means that we need to revisit the rules when we can project Swift struct to C# struct.

Following https://github.com/dotnet/designs/blob/main/proposed/swift-interop.md#structsvalue-types the rules to project Swift struct into c# struct will be:

  • Struct needs to be marked as frozen, every member of this struct has to be either trivial or marked as frozen (recursively)
  • Structs needs to be POD/Trivial / Bitwise Movable

\cc @kotlarmilos @stephen-hawley @amanasifkhalid @matouskozak

@kotlarmilos
Copy link
Member

Nice findings -- There are two ways to resolve this:

  • Project this as class
  • Retain struct semantics in the projections and let the runtime handle lowering

We need to decide which tradeoff to make. Implementing this in the projections would bring part of the ABI into the projections. Handling it at the runtime level would require an annotation for structs to indicate if it is frozen.

Let's try to align this with discussion in #2796 (comment).

/cc: @jkoritzinsky

@jkurdek
Copy link
Member Author

jkurdek commented Nov 22, 2024

@kotlarmilos I did show incorrect code earlier, I edited the issue - now it should be ok. I think now it is clear that the layout is not know until runtime

@kotlarmilos
Copy link
Member

Correct, I confirmed that NonFrozenStruct doesn't have layout in the ABI file.

Struct needs to be marked as frozen, every member of this struct has to be either trivial or marked as frozen (recursively)
Structs needs to be POD/Trivial / Bitwise Movable

I'm okay with these rules. Let's see how generics are resolved, and if they don't require runtime-level handling, I think we can proceed with this change.

@jkurdek
Copy link
Member Author

jkurdek commented Nov 22, 2024

Bringing the PR discussion in:


Let's treat representation and runtime lowering separately for a moment.

If a frozen struct doesn't require a copy constructor when moved in memory, I would still consider it a value type (struct). Additionally, as we discussed offline, the projection will flip if a generic used becomes frozen. From the user’s perspective, this can occur without their control, which is my primary concern when introducing this rule.

I understand that it doesn't match the lowering on the ABI level. However, I see this as implementation detail that belongs to the runtime. @amanasifkhalid I would like to hear your perspective.

Originally posted by @kotlarmilos in #2796 (comment)


I think the problem is that user-facing C# Pair is a class. In the specific case of Pair<FrozenStruct, FrozenStruct>, it could be a struct (and that's the wrapper introduced later) but we would need to have 2x C# Pair (class and a struct) to handle the case of NonFrozenStruct as well. @jkurdek please correct me if I'm wrong here.

Originally posted by @matouskozak in #2796 (comment)


If a frozen struct doesn't require a copy constructor when moved in memory, I would still consider it a value type (struct).

So for generics strictly. I think we cannot tell at compile time whether a generic will have or wont have to use a copy constructor as we have no idea what the generic parameter will be. Some instances of the generic, Pair<SomethingThatRequiresCopyConstructor, int> will require it. Some like Pair< FrozenStruct, FrozenStruct> will not.

@matouskozak I think you got it right. Pair<FrozenStruct, FrozenStruct> would ideally be represented as struct.

Additionally, as we discussed offline, the projection will flip if a generic used becomes frozen. From the user’s perspective, this can occur without their control, which is my primary concern when introducing this rule.

Making something non-frozen frozen is an ABI breaking change. I do not think we can do much about it. One way of solving this would be representing every struct as a class on C# side and abstracting the implementation details

Originally posted by @jkurdek in #2796 (comment)


One way of solving this would be representing every struct as a class on C# side and abstracting the implementation details

Do you mean every generic struct? In this case, the size and alignment of an object of opaque layout, as well as whether it is trivial or bitwise movable, is determined by querying its value witness table. If contains frozen structs, the helper can be used.

Originally posted by @kotlarmilos in #2796 (comment)


I meant all structs. Its more of stretch idea thing than actual proposal. I feel that projecting some as structs and some as classes creates a weird UX for developers who are not Swift professionals. I would prefer to have uniformity - e.g. all structs to be classes. I do understand however that this would require a lot custom runtime work and would bring its own set of problems. Anyway it is a different discussion.

Originally posted by @jkurdek in #2796 (comment)


I meant all structs. Its more of stretch idea thing than actual proposal. I feel that projecting some as structs and some as classes creates a weird UX for developers who are not Swift professionals.

It seems we need this layer of abstraction for structs with opaque layouts (those not known until runtime). In other cases, this abstraction isn’t necessary. We should prioritize simplicity, ensuring that users reading Swift docs know what to expect. If a struct can be projected directly, we should do so.

I do understand however that this would require a lot custom runtime work and would bring its own set of problems. Anyway it is a different discussion.

Let's do cost estimate for this.

/cc: @jkoritzinsky

Originally posted by @kotlarmilos in #2796 (comment)

@amanasifkhalid
Copy link
Member

I understand that it doesn't match the lowering on the ABI level. However, I see this as implementation detail that belongs to the runtime.

@jakobbotsch can give you a more informed perspective on CoreCLR's Swift lowering implementation.

@jkurdek
Copy link
Member Author

jkurdek commented Nov 25, 2024

It seems we need this layer of abstraction for structs with opaque layouts (those not known until runtime). In other cases, this abstraction isn’t necessary. We should prioritize simplicity, ensuring that users reading Swift docs know what to expect. If a struct can be projected directly, we should do so.

@kotlarmilos. I agree with the simplicity part. But I don't think I agree with the "know what to expect" part. I feel that we are currently flowing ABI details into public API. The developer might look into the Swift docs -- see two different @frozen structs and due to the rules described above (lets say one of the frozen structs contains a non-frozen struct member) find themselves with one class and one struct on c# side (and all the semantic differences that brings).

@kotlarmilos
Copy link
Member

The developer might look into the Swift docs -- see two different @Frozen structs and due to the rules described above (lets say one of the frozen structs contains a non-frozen struct member) find themselves with one class and one struct on c# side (and all the semantic differences that brings).

This is legitimate concern. Let's look at data then. I run a brief search over StoreKit, SwiftUI, and Foundation.

StoreKit: 40 structs / 20 enums / 0 classes
Frozen generic types: 2

@frozen public enum PriceIncreaseStatus
@frozen public enum VerificationResult<SignedType> 

SwiftUI: 691 structs / 42 enums / 13 classes
Frozen generic types: 36

@frozen internal struct LimitedAvailabilityTableColumnContent<TableRowValue, TableColumnSortComparator> : SwiftUI.TableColumnContent where TableRowValue : Swift.Identifiable, TableColumnSortComparator : Foundation.SortComparator {
@frozen public struct SequenceGesture<First, Second> : SwiftUICore.Gesture where First : SwiftUICore.Gesture, Second : SwiftUICore.Gesture {
@frozen public struct SubscriptionView<PublisherType, Content> : SwiftUICore.View where PublisherType : Combine.Publisher, Content : SwiftUICore.View, PublisherType.Failure == Swift.Never {
@propertyWrapper @frozen public struct GestureState<Value> : SwiftUICore.DynamicProperty {
@frozen public struct GestureStateGesture<Base, State> : SwiftUICore.Gesture where Base : SwiftUICore.Gesture {
@frozen public struct _DiscreteSymbolEffectModifier<T> : SwiftUICore.ViewModifier, SwiftUICore._GraphInputsModifier where T : Swift.Equatable {
@frozen public struct _TaskValueModifier<Value> : SwiftUICore.ViewModifier where Value : Swift.Equatable {
@frozen public struct _GeometryActionModifier<Value> where Value : Swift.Equatable {
@frozen public struct _GeometryActionModifier2<Value> where Value : Swift.Equatable {
@frozen public struct _ContentShapeKindModifier<ContentShape> : SwiftUICore.ViewModifier where ContentShape : SwiftUICore.Shape {
@frozen public struct _PreferenceActionModifier<Key> : SwiftUICore.ViewModifier where Key : SwiftUICore.PreferenceKey, Key.Value : Swift.Equatable {
@frozen @propertyWrapper public struct SceneStorage<Value> : SwiftUICore.DynamicProperty {
@frozen @propertyWrapper public struct AppStorage<Value> : SwiftUICore.DynamicProperty {
@frozen @_Concurrency.MainActor @preconcurrency internal struct AccessibilityTupleRotorContent<T> : SwiftUI.AccessibilityRotorContent {
@frozen @_Concurrency.MainActor @preconcurrency internal struct AccessibilityOptionalRotorContent<Content> where Content : SwiftUI.AccessibilityRotorContent {
@frozen @propertyWrapper public struct FocusState<Value> : SwiftUICore.DynamicProperty where Value : Swift.Hashable {
@frozen @_Concurrency.MainActor @preconcurrency internal struct _TupleTabContent<T, U> where T : Swift.Hashable {
@frozen public struct _AnchorWritingModifier<AnchorValue, Key> : SwiftUICore.ViewModifier where Key : SwiftUICore.PreferenceKey {
@frozen public struct ViewThatFits<Content> : SwiftUICore.View where Content : SwiftUICore.View {
@frozen public struct _CoordinateSpaceModifier<Name> : SwiftUICore.ViewModifier, Swift.Equatable where Name : Swift.Hashable {
@frozen public struct EquatableView<Content> : SwiftUICore.View where Content : Swift.Equatable, Content : SwiftUICore.View {
@frozen public struct _ScrollViewBoundsModifier<Result> : SwiftUICore.ViewModifier where Result : SwiftUICore.ViewModifier {
@frozen public struct _ScrollViewBoundsModifier2<Result> : SwiftUICore.ViewModifier where Result : SwiftUICore.ViewModifier {
@frozen public struct Grid<Content> where Content : SwiftUICore.View {
@frozen public struct GridRow<Content> where Content : SwiftUICore.View {
@frozen public struct _AnchorTransformModifier<AnchorValue, Key> : SwiftUICore.ViewModifier where Key : SwiftUICore.PreferenceKey {
@frozen public struct _ContentShapeModifier<ContentShape> : SwiftUICore.ViewModifier where ContentShape : SwiftUICore.Shape {
@frozen public struct _IdentifiedModifier<Identifier> : SwiftUICore.ViewModifier, Swift.Equatable where Identifier : Swift.Hashable {
@frozen public struct ScrollViewReader<Content> : SwiftUICore.View where Content : SwiftUICore.View {
@frozen public struct TupleTableColumnContent<RowValue, Sort, T> : SwiftUI.TableColumnContent where RowValue : Swift.Identifiable, Sort : Foundation.SortComparator {
@frozen public struct _ShadowView<Content> : SwiftUICore.View where Content : SwiftUICore.Shape {
@frozen public struct _IgnoredByLayoutEffect<Base> : SwiftUICore.GeometryEffect where Base : SwiftUICore.GeometryEffect {
@propertyWrapper @frozen public struct AccessibilityFocusState<Value> : SwiftUICore.DynamicProperty where Value : Swift.Hashable {
@frozen public struct TupleTableRowContent<Value, T> : SwiftUI.TableRowContent where Value : Swift.Identifiable {
@frozen public struct _ContainerValueWritingModifier<Value> {
@frozen @propertyWrapper @preconcurrency @_Concurrency.MainActor public struct FocusedObject<ObjectType> : SwiftUICore.DynamicProperty where ObjectType : Combine.ObservableObject {

Foundation: 269 structs / 100 enums / 7 classes
No frozen generic types

@jkurdek Could you try to estimate the number of non-frozen nested types? It appears that frozen generic types are a minority in both StoreKit and SwiftUI. I think we need more data before deciding to proceed with all-struct-to-class projections.

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

No branches or pull requests

3 participants