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

Unit of code: Swift backend #1042

Merged
merged 12 commits into from
Sep 2, 2021
Merged

Unit of code: Swift backend #1042

merged 12 commits into from
Sep 2, 2021

Conversation

jhugman
Copy link
Contributor

@jhugman jhugman commented Aug 25, 2021

This PR builds on #993.

It completely replaces the swift backend with code copy/paste/adapted from the kotlin backend and the existing swift templates.

It doesn't implement the Wrapped or External types. Wrapped types look fairly simple from here, but I'm still unsure how External types are going to work.

It does provide a file for Callback Interfaces, including a place to add initialization code. I think this completes the Rust component for #353 /cc @patrick-fitzgerald.

There are a few differences between the Kotlin and Swift backends, down to the way the Uniffi's Swift runtime works— itself a consequence of being able to implement protocols on T?:

  • the Rust filter methods write and canonical_name are never used. I think this should work, as long as we can be sure that none of the ViaFfi swift methods can ever collide with the client code.
  • Map, Sequence and Option ViaFfi extensions are still in RustBufferHelper.swift. I don't think they need to be optionally declared.

@jhugman jhugman changed the base branch from main to unit-of-code August 25, 2021 17:42
@jhugman jhugman requested review from rfk, tarikeshaq and mhammond August 25, 2021 20:28
Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

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

I like the changes overall. I personally would rather the large blocks of code get put in their own template file (need that syntax highlighting 😆), but that's not such a big deal. I'll also note that there's a lot of changes here and I think it's hard for anyone to review all the details. I personally think that if the tests pass then we should be good, but I wonder if others agree.

.collect();

imports.sort();
imports
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of adding more to this impl. For the wrapper code, I ended up putting similar methods in the CI, but that didn't feel right.

"// Helper code for {} enum is found in EnumTemplate.swift",
self.type_label(oracle)
))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

When I read this, it makes me want to implement this commit on all the bindings. That would eliminate the lower, write, lift, read, and helper_code, methods.

That said I still think this change is positive. Even if the only methods are type_label, literal, and canonical_name, it's still nice to have them grouped together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just add this here, for want of a better place to put it: for Swift and Rust you can add traits to structs (and classes).

Rust has rules around owning either the trait or the struct (the orphan rule), but Swift does not. There are rules around visibility of the extensions, but Swift lets you add extensions to classes, structs, and protocols in place. Thus:

let obj = 1
let isInt = obj is Int // true

protocol Labelled {
    var label: String { get }
}
extension Int : Labelled {
    var label: String { "Int" }
}

let isLabelled = obj is Labelled // true
let label = obj.label // "Int"

which means we can write fairly simple extensions for structural types:

extension Optional : Labelled where Wrapped: Labelled {
    var label: String {
        guard let inner = self?.label else {
            return "nil"
        }
        return "\(inner)?"
    }
}

let obj: Int? = 1
let isLabelled = obj is Labelled // true
let label = obj.label // "Int?"

This is why everything can call the write method without going through the write_swift filter, it's all fairly homogenous. There are reasons why this isn't great; but on the absenting these read/write/lift/lower probably don't need to go through a CodeType.

For Kotlin, on the other hand, you can add extension methods to classes and interfaces, but they're syntactic sugar for static functions. You cannot add an extension interface to an existing class.

interface Labelled {
    fun label(): String
}

fun Int.label() = "Int"

val obj: Int = 1
val isLabelled = obj is Labelled // false
val label = obj.label() // "Int"

Conceivably, we could make sure every type has the right methods (in this case label), but then when it came to writing structural types we can't use:

fun <T: Labelled> T?.label() = this?.label ?: "null"

val obj: Int? = 2
val label = obj.label() // compile error!

because none of the types the user is using is implementing the Labelled protocol, even though it implements everything it needs to.

This of course, sucks and means that we need some logic around calling of the lift/lower/read/write methods for at least the Kotlin backend, and other languages that don't have extension protocols or traits.

If we now have to have that logic for Kotlin, why do we keep with it for Swift or any other language:

  1. uniformity/regularity. We want to be able to have the backends to all look and work approximately the same. The code it generates may not be the same, but if each backend is its own special snowflake, then we hinder our development velocity.
  2. visibility rules. These are different in each language, and I think we should try and stay out of the way of client code as much as possible. Any runtime code that we add should not be visible to the the client code. For Swift, I'm not sure if ViaFfi is visible to the client code or not (explicitly you have to make it public, even if all the methods are private).
  3. naming collisions. We do no explicit checking on this right now: uniffi will generate invalid code in arbitrary ways if the UDL code uses methods of the same names as used by the lift/lower/read/write/coerce machinery of a type. There are ways around it, but splitting off that machinery (e.g. in ourCallbackInterface kotlin backend we use CallbackInterfaceInternals) from the type itself.

This comment should probably go someplace more durable that a PR review comment. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a PR coming along these lines. I think the solution for Kotlin is the same solution for Rust external types, we separate the FFIConverter type from the type that it's converting. So you define objects like this:

object FFIConverterOptionalTypeFoo: FFIConverter<Foo?, RustBuffer> {
  fun lift(RustBuffer): Foo? { ... }
  fun lower(Foo?): RustBuffer { ... }
}

Then you add some logic to the template code so that it can map each type to the FFIConverter instance and you're there.

I think something like this is needed if you're going allow users to wrap types. They need some way to hook in to the existing lift/lower logic and if that only exists in the template code, then it's going to be hard.

Copy link
Contributor

Choose a reason for hiding this comment

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

The visibility question is interesting. It feels like the FFIConverter code should be private, but then importing types becomes hard -- I think you would need to duplicate the FFIConverter code in the library that's importing, which feels worse to me. I feel like the middleground is to make them public, but give them a prefix that makes it obvious that it's part of the UniFFI internals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm definitely on board for converter objects implementing a FFIConverter<Foo?, RustBuffer>, for both Kotlin and Swift.

#[allow(unused_imports)]
use super::filters;

macro_rules! impl_code_type_for_miscellany {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a docstring here about the usage so that people don't need to go through the macro code to understand what's going on.

Copy link
Contributor

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

Did a quick review, doesn't look too different from the Kotlin stuff! LGTM!

(I don't think any of my comments here are specific to swift vs kotlin, so feel free to ignore them)

.filter_map(|type_| oracle.find(&type_).import_code(oracle))
.flatten(),
)
.collect::<HashSet<String>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: any reason we first collect into a set and then into the Vec?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the best features! If multiple components require the same import, we'll only generate 1 import statement.

Super Super nit: Maybe collect into BTreeSet to de-dupe and sort at the same type.

use crate::interface::{ComponentInterface, Object};
use askama::Template;

use super::filters;
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, I'm curious why clippy is okay without the allow_unused here 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should imagine its been used by the Template macro below.

@jhugman jhugman force-pushed the unit-of-code branch 3 times, most recently from 2fddefe to 72407b1 Compare September 2, 2021 16:12
Base automatically changed from unit-of-code to main September 2, 2021 16:19
saks pushed a commit to saks/uniffi-rs that referenced this pull request Oct 8, 2021
* Swift backend written in rust, builds with cargo build

* Generated swift bindings compile and all tests pass

* Tidy read and writes and compounds

* Tidy up miscellaneous and object types

* Tidy up primitive types templates

* Tidy up unimplemented callback interfaces

* Tidy up enums

* Remove dead code

* cargo fmt

* Tidy error types

* Rebase after landing kotlin PR

* Make RustBufferHelper.swift like RustBufferHelper.kt
badboy added a commit to badboy/uniffi-rs that referenced this pull request Oct 26, 2021
Now each type lives in its own file, with its own template (more or less)
and each type knows how to render itself.

This follows mozilla#993 (UoC for Kotlin) and mozilla#1042 (UoC for Swift).

Fixes mozilla#1072
badboy added a commit to badboy/uniffi-rs that referenced this pull request Oct 26, 2021
Now each type lives in its own file, with its own template (more or less)
and each type knows how to render itself.

This follows mozilla#993 (UoC for Kotlin) and mozilla#1042 (UoC for Swift).

Fixes mozilla#1072
badboy added a commit to badboy/uniffi-rs that referenced this pull request Oct 27, 2021
Now each type lives in its own file, with its own template (more or less)
and each type knows how to render itself.

This follows mozilla#993 (UoC for Kotlin) and mozilla#1042 (UoC for Swift).

Fixes mozilla#1072
badboy added a commit to badboy/uniffi-rs that referenced this pull request Oct 27, 2021
Now each type lives in its own file, with its own template (more or less)
and each type knows how to render itself.

This follows mozilla#993 (UoC for Kotlin) and mozilla#1042 (UoC for Swift).

Fixes mozilla#1072
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants