Codable conformance for Measurement #1192

bubski · 2017-08-27T03:58:31Z

Continuation of #1108, #1127 & #1148
This PR mirrors final missing changes from swiftlang/swift#9715

Added conformances + unit tests for

Measurement

bubski · 2017-08-27T04:04:49Z

@itaiferber could you please have a look at it?
To mimic your approach I had to add required init in some types.

Also, I'm not really sure how this loop can work
https://github.com/apple/swift/blob/master/test/stdlib/CodableTests.swift#L495
since it specializes to

expectRoundTripEqualityThroughJSON<Measurement<Dimension>>

thus losing the real type of associated Unit when decoding.
Interestingly, I ran this in playgrounds and it causes and exception.
🤔 Maybe I'm missing something.

And lastly, do you plan revisiting Measurement : Codable? It feels like there should be a better solution to that.

no support for UnitConverterReciprocal
no support for custom Unit subclasses

Are we not able to make Unit conformt to Codable?

itaiferber · 2017-08-28T18:06:16Z

@bubski Taking a look now. Measurement conformance to Codable is... complicated, due to the class structure we've got, and the generics involved. In fact, I would say that Measurement conformance in the overlay is incomplete (and if you'll notice, the unit test you mention is actually commented out at the bottom of the file).

I'll give further comments soon as I reacquaint myself with the structure of this.

itaiferber · 2017-08-28T19:20:40Z

To give a high-level overview of why the Foundation overlay does what it does:

The primary issue is that while Measurement is the Swift value type equivalent for NSMeasurement, Unit and Dimension are actually imported from ObjC on Darwin (they're actually NSUnit and NSDimension)
Unit is the abstract-ish superclass of all units; it's relatively rare to need to use it directly, and more so, relatively rare to need to subclass it for something which Dimension does not already cover. It's relatively rare to need to subclass Dimension, too, as you can create Dimension instances directly
However, since Unit and Dimension are imported from ObjC, new methods on them can only be added in extensions; Swift does not allow you to override methods which come from extensions, so adopting Codable directly on Unit is dangerous because all Units appear to be Codable, even though that behavior cannot be overridden. Unit could maybe handle the case for Dimension as well, but it wouldn't be able to handle custom unit types
UnitConverter & co. have the same issue — since they are imported directly from ObjC, you can only make them Codable by adopting conformance in an extension, making it non-overridable
Hence, Measurement does the nasty work of manually encoding these types itself, unfortunately breaking OOP encapsulation...
As for NSUnitConverterReciprocal — it's not public API on Darwin, and is not exposed in Swift at all. Luckily, only one unit actually uses it, but supporting it would require awkwardly exposing this class via the runtime.

So all of this comes down to ObjC importing and how these classes are exposed in Swift. While none of these issues are present in swift-corelibs-foundation as all of the classes and types are local, I don't think we should expose Codable conformance on some types through swift-corelibs-foundation, while not being able to do the same through the Foundation overlay.

This implementation is unfortunately messy, but about as good as we can do at the moment. Going to go through a more specific review of this right now.

(And yes, I do wish I'd commented all of this in Measurement.swift as I worked on it.)

itaiferber

Overall, this looks good, but can use a small amount of cleanup.

itaiferber · 2017-08-28T19:29:37Z

Foundation/Unit.swift

-
+
+    public required init(symbol: String) {
+        fatalError("You must use the designated initializer.")


I think this error message will read better as something like "\(type(of: self) must be initialized with designated initializer \(type(of: self)).init(symbol: String, converter: UnitConverter)", because... (further comment below in overrides of this method)

I'm afraig I can't simply use "\(type(of: self)) must be … though, unless I fully initialize self.

So I think our two options are

public required init(symbol: String) { // instance initialized only to provide a more readable error message with class type. self.converter = UnitConverter() super.init(symbol: symbol) fatalError("\(type(of: self)) must be initialized with designated initializer \(type(of: self)).init(symbol: String, converter: UnitConverter)") }

public required init(symbol: String) { fatalError("Dimension subclass must be initialized with designated initializer init(symbol: String, converter: UnitConverter)") }

Which one would you recommend?

Unless there is a third way I'm missing 🤔

@bubski The diagnostic here is misleading; this is actually happening because string interpolation actually puts arguments in capturing autoclosures (hence the complaint).

You should be able to check the type up-front:

let T = type(of: self) fatalError("\(T) must be initialized with designated initializer \(T).init(symbol: String, converter: UnitConverter)")

Ha! That's a surprise. Thanks again @itaiferber :)

Sure thing! It surprised me too — @phausler just showed this to me, so I'm just passing on the knowledge. 😉

itaiferber · 2017-08-28T19:30:16Z

Foundation/Unit.swift

 }

 open class UnitAcceleration : Dimension {

    /*
     Base unit - metersPerSecondSquared
     */
+
+    public required init(symbol: String) {
+        fatalError("init(symbol:) has not been implemented")


... it would now be sufficient to simply override this with super.init(symbol: symbol) and the error message will still be readable.

That's a very good suggestion. Thanks 👍

itaiferber · 2017-08-28T19:32:59Z

Foundation/Unit.swift

 }

 open class UnitAngle : Dimension {

    /*
     Base unit - degrees
     */
+
+    public required init(symbol: String) {


If both of these methods call into super, neither needs to be overridden and can just be inherited (defining the convenience initializer below won't affect anything).

Of course! I have no idea why I assumed otherwise 😶

Hehe, that's alright, happens to the best of us 🙂

itaiferber · 2017-08-28T19:33:37Z

Foundation/Unit.swift

 }

 open class UnitArea : Dimension {

    /*
     Base unit - squareMeters
     */
+
+    public required init(symbol: String) {
+        fatalError("init(symbol:) has not been implemented")


Same here (and in all repetitions below)

bubski · 2017-08-28T23:35:22Z

Thanks for a very detailed explanation @itaiferber 🙇
I was afraid there wouldn't be an obvious better way to do this with limitations coming from deriving NSUnit from ObjC/Foundation.

I fully agree that we should be as aligned with Foundation as possible, and not adding conformance to Codable where not expected.
When I asked

Are we not able to make Unit conform to Codable?

I was really asking if it would be possible in Foundation at all (by some other trickery than plain ObjC-Swift interop).

Then again, I'm not sure if it would be fair to force Unit to conform to Codable by design.

On a side note, Obj-C interop introduces lots of other weird stuff with Measurement in Foundation,
e.g.

let d = Dimension(symbol: "a") // ok
d.converter // EXC_BAD_ACCESS

So these fatalErrors in "illegal" inits are not 100% aligned with Foundation's behavior

or

let m1 = Measurement<UnitLength>(value: 1, unit: .miles)
let m2 = Measurement<Dimension>(value: 1, unit: Dimension(symbol: "mi", converter: UnitLength.miles.converter))
m2 == m1 // ok
m1 == m2 // EXC_BAD_ACCESS

itaiferber · 2017-08-29T18:13:08Z

@bubski Regarding Unit: conceptually, I don't think there's any reason for it to not conform to Codable, short of the technical limitations. We've got other places where this is really inconvenient; in the future, it's possible that we'll work around this by introducing class Unit and class Dimension in pure Swift and having those be 1-to-1 compatible with NSUnit and NSDimension (and having the Codable implementation actually be inheritable), but for now, I think we're just going to have to settle for things as-is.

Re: the fatalErrors — I would actually consider it a bug in the ObjC implementation that we don't enforce DI rules there; ObjC (like Swift) can't enforce that Dimension(symbol: "a") be uncallable statically, but we should certainly halt there since the result is nonsensical. There's a difference between being 100% in line with Foundation's interface (swift-corelibs-foundation shouldn't offer protocol conformances/API that Foundation itself does not), and with its implementation, and I think implementation discrepancies like this are understandable. In this case, I'd say it's Foundation itself that's at fault, and the fatalErrors are in line with how things should be. So, leave them in for now.

itaiferber

LGTM!

itaiferber · 2017-08-29T18:18:42Z

@swift-ci Please test

bubski · 2017-08-29T18:20:00Z

@itaiferber Thanks for all the help and especially for sharing your take on the whole Measurement stuff so extensively. It's very interesting and helpful 👍

itaiferber · 2017-08-29T19:17:18Z

@bubski Happy to help — thanks for taking the time and porting over all of these implementations!

itaiferber · 2017-08-29T19:17:25Z

@swift-ci Please test

ianpartridge · 2017-08-31T13:05:54Z

@bubski thanks for working on this! I think the only remaining one is URLComponents - see swiftlang/swift@bf4a125

Are you willing to do this one too?

bubski · 2017-08-31T13:47:08Z

@ianpartridge oh, I totally missed it.
Sure, I'll port it 👍

bubski added 4 commits August 27, 2017 04:35

Unit Equality fixed

99689f9

UnitConverter Equality fixed

3c9657a

Measurement : Codable implemented

33a1ebe

Unit Equatable unit tests added

bcbe73f

itaiferber suggested changes Aug 28, 2017

View reviewed changes

Refactored repetitive code

9dcfe9d

itaiferber approved these changes Aug 29, 2017

View reviewed changes

itaiferber merged commit 81048e4 into swiftlang:master Aug 30, 2017

bubski mentioned this pull request Nov 5, 2017

Codable conformance for URLComponents #1299

Merged



		public required init(symbol: String) {
		fatalError("You must use the designated initializer.")

Codable conformance for Measurement #1192

Codable conformance for Measurement #1192

Uh oh!

Conversation

bubski commented Aug 27, 2017

Added conformances + unit tests for

Uh oh!

bubski commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itaiferber commented Aug 28, 2017

Uh oh!

itaiferber commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itaiferber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bubski commented Aug 28, 2017

Uh oh!

itaiferber commented Aug 29, 2017

Uh oh!

itaiferber left a comment

Choose a reason for hiding this comment

Uh oh!

itaiferber commented Aug 29, 2017

Uh oh!

bubski commented Aug 29, 2017

Uh oh!

itaiferber commented Aug 29, 2017

Uh oh!

itaiferber commented Aug 29, 2017

Uh oh!

ianpartridge commented Aug 31, 2017

Uh oh!

bubski commented Aug 31, 2017

Uh oh!

Uh oh!

bubski commented Aug 27, 2017 •

edited

Loading

itaiferber commented Aug 28, 2017 •

edited

Loading