Swift 5: adopt new Data.withUnsafeBytes API #843

alanzeino · 2019-03-16T00:35:09Z

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 · 2019-03-16T16:27:04Z

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 · 2019-03-19T22:00:49Z

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 · 2019-03-19T22:11:07Z

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 · 2019-03-20T18:24:18Z

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 · 2019-03-20T21:05:43Z

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.

Sources/SwiftProtobuf/AnyMessageStorage.swift

Sources/SwiftProtobuf/Data+Extensions.swift

Sources/SwiftProtobuf/JSONEncoder.swift

tbkka · 2019-03-21T02:23:57Z

@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 · 2019-03-22T18:08:50Z

@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 · 2019-03-27T21:32:19Z

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

tbkka · 2019-03-28T02:39:13Z

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 · 2019-03-28T21:43:08Z

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 · 2019-03-28T22:23:34Z

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 · 2019-03-28T22:31:47Z

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 · 2019-03-28T22:41:30Z

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.

Sources/SwiftProtobuf/AnyMessageStorage.swift

Sources/SwiftProtobuf/BinaryDecoder.swift

Sources/SwiftProtobuf/Google_Protobuf_Any+Extensions.swift

Sources/SwiftProtobuf/JSONEncoder.swift

Sources/SwiftProtobuf/JSONScanner.swift

Sources/SwiftProtobuf/Message+JSONArrayAdditions.swift

Sources/SwiftProtobuf/Message+TextFormatAdditions.swift

Sources/SwiftProtobuf/TextFormatEncoder.swift

Sources/SwiftProtobuf/TextFormatEncodingVisitor.swift

tbkka · 2019-03-30T00:03:23Z

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.

SwiftProtobuf.xcodeproj/project.pbxproj

thomasvl · 2019-04-01T16:52:27Z

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 · 2019-04-04T21:48:41Z

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 · 2019-04-04T21:54:31Z

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

alanzeino · 2019-04-04T21:56:51Z

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.

Sources/SwiftProtobuf/JSONScanner.swift

tbkka · 2019-04-04T23:14:25Z

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

alanzeino · 2019-04-05T00:52:59Z

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

tbkka · 2019-04-05T00:59:49Z

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

thomasvl reviewed Mar 16, 2019

View reviewed changes

tbkka reviewed Mar 21, 2019

View reviewed changes

Sources/SwiftProtobuf/AnyMessageStorage.swift Outdated Show resolved Hide resolved

Sources/SwiftProtobuf/Data+Extensions.swift Outdated Show resolved Hide resolved

Sources/SwiftProtobuf/JSONEncoder.swift Outdated Show resolved Hide resolved

tbkka mentioned this pull request Mar 21, 2019

Swift5.0: Change Data(bytes:) to Data(_:) #848

Merged

tbkka changed the title ~~Update for Swift 5 compiler~~ Swift 5: adopt new Data.withUnsafeBytes API Mar 21, 2019

thomasvl mentioned this pull request Mar 22, 2019

plenty of warnings when compiling with Swift 5 #849

Closed

tbkka reviewed Mar 29, 2019

View reviewed changes

thomasvl reviewed Apr 1, 2019

View reviewed changes

SwiftProtobuf.xcodeproj/project.pbxproj Show resolved Hide resolved

thomasvl mentioned this pull request Apr 2, 2019

Upgrade to swift 5 please #860

Closed

tbkka requested changes Apr 4, 2019

View reviewed changes

Sources/SwiftProtobuf/JSONScanner.swift Show resolved Hide resolved

swift5

48bf5a8

tbkka approved these changes Apr 4, 2019

View reviewed changes

tbkka merged commit 3dd275f into apple:master Apr 5, 2019

alanzeino deleted the swift5 branch April 6, 2019 05:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swift 5: adopt new Data.withUnsafeBytes API #843

Swift 5: adopt new Data.withUnsafeBytes API #843

alanzeino commented Mar 16, 2019

thomasvl commented Mar 16, 2019 •

edited

Loading

thomasvl left a comment

alanzeino commented Mar 19, 2019 •

edited

Loading

tbkka commented Mar 19, 2019

thomasvl commented Mar 20, 2019 •

edited

Loading

alanzeino commented Mar 20, 2019

tbkka left a comment

tbkka commented Mar 21, 2019

alanzeino commented Mar 22, 2019

keith commented Mar 27, 2019

tbkka commented Mar 28, 2019

alanzeino commented Mar 28, 2019

alanzeino commented Mar 28, 2019 •

edited

Loading

tbkka commented Mar 28, 2019

alanzeino commented Mar 28, 2019 •

edited

Loading

tbkka left a comment

tbkka commented Mar 30, 2019

thomasvl commented Apr 1, 2019

alanzeino commented Apr 4, 2019

tbkka commented Apr 4, 2019

alanzeino commented Apr 4, 2019

tbkka left a comment

tbkka commented Apr 4, 2019

alanzeino commented Apr 5, 2019

tbkka commented Apr 5, 2019

Swift 5: adopt new Data.withUnsafeBytes API #843

Swift 5: adopt new Data.withUnsafeBytes API #843

Conversation

alanzeino commented Mar 16, 2019

thomasvl commented Mar 16, 2019 • edited Loading

thomasvl left a comment

Choose a reason for hiding this comment

alanzeino commented Mar 19, 2019 • edited Loading

tbkka commented Mar 19, 2019

thomasvl commented Mar 20, 2019 • edited Loading

alanzeino commented Mar 20, 2019

tbkka left a comment

Choose a reason for hiding this comment

tbkka commented Mar 21, 2019

alanzeino commented Mar 22, 2019

keith commented Mar 27, 2019

tbkka commented Mar 28, 2019

alanzeino commented Mar 28, 2019

alanzeino commented Mar 28, 2019 • edited Loading

tbkka commented Mar 28, 2019

alanzeino commented Mar 28, 2019 • edited Loading

tbkka left a comment

Choose a reason for hiding this comment

tbkka commented Mar 30, 2019

thomasvl commented Apr 1, 2019

alanzeino commented Apr 4, 2019

tbkka commented Apr 4, 2019

alanzeino commented Apr 4, 2019

tbkka left a comment

Choose a reason for hiding this comment

tbkka commented Apr 4, 2019

alanzeino commented Apr 5, 2019

tbkka commented Apr 5, 2019

thomasvl commented Mar 16, 2019 •

edited

Loading

alanzeino commented Mar 19, 2019 •

edited

Loading

thomasvl commented Mar 20, 2019 •

edited

Loading

alanzeino commented Mar 28, 2019 •

edited

Loading

alanzeino commented Mar 28, 2019 •

edited

Loading