[Review] SE-0167: Swift Encoders


(Douglas Gregor) #1

Hello Swift community,

The review of SE-0167 "Swift Encoders" begins now and runs through April 12, 2017. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0167-swift-encoders.md
Note that this proposal is closely related to (and dependent on) SE-0166: Swift Archival & Serialization <https://github.com/apple/swift-evolution/blob/master/proposals/0166-swift-archival-serialization.md>. Please read and review that proposal as well!

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0167-swift-encoders.md
Reply text
Other replies
<https://github.com/apple/swift-evolution/blob/master/process.md#what-goes-into-a-review-1>What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at

https://github.com/apple/swift-evolution/blob/master/process.md
Thank you,

-Doug

Review Manager


(Jordan Rose) #2

[Proposal: https://github.com/apple/swift-evolution/blob/master/proposals/0167-swift-encoders.md]

There's some pretty cool stuff in here. Nice work. I do have a few questions about this design, though:

- Why are the encoders and decoders open classes? (In particular, the fixed set of "strategies" seems like it'll be a problem if we plan to keep adding cases.) For that matter, why make them classes at all?

- I don't understand 'DateEncodingStrategy.deferredToDate'. It seems like this will always match one of the other cases in practice, and it isn't safe for any stable encoding.

- I assume you've thought about this, but there's no way for existing Cocoa types to conform to Decodable, since it introduces a new required initializer.* The design here doesn't rely on that at all, but I feel like it's worth calling out explicitly.

* Why does Decodable introduce a required initializer? Because when a class conforms to a protocol, it's declaring that it and all its subclasses conform to the protocol. But since initializers are not automatically inherited in Swift, there's no way to add new initializers to all existing subclasses.

Jordan


(Russ Bishop) #3

Doug,

Would it be worth deferring this slightly since it relies on SE-0166? If changes are made to SE-0166 as a result of review it will necessarily impact this proposal.

Russ Bishop

···

On Apr 6, 2017, at 11:10 AM, Douglas Gregor via swift-evolution <swift-evolution@swift.org> wrote:

Hello Swift community,

The review of SE-0167 "Swift Encoders" begins now and runs through April 12, 2017. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0167-swift-encoders.md
Note that this proposal is closely related to (and dependent on) SE-0166: Swift Archival & Serialization <https://github.com/apple/swift-evolution/blob/master/proposals/0166-swift-archival-serialization.md>. Please read and review that proposal as well!

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0167-swift-encoders.md
Reply text
Other replies
<https://github.com/apple/swift-evolution/blob/master/process.md#what-goes-into-a-review-1>What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at

https://github.com/apple/swift-evolution/blob/master/process.md
Thank you,

-Doug

Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Brad Hilton) #4

-1. I left my feedback for this proposal and SE-0166 on the other thread, but TLDR; I think this is premature until we add true reflection capabilities
to Swift.

···

Hello Swift community,

The review of SE-0167 "Swift Encoders" begins now and runs through April 12, 2017. The proposal is available here:

> https://github.com/apple/swift-evolution/blob/master/proposals/0167-swift-encoders.md
Note that this proposal is closely related to (and dependent on)SE-0166: Swift Archival&Serialization(https://github.com/apple/swift-evolution/blob/master/proposals/0166-swift-archival-serialization.md). Please read and review that proposal as well!

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

> https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

>
> Proposal link:
>
> > https://github.com/apple/swift-evolution/blob/master/proposals/0167-swift-encoders.md
>
> Reply text
> > Other replies
>
What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at

> https://github.com/apple/swift-evolution/blob/master/process.md
Thank you,

-Doug

Review Manager


(Jon Shier) #5

What is your evaluation of the proposal?

-1. There is a lot good here, but this feature is too important to let an “okay” proposal through.

* Usage of the API doesn’t feel very Swifty, unless I’m missing some intended use case. Why are the encoders / decoders classes? Why do they have mutable properties? What is the userInfo value for? It all feels very reminiscent of the NSFormatter APIs, which isn’t really a good thing. This is supposed to be the Swift native way to encode and decode types, so it should feel native. It’s close in a lot of ways (I like the strategies enums) but isn’t really there.

* This is more for 166, but decode(MyValue.self)? Gross. Use the generic to convey the type inside the API.

* This proposal fails to make full usage of the Swift’s error capabilities and the errors live in the wrong place. I’m putting this here since this proposal defines the errors but really they should be part of 166. These error types are inferior to the errors we can produce in Swift right now. They should be Swift native errors first and bridged into NSError second, not created with the primary intent to be bridged. For example:

public static var coderTypeMismatch: CocoaError.Code

First off, the coder* prefix is unnecessary if this is properly moved into its own concrete error type. Second, it doesn’t capture the mismatch values, leading to unnecessary debugging overhead. Make the error a proper enum that can capture the type found and type expected and it becomes far more useful. For example (roughly):

public enum DecoderError: Error {
    typeMismatch(expected:actual:)
    readCorrupt(underlyingError: Error) // presumably this wraps the underlying serialization error?
    valueNotFound(forKey: Key)
}

Also, these errors should live in the standard library, otherwise how are native types going do their own encoding/decoding without importing Foundation. Again, this is more for 166, but they were defined here, so… I understand the need to perhaps bridge them to existing errors (I don’t think it’s necessary, but I understand the desire), but that should be done under the covers, so to speak, and not exposed to Swift developers by limiting a core API so badly. Besides, the errors you want to bridge to aren’t very good in the first place. "The data couldn't be read because it isn't in the correct format.” Seriously? Could we get a little more detail, please?

* There is nothing proposed to add any type safety to the decoding process. We know exactly what types can exist in JSON. Why are we still representing it as Any or the raw Data? Providing greater safety during the decoding process helps developers find what’s wrong faster.

Is the problem being addressed significant enough to warrant a change to Swift?

Yes, we need native decoding and encoding of common formats like JSON, but I don’t believe this solution is good enough.

Does this proposal fit well with the feel and direction of Swift?

No, this feels like a port of an Objective-C API, and it literally is in some ways.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

I’ve been using Argo for 2 years now. While much of the comparison is more relevant to 166, I think it’s still a great example of what a Swift native decoding experience should look like. It’s especially superior in the errors it produces.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Fairly in depth I’d say.

Jon Shier


(Douglas Gregor) #6

Doug,

Would it be worth deferring this slightly since it relies on SE-0166? If changes are made to SE-0166 as a result of review it will necessarily impact this proposal.

If there are changes to SE-0166 that affect this, we can always hold off on a decision to make more adjustments. I don’t think we need to stagger the review of two closely-related proposals like this.

  - Doug

···

On Apr 12, 2017, at 11:49 AM, Russ Bishop <rbishopjr@apple.com> wrote:

Russ Bishop

On Apr 6, 2017, at 11:10 AM, Douglas Gregor via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Hello Swift community,

The review of SE-0167 "Swift Encoders" begins now and runs through April 12, 2017. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0167-swift-encoders.md
Note that this proposal is closely related to (and dependent on) SE-0166: Swift Archival & Serialization <https://github.com/apple/swift-evolution/blob/master/proposals/0166-swift-archival-serialization.md>. Please read and review that proposal as well!

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at

https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:

Proposal link:

https://github.com/apple/swift-evolution/blob/master/proposals/0167-swift-encoders.md
Reply text
Other replies
<https://github.com/apple/swift-evolution/blob/master/process.md#what-goes-into-a-review-1>What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at

https://github.com/apple/swift-evolution/blob/master/process.md
Thank you,

-Doug

Review Manager

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution