Swift 5: adopt new Data.withUnsafeBytes API

[alanzeino]

Unfortunately I wasn't able to use the #if compiler(>=5.0) flag since the project supports older swift versions where this directive is not available.

[thomasvl]

Unfortunately I wasn't able to use the #if compiler(>=5.0) flag since the project supports older swift versions where this directive is not available.

Why? We already use directive like that without problems.

[thomasvl]

Rather that duplicating some of the larger blocks of code, can any of this be down by providing shims for for older Swift version that expose the new api names instead? Like:

https://github.com/apple/swift-protobuf/blob/master/Sources/SwiftProtobuf/MathUtils.swift#L42-L88

This way the code gets update to the Swift 5 syntax and the conditionals are kept to a minium reducing the chance of a bug that can exist in two almost identical blocks.

[alanzeino]

Unfortunately I wasn't able to use the #if compiler(>=5.0) flag since the project supports older swift versions where this directive is not available.

Why? We already use directive like that without problems.

Sorry I should've been more clear. #if swift() has been supported for a while but #if compiler() less so. The project at first glance supports Swift versions right back to 3.0, but #if compiler() was added much later than that.

I didn't see any in the codebase when I grep'd for it, but I would prefer to be wrong because I want to use that here :)

Rather that duplicating some of the larger blocks of code, can any of this be down by providing shims for for older Swift version that expose the new api names instead? Like:

https://github.com/apple/swift-protobuf/blob/master/Sources/SwiftProtobuf/MathUtils.swift#L42-L88

This way the code gets update to the Swift 5 syntax and the conditionals are kept to a minium reducing the chance of a bug that can exist in two almost identical blocks.

Yep! I'll take a stab at it.

[tbkka]

The project at first glance supports Swift versions right back to 3.0,...

Our policy is to support one full major version back. Since 5.0 is the current release version, that means we still support 4.0 and later. Once 5.2 is released, we'll be able to use #if compiler since we'll only support >= 4.2 at that point.

[thomasvl]

I also don't know the #compiler would be correct, as some of these seem to be API changes. API changes should be tied to the language version, not the underlying compiler version/release. If they were tied to the compiler doesn't that mean some code that compiles with Swift 4.x would suddenly stop compiling in 4.x mode with the 5 compiler?

ps - I completely read that initial comment as #if swift(), adding to the confusion here. 😄

[alanzeino]

I also don't know the #compiler would be correct, as some of these seem to be API changes. API changes should be tied to the language version, not the underlying compiler version/release. If they were tied to the compiler doesn't that mean some code that compiles with Swift 4.x would suddenly stop compiling in 4.x mode with the 5 compiler?

ps - I completely read that initial comment as #if swift(), adding to the confusion here. 😄

Yep you're totally right. These changes are all Swift 5 compiler in 5.0 mode, not backwards compatible with 5 compiler in 4.2 mode.

[tbkka]

A few suggestions to simplify/improve the code.

Note: You don't need to fix all the Swift 5 issues at once. Multiple smaller PRs are easier to review.

[tbkka]

@alanzeino -- I'm testing a branch where I've globally replaced all of the Data(bytes:) with Data(_:). Once I finish running tests (for Swift 4.0 through 5.0), I'll put up a PR for that. So I think you can focus on just the 24 warnings about withUnsafeBytes and withUnsafeMutableBytes. If you can fix those, I think I've got the rest of the 5.0 issues covered.

[alanzeino]

@alanzeino -- I'm testing a branch where I've globally replaced all of the Data(bytes:) with Data(_:). Once I finish running tests (for Swift 4.0 through 5.0), I'll put up a PR for that. So I think you can focus on just the 24 warnings about withUnsafeBytes and withUnsafeMutableBytes. If you can fix those, I think I've got the rest of the 5.0 issues covered.

Yep, I'll rebase.

[keith]

@thomasvl @tbkka think we could merge this so there can be a new release that supports Swift 5?

[tbkka]

The following seems sufficient to emulate the Swift 5 API on Swift 4:

#if !swift(>=5.0)
extension Data {
  func withUnsafeBytes<T>(_ body: (UnsafeRawBufferPointer) throws -> T) rethrows -> T {
    let c = count
    return try withUnsafeBytes { (p: UnsafePointer<UInt8>) throws -> T in
      try body(UnsafeRawBufferPointer(start: p, count: c))
    }
  }
  mutating func withUnsafeMutableBytes<T>(_ body: (UnsafeMutableRawBufferPointer) throws -> T) rethrows -> T {
    let c = count
    return try withUnsafeMutableBytes { (p: UnsafeMutablePointer<UInt8>) throws -> T in
      try body(UnsafeMutableRawBufferPointer(start: p, count: c))
    }
  }
}
#endif

Note that every place we used the old withUnsafeBytes APIs, we always used it with UInt8, so this is actually correct for us.

Using the above, I've experimentally switched a couple of places to use the Swift 5 APIs -- the result seems to work correctly on both Swift 4 and Swift 5. In the process, I did see some spots where the resulting code could probably be simplified, but I think such improvements can wait until after we get a clean build.

[alanzeino]

The following seems sufficient to emulate the Swift 5 API on Swift 4:

#if !swift(>=5.0)
extension Data {
  func withUnsafeBytes<T>(_ body: (UnsafeRawBufferPointer) throws -> T) rethrows -> T {
    let c = count
    return try withUnsafeBytes { (p: UnsafePointer<UInt8>) throws -> T in
      try body(UnsafeRawBufferPointer(start: p, count: c))
    }
  }
  mutating func withUnsafeMutableBytes<T>(_ body: (UnsafeMutableRawBufferPointer) throws -> T) rethrows -> T {
    let c = count
    return try withUnsafeMutableBytes { (p: UnsafeMutablePointer<UInt8>) throws -> T in
      try body(UnsafeMutableRawBufferPointer(start: p, count: c))
    }
  }
}
#endif

Note that every place we used the old withUnsafeBytes APIs, we always used it with UInt8, so this is actually correct for us.

Using the above, I've experimentally switched a couple of places to use the Swift 5 APIs -- the result seems to work correctly on both Swift 4 and Swift 5. In the process, I did see some spots where the resulting code could probably be simplified, but I think such improvements can wait until after we get a clean build.

Ah I didn't think to hardcode the type. Updating now.

[alanzeino]

Updated all the cases. One thing to mention, looks like the master branch does not compile right now. So I did this rebased on top of a commit that did compile, then once I verified that it all compiled after my changes, I rebased so that the diff didn't have any conflicts.

Any plan to add a CI check to these PRs?

[tbkka]

We don't have PR testing, but we do have CI for the master branch:

https://ci.swift.org/job/swift-protobuf-linux-ubuntu-16_04/

According to that, the master branch did build last night. I'll take a look when I get a chance.

[alanzeino]

We don't have PR testing, but we do have CI for the master branch:

https://ci.swift.org/job/swift-protobuf-linux-ubuntu-16_04/

According to that, the master branch did build last night. I'll take a look when I get a chance.

If I were to guess, I'd say that master branch build uses swift build and not xcodebuild build — and the reason why this doesn't compile is because a new type (TextFormatEncodingOptions) was added in a new file, but the .xcodeproj wasn't updated to include that new file.

That also raises another question I had — should we add the .xcodeproj to the .gitignore and just generate it using Swift PM? Then we can add a CI job that does both swift build and then swift package generate-xcodeproj && xcodebuild build -scheme SwiftProtobuf_iOS , which would catch these project inconsistencies.

[tbkka]

Much better! The one cleanup I'd like to see right away is to fix a few places where the closure is accessing the Data object directly instead of using the buffer argument. I've marked some of them.

[tbkka]

Does your branch pass the test suite? (E.g. make test?)

I'm seeing failures here in the following three tests:
SwiftProtobufTests.Test_JSON testMultipleFields
SwiftProtobufTests.Test_JSON_Array testTwoObjectsWithMultipleFields
SwiftProtobufTests.Test_Map_JSON testMapInt32Bytes

I find it interesting that the failing cases all relate to JSON and all of them have a bytes field which is empty.

[thomasvl]

That also raises another question I had — should we add the .xcodeproj to the .gitignore and just generate it using Swift PM? Then we can add a CI job that does both swift build and then swift package generate-xcodeproj && xcodebuild build -scheme SwiftProtobuf_iOS , which would catch these project inconsistencies.

Since SwiftPM doesn't really do iOS, and to better support users wanting Carthage, etc.; I think we end up needing a project file checked in. 🙁 The Makefile does have a bunch of Xcode related targets (test-xcode*), but I don't think most people bother to check them often.

[alanzeino]

Does your branch pass the test suite? (E.g. make test?)

I'm seeing failures here in the following three tests:
SwiftProtobufTests.Test_JSON testMultipleFields
SwiftProtobufTests.Test_JSON_Array testTwoObjectsWithMultipleFields
SwiftProtobufTests.Test_Map_JSON testMapInt32Bytes

I find it interesting that the failing cases all relate to JSON and all of them have a bytes field which is empty.

I should've taken your advice and just converted a few cases just to be safe. Unfortunately I have a bit of a problem in that I'm starting at a new employer next week that might not want me working on Open Source so how about for now I just land the back–ported extension in Data+Extensions.swift and convert one other place for reference, and I'll leave the rest to you since it would be a more careful strategy to do them piecemeal?

[tbkka]

If you can fix the style issues I pointed out earlier, I'll see if I can debug that test failure.

[alanzeino]

If you can fix the style issues I pointed out earlier, I'll see if I can debug that test failure.

Okay!

[tbkka]

I figured out why zero-length bytes fields were failing.

[tbkka]

Looks good. I'm going to run some more tests before I merge this.

[alanzeino]

Cool! Lemme know if there's more changes I should make.

[tbkka]

I ran tests on Ubuntu with Swift 4.0 through a 5.0 beta and it all looks good. Thanks for working through this!