-
Notifications
You must be signed in to change notification settings - Fork 70
[Feature] Escaping closures #491
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
base: main
Are you sure you want to change the base?
Conversation
ARCHITECTURE.md
Outdated
| │ (.swift) │ ───▶ │ Representation │ ───▶ │ (.java, .swift) │ | ||
| └─────────────────┘ └─────────────────┘ └─────────────────┘ | ||
| Parse ImportedDecls Generator (JNI/FFM) | ||
| ``` |
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 need to explain wrap-java here too then I suppose. Its workflow is
The other direction, when starting from a Java library and wanting to access it from Swift is handled by the `wrap-java` command, which uses a classpath as its input:
┌──────────────────┐ ┌─────────────────┐ ┌───────────────────────┐
│ Java classpath │ │ Analysis │ │ Generated Code │
│ (.*class) │ ───▶ │ │ ───▶ │ (.swift with @macros) │
└──────────────────┘ └─────────────────┘ └───────────────────────┘
Parse ImportedDecls Generator (JNI/FFM)
ARCHITECTURE.md
Outdated
| let generator = FFMSwift2JavaGenerator(...) | ||
| try generator.generate() | ||
| } | ||
| } |
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.
I would be concerned having actual code snippets in this guide -- they WILL become outdated and confusing. How about we just explain the same with words please?
ARCHITECTURE.md
Outdated
| @@ -0,0 +1,355 @@ | |||
| # Swift-Java Architecture Guide | |||
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.
Can we please split out this md file addition from the rest of the PR? Many comments here that are should not affect the rest of the PR
CONTRIBUTING.md
Outdated
| ### Development Setup | ||
|
|
||
| ```bash | ||
| # Prerequisites: Java 22+ and Swift 6.0+ |
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.
| # Prerequisites: Java 22+ and Swift 6.0+ | |
| # Prerequisites: Java 25+ and Swift 6.2+ |
| let apiParams = functionType.parameters.map({ $0.parameter.renderParameter() }) | ||
|
|
||
| printer.print( | ||
| printer.print( |
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.
indent by mistake?
| | `@escaping` `Void` closures: `func callMe(_: @escaping () -> ())` | ❌ | ✅ | | ||
| | `@escaping` closures with primitive arguments/results: `func callMe(_: @escaping (String) -> (String))` | ❌ | ✅ | | ||
| | `@escaping` closures with custom arguments/results: `func callMe(_: @escaping (Obj) -> (Obj))` | ❌ | ❌ | | ||
| | Swift type extensions: `extension String { func uppercased() }` | 🟡 | 🟡 |ommit to support primitive escaping closures) |
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 isn't changing anything about extensions, why this change?
This is added outside the table?
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.
Apologies, a mistake when solving merge conflicts, thank you
| /// ``` | ||
| public final class JNIClosureContext: @unchecked Sendable { // @unchecked Sendable is needed because the globalRef is a jobject | ||
| /// The JNI GlobalRef to the Java closure object | ||
| public let globalRef: jobject |
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 just the existing JavaObjectHolder, no need to introduce a new type
| """ | ||
| public static class setCallback { | ||
| @FunctionalInterface | ||
| public interface callback extends AutoCloseable { |
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 illegal Java code and would not compile, a functional interface cannot have multiple abstract methods
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 seems the generated code is more correct actually, the test isn't passing and you need to remove those auto closable ideas from here
| printer.print("fatalError(\"Failed to get JNI environment for escaping closure call\")") | ||
| printer.outdent() | ||
| printer.print("}") | ||
| printer.print("") |
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.
Generally we do a big print with """ strings rather than this style when possible
4f99c21 to
98efb49
Compare
98efb49 to
7f81ac0
Compare
This PR introduces support for escaping closures with primitive types only.