Skip to content

[interop][SwiftToCxx] add support for exposing non-isolated actor declarations #61882

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

Merged
merged 4 commits into from
Nov 3, 2022

Conversation

hyp
Copy link
Contributor

@hyp hyp commented Nov 2, 2022

This exposes Swift actor classes to C++. Only non-isolated declarations get bridged to C++ though.

@hyp hyp added the c++ interop Feature: Interoperability with C++ label Nov 2, 2022
@hyp hyp requested a review from ktoso November 2, 2022 19:20
@hyp
Copy link
Contributor Author

hyp commented Nov 2, 2022

@swift-ci please test

@hyp
Copy link
Contributor Author

hyp commented Nov 2, 2022

@swift-ci please test source compatibility

@hyp
Copy link
Contributor Author

hyp commented Nov 2, 2022

@swift-ci please test

@hyp
Copy link
Contributor Author

hyp commented Nov 2, 2022

@swift-ci please test source compatibility

1 similar comment
@hyp
Copy link
Contributor Author

hyp commented Nov 2, 2022

@swift-ci please test source compatibility

@hyp
Copy link
Contributor Author

hyp commented Nov 2, 2022

@swift-ci please test

@hyp
Copy link
Contributor Author

hyp commented Nov 2, 2022

@swift-ci please test source compatibility


public nonisolated func method() {
print("nonisolated method")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you'd also want to add a

public nonisolated func methodAsync() async 

and confirm it does not import; though I guess this perhaps is already handled -- it's the same as any normal async function after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

async functions are not bridged already, we cover that. But I can add as a follow-up!

distributed public func dist() {}

@_expose(Cxx) // ok
public init() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public init() {
public init(actorSystem: LocalTestingDistributedActorSystem) {
self.actorSystem = actorSystem

is what you'd need to do to make sure this is a 100% correct actor; this should have failed to compile tbh as it is declared now - but the other errors probably prevented it from going into those checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll commit this as a follow-up!

}

@_expose(Cxx)
public distributed actor DistributedActorClass {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hooray! Semantics look good 👍

return {Unsupported, UnrepresentableActorClass};
}
if (getActorIsolation(const_cast<ValueDecl *>(VD)).isActorIsolated())
return {Unsupported, UnrepresentableIsolatedInActor};
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome, glad that this indeed covered all the bases :-)

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM! Might want to add a test with nonisolated + async method too though

@hyp
Copy link
Contributor Author

hyp commented Nov 3, 2022

@swift-ci please test macOS platform

1 similar comment
@hyp
Copy link
Contributor Author

hyp commented Nov 3, 2022

@swift-ci please test macOS platform

@hyp
Copy link
Contributor Author

hyp commented Nov 3, 2022

source compatibility suite failures are unrelated, will merge:

Failures:
  FAIL: CoreStore, 4.2, 286360, CoreStore watchOS, generic/platform=watchOS
  FAIL: CoreStore, 4.2, 286360, CoreStore tvOS, generic/platform=tvOS
  FAIL: CoreStore, 4.2, 286360, CoreStore OSX, generic/platform=macOS
  FAIL: CoreStore, 4.2, 286360, CoreStore iOS, generic/platform=iOS
  FAIL: CoreStore, 4.0, 83e608, CoreStore watchOS, generic/platform=watchOS
  FAIL: CoreStore, 4.0, 83e608, CoreStore tvOS, generic/platform=tvOS
  FAIL: CoreStore, 4.0, 83e608, CoreStore OSX, generic/platform=macOS
  FAIL: CoreStore, 4.0, 83e608, CoreStore iOS, generic/platform=iOS
========================================

@hyp hyp merged commit 56665d5 into swiftlang:main Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants