[Swift] Swift implementation 🎉🎉

[mustiikhalil]

Good afternoon,

@mzaks and I hope with this PR we would be able to add an official implementation for swift, it's heavily inspired by the C++ and C# implementations. There are some elements that are still missing such as the code generator and also documentation for the code base, however I found that we can implement those after getting an initial review for the swift code base, and if it lives up to the standards.

The FlatBuffer class uses apples underlaying UnsafeMutableRawPointer which allows is to write directly to the memory, and lefts a lot of heavy weight from our shoulders, since this implementation comes which the following function storeBytes with takes a value and it's Bytes representation and just adds it to the buffer. UnsafeMutableRawPointer the name is a bit misleading since it's actually safe to use if everything writing the bytes is well structured. and since we don't really allow the user to get to the underlying adding and pushing to the buffer and only allowing them to do so through the builder it would be completely safe to use it.

The FlatBuffersBuilder class uses the same underlying logic as C++ and C#, but unlike C# it relies on generics to actually add bytes and elements into the buffer. we took a different approach regarding adding the structs into the buffer, since swift uses the storeBytes to actually store the struct into the buffer, and since FlatBuffers do not allow string in Structs we took that into our advantage.

struct Vec2: Writeable {
    var _x: Float32
    var _y: Float32
    var _z: Float32
    var _c: Color2

     init(x: Float32, y: Float32, z: Float32, color: Color2) { _c = color; _x = x; _y = y; _z = z }

     static func createVec2(_ bb: FlatBuffersBuilder, v: Vec2) -> Offset<UOffset> {
        return bb.create(struct: v)
    }
}

the following struct will be able to be directly inserted into the buffer, and we will be implementing a reader struct that will read the Writable struct as shown below. This will have the same concept as the builder Objects in c++. The readers were implemented to follow the C# way of fetching elements from the buffer.

struct Vec2_Read: Readable {
    private var __p: Struct
    init(_ fb: FlatBuffer, o: Int32) { __p = Struct(bb: fb, position: o) }
    var c: Color2 { return Color2(rawValue: __p.readBuffer(of: Int32.self, at: 12)) ?? .red }
    var x: Float32 { return __p.readBuffer(of: Float32.self, at: 0)}
    var y: Float32 { return __p.readBuffer(of: Float32.self, at: 4)}
    var z: Float32 { return __p.readBuffer(of: Float32.self, at: 8)}
}

All the test case are passing, and they were created by the flatc code generator for both C++ and C#, and the basic implementation of swift was verified against those two since the code generator isn't implemented yet.

what's missing:
1- Code Generator
2- Documentation
3- Benchmarking

when merging this PR we would be closing the following issues, Closes #5504. and I will be rebasing the commit before merging for sure

[dbaileychess]

Can you add some tests that read the tests/monsterdata_test.mon to ensure that data written by another language is correctly read by Swift?

[mustiikhalil]

Definitely would do that, I just want to know if the monsterdata_test.mon data is mapped to the following monster_test.fbs? Since I need to know what are the actual objects with the buffer.

[dbaileychess]

Yep, monster_test.fbs is the schema and monsterdata_test.json are the values that are used. You can also follow the other tests files for C#, Java, etc.. to see how they use the monster_test.mon file to test.

[aardappel]

Thanks, this is exciting! Some quick thoughts from your readme, and then an initial code review will follow:

• Looking at the docs this is not clear, but I am guessing that UnsafeMutableRawPointer when it reads a 16/32/64-bit quantity, it will do so with machine-endianness ordering, which means it won't work correctly on big endian machines. This means you should either have explicit big-endian versions of most of these functions, or (and this is entirely acceptable) explicitly disable this code to run on big-endian.
• As for splitting a struct into 2 versions for reading and writing: this sounds acceptable to me, except I'd give the version for reading the shorter name, as it may be used more often. Or rather, reading code is more important to be "pretty" or native looking.
• There's no code generator? Meaning all current code was hard-coded for the test? That is useful for an initial look, but that makes it far off being a complete PR.

[aardappel]

This generally looks great! It indeed looks like it mirrors the C++ (and C#) code closely, and I have very little to complain about. This looks like it would result in a pretty efficient Swift implementation, though I guess that remains to be tested.

So biggest thing right now would be an actual code generator to see how that would affect things.

[aardappel]

Ideally we don't store IDE data in repo, not sure how unavoidable that is in the case of Swift. Does this code build from the command-line without XCode files?

[mustiikhalil]

I believe we can remove that indeed, however we will need a way to make SPM work since it would be a nested file. since it would be looking for the Package.swift, so I think we might need to make it a git submodule

[aardappel]

We currently don't have any submodules for this repo, so would be good to avoid starting that now :)

[aardappel]

This seems a new test case specific to Swift? Important that once we have codegen, that it uses the existing tests.

[mustiikhalil]

Yes, all the test cases were writing manually for swift since we don't have a codegen. however they were all written and compared with both cpp and C#. Since you can see that all the test cases are compared by Bytes to an array

[aardappel]

so? can these tests be removed now then? Or do you think they provide useful extra coverage?

[mustiikhalil]

I think it's nice to have them for extra coverage.

[aardappel]

I assume in Swift this T.Type argument is fully static and does not involve any runtime value?

[mustiikhalil]

• A metatype type refers to the type of any type, including class types, structure types, enumeration types, and protocol types. https://docs.swift.org/swift-book/ReferenceManual/Types.html

This would request the codegen to specify the type of the readable object before reading it. As in __p.readBuffer(of: Double.self, offset: 10) since Double confirms to the type Scalar, however I am not sure if it would be fully static or not.

[mustiikhalil]

On a side note, this is how Swift and Apple have implemented most of their methods, and it can be seen all over swift. so I think it would be pretty safe to use it when using the code generator, since users would be advised against changing it.

[mustiikhalil]

Just found a talk from apple that might actually help out https://developer.apple.com/videos/play/wwdc2016/416/

[mustiikhalil]

• Looking at the docs this is not clear, but I am guessing that UnsafeMutableRawPointer when it reads a 16/32/64-bit quantity, it will do so with machine-endianness ordering, which means it won't work correctly on big endian machines. This means you should either have explicit big-endian versions of most of these functions, or (and this is entirely acceptable) explicitly disable this code to run on big-endian.

Currently, indeed the code is disabled from running on Big-endian machines. In hopes that we would be implementing it whenever we get to have an issue regarding it.

[aardappel]

That's fine.

[mustiikhalil]

• As for splitting a struct into 2 versions for reading and writing: this sounds acceptable to me, except I'd give the version for reading the shorter name, as it may be used more often. Or rather, reading code is more important to be "pretty" or native looking.

The struct names can be modified, it won't be a problem, when we get to have the code generator. we can make the codegen create them as follows

struct Vec3: Readable {}
struct Vec3Writer: Writeable {}

• There's no code generator? Meaning all current code was hard-coded for the test? That is useful for an initial look, but that makes it far off being a complete PR.

Yes, all the test cases were hard coded for the current tests, we wanted to get your opinion regarding the current implementation before going any further than what we currently have.

[mustiikhalil]

@aardappel, @mzaks and I were implementing the test cases for the swift, that adhere to the test cases that the FlatBuffers repository has. However, we ran into an issue: as explained in following issue. the short explanation would be the current implementation fo the swift doesn't allow us to next objects into structs, or have different types which is solved easily using the following:

withUnsafeBytes(of: &vec) { (buffer)  in
    _memory.advanced(by: writerIndex - len).copyMemory(from: buffer.baseAddress!, byteCount: buffer.count)
}

however as explained in the issue this will lead into a none zero padding, would that be an issue?
example:

struct Test { a: short; b: byte }
struct Vec3 (force_align: 8) {
  x:float;
  y:float;
  z:float;
  test1:double;
  test2:Color;
  test3:Test;
}

would result into the following Binary representation for cpp and c#:
[0, 0, 128, 63, 0, 0, 0, 64, 0, 0, 64, 64, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 8, 64, 1, 0, 5, 0, 6, 0, 0, 0]

however since we are writing and reading from memory in swift for structs this might lead to something like this, where the padding between the floats and doubles would have 0, 0, 4, 0 some unneeded data
[0, 0, 128, 63, 0, 0, 0, 64, 0, 0, 64, 64, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 8, 64, 1, 0, 5, 0, 6, 0, 0, 0]

[aardappel]

@mustiikhalil you're saying Swift's padding can generate garbage data for padding?

While I don't believe there is currently any code that expects padding to be 0, that is what all other languages write out, so Swift generating non-zero padding could cause problems for any code expecting binary equivalence of some sort. I'd say writing it in a way that guarantees zeroes is pretty important.

Having garbage there also makes output non-deterministic, and it makes compression of serialized data worse.

[mustiikhalil]

yeah it makes sense, it's just in the structs that we face this issue, we can find other ways to implement it and fix it

[mustiikhalil]

@aardappel following up on yesterdays issue, I managed to implement the following fix for the structs, however I am not sure if you would approve it

struct Vec3Writer: Writeable {
    
    var memory: UnsafeMutableRawPointer
    static var size = 32
    static var alignment = 8

    init(x: Float32, y: Float32, z: Float32, test1: Double, color: Color_1, test: Test_1) {
    /// creates a memory that would store the struct in itself so it would be passed on later 
        memory = UnsafeMutableRawPointer.allocate(byteCount: Vec3Builder.size, alignment: Vec3Builder.alignment)
        memory.initializeMemory(as: UInt8.self, repeating: 0, count: Vec3Builder.size)
        memory.storeBytes(of: x, toByteOffset: 0, as: Float32.self)
        memory.storeBytes(of: y, toByteOffset: 4, as: Float32.self)
        memory.storeBytes(of: z, toByteOffset: 8, as: Float32.self)
        memory.storeBytes(of: test1, toByteOffset: 16, as: Double.self)
        memory.storeBytes(of: color.rawValue, toByteOffset: 24, as: Int8.self)
        memory.storeBytes(of: 0, toByteOffset: 25, as: Int8.self)
        memory.storeBytes(of: test.a, toByteOffset: 26, as: Int16.self)
        memory.storeBytes(of: test.b, toByteOffset: 28, as: Int8.self)
    }
}

///we create the struct as follows and then pass it to create struct
let v = Vec3Writer(x: 1, y: 2, z: 3, test1: 3, color: .green, test: Test_1(a: 5, b: 6))
fbb.create(struct: v)
/// this is the internal call for copying the memory of the current implemented struct

buffer.copyMemory(v.memory, count: size)

also thought of passing the memory only to the FlatBufferBuilder but that would make it hard for us to follow with the size and the alignment of everything since we really depend on it

[mzaks]

@mustiikhalil you're saying Swift's padding can generate garbage data for padding?

While I don't believe there is currently any code that expects padding to be 0, that is what all other languages write out, so Swift generating non-zero padding could cause problems for any code expecting binary equivalence of some sort. I'd say writing it in a way that guarantees zeroes is pretty important.

Having garbage there also makes output non-deterministic, and it makes compression of serialized data worse.

I was also surprised to see non 0 padding, but it is how it is.
The alternative is, what @mustiikhalil described in his latest reply - storing data value by value. It has it's benefits actually, as it is easier to provide BigEndian support. We are not dependent on Swift struct layout, which theoretically might change and be different from the one in FlatBuffers. And we can support custom FlatBuffers attributes on struct definitions, such as force_align.
The downside is that the generated code will become more complex and the performance of struct serialisation and probably deserialisation will be slower as we are not just doing a single memory copy.

Regarding deserialisation. If we are doing a value by value serialisation than it makes sense to have value by value deserialisation as otherwise we might hit problem (the byte array representing a struct being not compatible with Swift struct representation).

[aardappel]

Yes, most languages can't guarantee that their memory layout is the same as FlatBuffers, so should typically be done one by one. In C++ we have so macro trickery to check at compile time that the layout is the same, and so far we haven't found a compiler for which that fails, but if we did, we'd need per-field serialization even in C++.

As for Vec3Writer, I don't understand why this has to be struct, why not simply a free-standing function that gets a FlatBufferBuilder argument along with all the rest?

[mustiikhalil]

@aardappel we created it as a struct so that we would be able to insert structs as a vector, which would have the alignment of the memory and also the size of the memory stripe. since we don't want to have variables that are laying around,

static var size = 32
static var alignment = 8

that's why we implemented it with a struct to contain the values within the data structure

but other than the struct, does the implementation of the memory and sending it to the FlatBufferBuilder?

so technically this is how it would look like:

func createVec3(x: Float32, y: Float32, z: Float32, test1: Double, color: Color_1, testA: Int16, testB: Int8) -> UnsafeMutableRawPointer {
    let memory = UnsafeMutableRawPointer.allocate(byteCount: Vec3.size, alignment: Vec3.alignment)
    memory.initializeMemory(as: UInt8.self, repeating: 0, count: Vec3.size)
    memory.storeBytes(of: x, toByteOffset: 0, as: Float32.self)
    memory.storeBytes(of: y, toByteOffset: 4, as: Float32.self)
    memory.storeBytes(of: z, toByteOffset: 8, as: Float32.self)
    memory.storeBytes(of: test1, toByteOffset: 16, as: Double.self)
    memory.storeBytes(of: color.rawValue, toByteOffset: 24, as: Int8.self)
    memory.storeBytes(of: 0, toByteOffset: 25, as: Int8.self)
    memory.storeBytes(of: testA, toByteOffset: 26, as: Int16.self)
    memory.storeBytes(of: testB, toByteOffset: 28, as: Int8.self)
    return memory
}

struct Vec3: Readable {
    static var size = 32
    static var alignment = 8
    
    init(_ bb: FlatBuffer, o: Int32) {}
    /// reader code comes here
}

// and would be used as follows
fbb.create(struct: createVec3(x: 1, y: 2, z: 3, test1: 3, color: .green, testA: 5, testB: 6), type: Vec3.self)

[mustiikhalil]

@aardappel currently @mzaks and I have almost finished everything regarding the swift implementation. we've implemented all the required APIs and also convenience APIs that would allow the codegen to implement the functions accordingly and without worrying about the index and positions of stuff.
The Table implementation has directRead with ignores the position of an object, and also a read that respects it. that's all to minimize the codeGen work.

@dbaileychess as you requested earlier when we opened the PR the implementation of the test cases that will be reading from different languages, that is done using the .mon file you referenced earlier. currently we can write/read the object Monster. With our current exposed and implemented APIs. (all of this was done referencing the CSharp code) you can check the following file FlatBuffersMonsterWriterTests.swift

what's missing:

• Documentation
• Some functions in the table/struct implementation, including the following function implementation TestarrayoftablesByKey
• codegen

[aardappel]

@mustiikhalil does this have codegen now?

[mustiikhalil]

@aardappel i just want a final review for the current swift APIs. so we start building the code gen.

[aardappel]

@mustiikhalil reviewing a PR this size is a lot of work, so I'd prefer doing that again once you feel you've done everything you can.

Besides, API changes are probably even easier once you have code generation, and it is easier to see for me how it compares to the other languages when it is generating code for the exact same schemas and the same tests.

[mustiikhalil]

@aardappel and @mzaks I have a couple of questions regarding the code gen and would love to get your input regarding the matter. I've built like 90% of the code generator, however I am not sure how to handle the namespaces? we don't want people to keep typing MyGame.Example.Monster each time they are gonna create a Monster object. and if we remove namespaces from the swift implementation, how do we call the new objects in swift? Monster, and Monster1

what we are missing currently is the following in the code gen:
1- generating the comments in the fbs file
2- a create object function for the tables
3- a lookup function for the keys in the buffer

do we need to implement the function to create an object in swift? CreateMonster(fbb: ...)

[mzaks]

... how to handle the namespaces? we don't want people to keep typing MyGame.Example.Monster each time they are gonna create a Monster object ...

Namespaces are a bit of a problem as Swift doesn't really have the concept. In Swift you either use Modules (but you can't have nested modules), or people also wrap definitions in empty enums, in order to mimic a name space. e.g.:

public enum MyGame {
  public enum Example {
    public struct Monster {
      ...
    }
  }
}

In order to avoid repeating your self with MyGame.Example.Monster you users can define type aliases:

typealias Monster = MyGame.Example.Monster

Which is not ideal, but in case that your schema makes use of namespacing and it is possible that Swift is not the only language using this schema, I am afraid this is the only way to make thing correct.

do we need to implement the function to create an object in swift? CreateMonster(fbb: ...)

Not sure I understand the question.

[mustiikhalil]

do we need to implement the function to create an object in swift? CreateMonster(fbb: ...)

Not sure I understand the question.

There are some tables that would have a constructor that will hold all the values in place example:

static func createMonster(builder: inout FlatBuffersBuilder,
                                         position: Offset<UOffset>,
                                         hp: Int16,
                                         name: Offset<String>,
                                         inventory: Offset<UOffset>,
                                         color: Color3,
                                         weapons: Offset<UOffset>,
                                         equipment: Equipment = .none,
                                         equippedOffset: Offset<Weapon>,
                                         path: Offset<UOffset>) -> Offset<Monster> {
...
}

are we required to have this in the initial release?

@mzaks and if it's possible can you take a look at the code Gen? just so we know that it's good to go, since what's left is the lookup by key code, in swift and cpp

[aardappel]

Yes, use of namespaces should mimic the other languages, since we're not changing the schemas for Swift.

To what extend is this enum trick standard? Should we use it? Since this is generated code, some verbosity is ok, so simply using full names everywhere should be ok.

For the user, if there is no way to do "using namespace" in Swift, they are going to have to deal with the long names. We can't generate typealiases for them, since they may clash.

Having the 1 call createMonster is certainly nice to have, but not required.

[mustiikhalil]

It's either we use Enums or structs for that and as @mzaks already suggested I think the enums will do the job, since apple does use it to nest classes in their Combine Framework. so I think it's doable. The typealiases will be made by the users, since as you mentioned it's kinda impossible to generate ones that will not clash.

[mzaks]

@mustiikhalil sorry for being so unresponsive. Will try to go through the generated code this week.

[mustiikhalil]

@aardappel and @mzaks I just finished the implementation for the code Gen, and I do apologize in advance if the cpp code isn't as you expect it to be, the code Gen generates the swift file by --swift, and just like other languages, the mutable will only be generated if we use --gen-mutable. Since Xcode requires the user to drop the code to it's navigator is I just simply added a file called monster_test_generated.swift that hosts all the generated code.
The code is already tested on a linux system that's running swift, and that's why we have the

#if os(Linux)
import CoreFoundation
#else
import Foundation
#endif

however the test cases, that are running on this PR are still failing, I am not sure why though

[aardappel]

@mustiikhalil to make the C++ code more conform, first step is to run clang-format on your generator (see also src/clang-format-git.sh), then compare your code with that of the other generators (in particular, the C++ one).

And yes, would be good to have the tests pass first :)

[mustiikhalil]

@aardappel and @mzaks I think this PR is finalized now, whenever @mzaks has time to review the swift code that would be amazing. I've already tested it on linux, macOS and iOS. I've fixed all the memory issues that we were facing. I've used Xcode's the following to debug the entire project:
1- Malloc Stack and fixed all the allocation issues that we faced
2- Address Sanitizer that makes sure that we aren't writing or reading from memory that doesn't belong the buffers.
all of them passed.

[mzaks]

Generally thing already looks quite great. I have another few things, which are not blockers, but seem strange to me and could be addressed now or maybe in sub sequential releases:

1. In union_vector_generated.swift we have following API public func mutate(mainCharacterType: Character) which implies that we can mutate the type of the character inline. IMHO this is problematic as the actual value can't be mutated and the layout of tables included in the union can be very different. e.g. Character.Mulan is of type Attacker and Character.Other is of type string meaning changing just the type of a union can do more harm than good in my opinion.
2. Same file, same class, we have public var charactersTypeCount: Int32 and public var charactersCount: Int32 which is not bad it is just redundant as both should return the same value.
3. Same file, same class, public static func add(charactersType: Offset<UOffset>, _ fbb: FlatBufferBuilder) just a small thing, I think it make sense to name the first parameter characterTypes as other wise it feels like it should be same type for multiple characters, where it is actually different types per character. Maybe a more descriptive name would be even better, as we the type does not directly communicate that we expect an offset to a vector.
4. In moster_test_generated.swift I see that we support the (key) annotation, which is awesome, but the API public static func lookupByKey(vector: Int32, key: UInt64, fbb: ByteBuffer) -> Referrable? and other lookupByKey static methods should probably be of visibility fileprivate or at least internal as I don't think end users will be able to do something useful with this API.

All in all great job @mustiikhalil!

[mzaks]

One more thing which could make Swift support better and probably could benefit also other languages as well. Swift package manager works best if the git repos root contains the Package.swift file. In our case the Package.swift feel is in the swift folder, so we can't point SPM directly to the GitHub repo. There is a solution by using git subtree https://www.atlassian.com/git/tutorials/git-subtree. I stumbled upon this solution just recently myself by reading following blog post (https://neogeek.dev/creating-custom-packages-for-unity-2018.3/).

[mustiikhalil]

@mzaks so regarding the package.swift, we did have this conversation with @aardappel and he requested it to be within the directory swift since the other option was a git submodule. However if the git subtree would be helpful we should look into it.
1- You are absolutely right! Completely forgot about that one, I will be removing all the unions from the mutations.
2- All the languages actually produce it all the time, so I think we should also keep it that way too.
3-
3.1 the actual fbs file has the name of the element as characters and when we generate the field in other languages it's always handled like this CharactersType which is similar to what we have in swift.
3.2: what do you think of the following then addVectorOf(charactersType:)?
4- that's a great idea, we should make it fileprivatesince we don't want the user to really use it

[aardappel]

@mzaks would prefer to keep package file in subdir if at all possible.

[mustiikhalil]

@aardappel, @mzaks fixed all the issues you mentioned yesterday:
1- Removed the union mutation.
2- Renamed the vector functions to addVectorOf(elements:), however the names of the elements is the same as all the other languages. Unless you would prefer having a TypeAliases of Offset<UOffset> for vectors to VectorOffset
3- fileprivate for the lookup functions.

However, I didn't change the count for the unions arrays since all the languages do the same thing, so I kept that to adhere to the other APIs. and didn't touch the package.swift

[mzaks]

@aardappel as far as I can tell in order to make subtree work, we need something like following command:

git subtree push --prefix swift origin swift

To create a new branch swift, which contains only the code from swift folder. This branch can be used for releases of Swift Library and also as a URL for Swift Package Manager. We would need to enhance the build and release pipeline though. But then we could use the same technique for Rust, Dart, etc... And move package.json and pom.xml into subfolders.

[mzaks]

@mustiikhalil one more small thing, you probably what to cd back here:
https://github.com/mustiikhalil/flatbuffers/blob/swift-implementation/tests/TestAll.sh#L64
Now it is not a problem as swift is the last test but it will be bad for people who want to add another case.

[mustiikhalil]

@mzaks that's done :D so hopefully the PR looks clean now! and the swift implementation would be good to go!
I did try that git subtree and that would actually be great! since it would enable all the languages to be in the same repository in different dir, however act as if they are maintained in different repositories! see example I made also a small example @aardappel so you can actually see it's going to be worth it. the repo has all directories, however the branch ownership only has the ownership files only. however, I am not 100% sure how maintaining a branch for each language would be.
Had to rebase since some of the test cases would fail

[aardappel]

Yup, would totally be a fan of moving other language cruft into subdirs as well.. but that's probably for another PR.

So.. ready for me to merge?

[mustiikhalil]

i think so, unless @mzaks would say otherwise

[aardappel]

Ok, time to merge then!
Anything else can be in a follow-up.
Thanks @mustiikhalil for your hard work, and @mzaks and @vglavnyy for reviewing :)