<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta http-equiv="Content-Type" content="text/xhtml; charset=utf-8">
</head>
<body>
<div style="font-family:sans-serif"><div style="white-space:normal">
<p dir="auto">On 16 Mar 2017, at 14:29, Matthew Johnson wrote:</p>
<p dir="auto"></p></div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #777; color:#777; margin:0 0 5px; padding-left:5px"><p dir="auto">This is a fantastic proposal! I am very much looking forward to robust Swift-native encoding and decoding in Foundation. The compiler synthesized conformances is especially great! I want to thank everyone who worked on it. It is clear that a lot of work went into the proposal.<br>
<br>
The proposal covers a lot of ground so I’m breaking my comments up by topic in the order the occur in the proposal.</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">Thanks for the feedback, Matthew! Responses inline.</p>
<p dir="auto"></p></div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #777; color:#777; margin:0 0 5px; padding-left:5px"><p dir="auto">Encode / Decode only types:<br>
<br>
Brent raised the question of decode only types. Encode only types are also not uncommon when an API accepts an argument payload that gets serialized into the body of a request. The compiler synthesis feature in the proposal makes providing both encoding and decoding easy in common cases but this won’t always work as needed.<br>
<br>
The obvious alternative is to have Decodable and Encodable protocols which Codable refines. This would allow us to omit a conformance we don’t need when it can’t be synthesized.</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">If conformances are still synthesized individually (i.e. for just <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Decodable</code> or just <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Encodable</code>), it would be way too easy to accidentally conform to one or the other and not realize that you’re not conforming to <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Codable</code>, since the synthesis is invisible. You’d just be missing half of the protocol.</p>
<p dir="auto">If the way out of this is to only synthesize conformance to <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Codable</code>, then it’s much harder to justify the inclusion of <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Encodable</code> or <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Decodable</code> since those would require a manual implementation and would much more rarely be used.</p>
<p dir="auto"></p></div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #777; color:#777; margin:0 0 5px; padding-left:5px"><p dir="auto">Your reply to Brent mentions using `fatalError` to avoid implementing the direction that isn't needed. I think it would be better if the conformance can reflect what is actually supported by the type. Requiring us to write `fatalError` as a stub for functionality we don’t need is a design problem IMO. I don’t think the extra protocols are really that big a burden. They don’t add any new functionality and are very easy to understand, especially considering the symmetry they would have with the other types you are introducing.<br>
<br>
Coding Keys:<br>
<br>
As others have mentioned, the design of this protocol does not require a value of a conforming type to actually be a valid key (it can return nil for both `intValue` and `stringValue`). This seems problematic to me.<br>
<br>
In the reply to Brent again you mention throwing and `preconditionFailure` as a way to handle incompatible keys. This also seems problematic to me and feels like a design problem. If we really need to support more than one underlying key type and some encoders will reject some key types this information should be captured in the type system. An encoder would only vend a keyed container for keys it actually supports. Ideally the conformance of a type’s CodingKeys could be leveraged to produce a compiler error if an attempt was made to encode this type into an encoder that can’t support its keys. In general, the idea is to produce static errors as close to the origin of the programming mistake as possible.<br>
<br>
I would very much prefer that we don’t defer to runtime assertions or thrown errors, etc for conditions that could be caught statically at compile time given the right design. Other comments have indicated that static guarantees are important to the design (encoders *must* guarantee support of primitives specified by the protocols, etc). Why is a static guarantee of compatible coding keys considered less important?</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">I agree that it would be nice to support this in a static way, but while not impossible to represent in the type system, it absolutely explodes the API into a ton of different types and protocols which are not dissimilar. We’ve considered this in the past (see approach #4 in the <a href="https://github.com/itaiferber/swift-evolution/blob/637532e2abcbdb9861e424359bb6dac99dc6b638/proposals/XXXX-swift-archival-serialization.md#alternatives-considered" style="color:#3983C4">Alternatives Considered</a> section) and moved away from it for a reason.</p>
<p dir="auto">To summarize:</p>
<ul>
<li>To statically represent the difference between an encoder which supports string keys and one which supports integer keys, we would have to introduce two different protocol types (say, <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">StringKeyEncoder</code> and <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">IntKeyEncoder</code>)</li>
<li>Now that there are two different encoder types, the <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Codable</code> protocol also needs to be split up into two — one version which encodes using a <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">StringKeyEncoder</code> and one version which encodes using an <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">IntKeyEncoder</code>. If you want to support encoding to an encoder which supports both types of keys, we’d need a <em>third</em> Codable protocol which takes something that’s <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">StringKeyEncoder & IntKeyEncoder</code> (because you cannot just conform to both <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">StringCodable</code> and <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">IntCodable</code> — it’s ambiguous when given something that’s <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">StringKeyEncoder & IntKeyEncoder</code>)</li>
<li>On encoders which support both string and integer keys, you need overloads for <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">encode<T : StringCodable>(…)</code>, <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">encode<T : IntCodable>(…)</code>, and <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">encode<T : StringCodable & IntCodable>(…)</code> or else the call is ambiguous
<ul>
<li>Repeat for both <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">encode<T : …>(_ t: T?, forKey: String)</code> and <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">encode<T : …>(_ t: T?, forKey: Int)</code></li>
</ul></li>
<li>Repeat for decoders, with all of their overloads as well</li>
</ul>
<p dir="auto">This is not to mention the issue of things which are single-value encodable, which adds additional complexity. Overall, the complexity of this makes it unapproachable and confusing as API, and is a hassle for both consumers and for <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Encoder</code>/<code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Decoder</code> writers.</p>
<p dir="auto">We are willing to make the runtime failure tradeoff for keeping the rest of the API consumable with the understanding that we expect that the vast majority of <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">CodingKey</code> conformances will be automatically generated, and that type authors will generally provide key types which are appropriate for the formats they expect to encode their own types in.</p>
<p dir="auto"></p></div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #777; color:#777; margin:0 0 5px; padding-left:5px"><p dir="auto">Keyed Containers:<br>
<br>
Joe posted raised the topic of the alternative of using manual type erasure for the keyed containers rather than abstract classes. Did you explore this direction at all? It feels like a more natural approach for Swift and as Joe noted, it can be designed in such a way that eases migration to existentials in the future if that is the “ideal” design (which you mentioned in your response).</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">Joe mentions the type-erased types as an example of where we’ve had to use those because we’re lacking other features — I don’t see how type erasure would be the solution. We’re doing the opposite of type-erasure: we’re trying to offer an abstract type that is generic and <em>specified</em> on a type you give it. The base <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">KeyedEncodingContainer</code> is effectively a type-erased base type, but it’s the generics that we really need.</p>
<p dir="auto"></p></div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #777; color:#777; margin:0 0 5px; padding-left:5px"><p dir="auto">Decoding Containers:<br>
<br>
returns: A value of the requested type, if present for the given key and convertible to the requested type.<br>
<br>
Can you elaborate on the details of “convertible to the requested type” means? It think this is an important detail for the proposal.<br>
<br>
For example, I would expect numeric values to attempt conversion using the SE-0080 failable numeric conversion initializers (decoding JSON was a primary motivating use case for that proposal). If the requested type conforms to RawRepresentable and the encoded value can be converted to RawValue (perhaps using a failable numeric initializer) I would expect the raw value initializer to be used to attempt conversion. If Swift ever gained a standard mechanism for generalized value conversion I would also expect that to be used if a conversion is available from the encoded type to the requested type.<br>
<br>
If either of those conversions fail I would expect something like an “invalid value” error or a “conversion failure” error rather than a “type mismatch” error. The types don’t exactly mismatch, we just have a failable conversion process that did not succeed.</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">Keep in mind that no type information is written into the payload, so the interpretation of this is up to the <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Encoder</code> and its format.<br>
JSON, for instance, has no delineation between number types. For <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">{"value": 1}</code>, you should be able to <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">decode(…, forKey: .value)</code> the value through any one of the numeric types, since 1 is representable by any of them. However, requesting it as a <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">String</code> should throw a <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">.coderTypeMismatch</code>.</p>
<p dir="auto">If you try to ask for <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">3.14</code> as an <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Int</code>, I think it’s valid to get a <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">.coderTypeMismatch</code> — you asked for something of the wrong type altogether. I don’t see much value in providing a different error type to represent the same thing.</p>
<p dir="auto"></p></div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #777; color:#777; margin:0 0 5px; padding-left:5px"><p dir="auto">Context:<br>
<br>
I’m glad Brent raised the topic of supporting multiple formats. In his example the difference was remote and local formats. I’ve also seen cases where the same API requires the same model to be encoded differently in different endpoints. This is awful, but it also happens sometimes in the wild. Supporting an application specified encoding context would be very useful for handling these situations (the `codingKeyContex` would not always be sufficient and would usually not be a good way to determine the required encoding or decoding format).<br>
<br>
A `[UserInfoKey: Any]` context was mentioned as a possibility. This would be better than nothing, but why not use the type system to preserve information about the type of the context? I have a slightly different approach in mind. Why not just have a protocol that refines Codable with context awareness?<br>
<br>
public protocol ContextAwareCodable: Codable {<br>
associatedtype Context<br>
init(from decoder: Decoder, with context: Context?) throws<br>
func encode(to encoder: Encoder, with context: Context?) throws<br>
}<br>
extension ContextAwareCodable {<br>
init(from decoder: Decoder) throws {<br>
try self.init(from: decoder, with: nil<br>
}<br>
func encode(to encoder: Encoder) throws {<br>
try self.encode(to: encoder, with: nil)<br>
}<br>
}</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">There are a few issues with this:</p>
<ol>
<li value="1">For an <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Encoder</code> to be able to call <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">encode(to:with:)</code> with the <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Context</code> type, it would have to know the <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Context</code> type statically. That means it would have to be generic on the <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Context</code> type (we’ve looked at this in the past with regards to the encoder declaring the type of key it’s willing to accept)</li>
<li value="2">It makes more sense for the <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Encoder</code> to define what context type it vends, rather than have <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Codable</code> things discriminate on what they can accept. If you have two types in a payload which require mutually-exclusive context types, you cannot encode the payload at all</li>
<li value="3"><code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">associatedtype</code> requirements cannot be overridden by subclasses. That means if you subclass a <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">ContextAwareCodable</code> type, you cannot require a different context type than your parent, which may be a no-go. This, by the way, is why we don’t have an official <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">associatedtype CodingKeys : CodingKey</code> requirement on <code style="background-color:#F7F7F7; border-radius:3px; margin:0; padding:0 0.4em" bgcolor="#F7F7F7">Codable</code></li>
</ol>
<p dir="auto"></p></div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #777; color:#777; margin:0 0 5px; padding-left:5px"><p dir="auto">Encoders and Decoders would be encouraged to support a top level encode / decode method which is generic and takes an application supplied context. When the context is provided it would be given to all `ContextAwareCodable` types that are aware of this context type during encoding or decoding. The coding container protocols would include an overload for `ContextAwareCodable` allowing the container to know whether the Value understands the context given to the top level encoder / decoder:<br>
<br>
open func encode<Value : ContextAwareCodable>(_ value: Value?, forKey key: Key) throws<br>
<br>
A common top level signature for coders and decoders would look something like this:<br>
<br>
open func encode<Value : Codable, Context>(_ value: Value, with context: Context) throws -> Data<br>
<br>
This approach would preserve type information about the encoding / decoding context. It falls back to the basic Codable implementation when a Value doesn’t know about the current context. The default implementation simply translates this to a nil context allowing ContextAwareCodable types to have a single implementation of the initializer and the encoding method that is used whether they are able to understand the current context or not.</p>
</blockquote></div>
<div style="white-space:normal">
<p dir="auto">I amend the above — this means that if you have two types which require different contexts in the same payload, only one of them will get the context and the other silently will not. I’m not sure this is better.</p>
<p dir="auto">A slightly more type-erased context type would allow all members to look at the context if desired without having to split the protocol, require multiple different types and implementations, etc.</p>
<p dir="auto"></p></div>
<div style="white-space:normal"><blockquote style="border-left:2px solid #777; color:#777; margin:0 0 5px; padding-left:5px"><p dir="auto">- Matthew</p>
</blockquote></div>
<div style="white-space:normal">
</div>
</div>
</body>
</html>