[swift-evolution] [Review] SE-0166: Swift Archival & Serialization

[Brent Royal-Gordon]

> On Apr 12, 2017, at 6:18 PM, Russ Bishop <rbishopjr at apple.com> wrote:
> 
>> 
>> On Apr 12, 2017, at 5:44 PM, Brent Royal-Gordon <brent at architechies.com <mailto:brent at architechies.com>> wrote:
>> 
>>> On Apr 12, 2017, at 11:44 AM, Russ Bishop via swift-evolution <swift-evolution at swift.org <mailto:swift-evolution at swift.org>> wrote:
>>> 
>>> * String and Int keys being optional, giving a CodingKey the opportunity to return nil for both at runtime.
>> 
>> This was true in the first public draft, but I believe in this version `stringValue` is no longer Optional.
> 
> You are correct; I still don't like the fact that you can be in an unkeyed archiver situation and hitting CodingKey adopters that don't provide integer keys. 

You are building this dislike on top of several misconceptions:

* There are no "keyed archivers" and "unkeyed archivers". There are keyed and unkeyed *containers*, and all coders handle both kinds.

* Unkeyed containers don't use keys at all, so this has nothing to do with keyed vs. unkeyed. Unkeyed containers are for storing a series of values with no inherent structure to them, only an order. Array elements and tuples are good examples. (But of course, you'd just encode the array as a whole into an entry in a keyed container and the array itself would create an unkeyed container and put its elements into it.)

* Integer keys are a space-efficient alternative to identifying keys by string value; they give supporting coders a fixed-width value they can use to represent a key. But they don't need to be contiguous and they don't necessarily have any influence on the order that values are encoded in. As long as you don't change an existing field's integerValue or reuse an integerValue previously assigned to a different key, there should be no versioning problems.

>>> * Encoder has three functions, but only one may ever be called. This API is the opposite of "pit of success".
>> 
>> Personally, I'm worried about that too. But I had a lot of trouble coming up with an alternative that didn't violate a more important goal, like "being able to throw errors" or "being compatible with classes".
>> 
>> Last-ditch suggestion: Change a bunch of names so that, for instance, `Encoder` is instead `EncodingContainerizer`. (That's a terrible name, but "Factory" gives me the hives.) That way, the name of the type gives you a hint that you're supposed to use it to make a container. You might even rename the methods to e.g. `useContainer(keyedBy:)`, which sound a little more stateful and mutually-exclusive.
>> 
>> 	func encode(to encoder: EncodingContainerizer) throws {
>> 		var container = encoder.useContainer(keyedBy: CodingKeys.self)
>> 		try container.encode(name, forKey: .name)
>> 		try container.encode(birthDate, forKey: .birthDate)
>> 	}
> 
> We could just completely separate the idea of keyed and unkeyed encoding?

As I said, I think you misunderstand what these things are for.

I would like, though, to have the type declare the kind of container it wants. In a previous thread, I suggested that `encode(to:)` should take a container directly, and we should construct it for them:

	func encode(to container: KeyedEncodingContainer<CodingKeys>) throws {
		try container.encode(name, forKey: .name)
		try container.encode(birthDate, forKey: .birthDate)
	}

The authors pointed out that this would require you to make the `CodingKeys` type public and would mean that subclasses couldn't switch container types. These really are important problems with any idea of that sort.

>> I argued about this pretty vigorously. They want to avoid the overhead of building an encoder and single-value container and then making several generic calls to encode the value into the container. Personally, I think this is premature optimization of the highest order—particularly since building an encoder and single-value container are often no-ops—but this is the design they chose, and I don't think it's worth rejecting for that alone.
> 
> Has someone done performance tests to validate that this is an issue? If nothing else this seems like an implementation detail a conforming type could opt to provide but it shouldn't be required in every implementation.

I don't know what testing they've done, but this is not something that individual coders could add after the fact. `encode(to:)` and `init(from:)` are passed only protocol existentials containing the coder, so they don't have access to any member that isn't in the protocol. The same for containers. Accessing any special, coder-specific methods would perhaps not be impossible, but it'd be prohibitively difficult. So if any coders want to have this fast path, it needs to be part of the protocol ahead of time; it can't be bolted on by only the conforming types that want it.

>> I think they've done a reasonable job of putting unkeyed coding in a sharps drawer by making you specifically ask for it and giving it an ugly name. 
> 
> Can you clarify what you mean? I see keyed and unkeyed spread across various protocols and types in the proposal. It's front and center. 

What I mean is that, when you get an encoder, you have three choices: `container(keyedBy:)`, `unkeyedContainer()`, and `singleValueContainer()`. If you looked into single-value, you'd quickly discover that it only supports a few low-level types (actually, I think it might be better to call it `singlePrimitiveValueContainer()`. So that leaves you with keyed and unkeyed. `unkeyedContainer()` has an ugly, awkward name (this is actually intentional), and just about every piece of documentation or tutorial is going to tell you not to use it. Meanwhile, `container(keyedBy:)` is nice and we generate all sorts of code and stuff to make it easy to use. Once you're into a keyed container, there's no way to accidentally use any unkeyed methods.

And all of this is only an issue if you write the conformance by hand in the first place. Usually, you won't.

-- 
Brent Royal-Gordon
Architechies

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20170412/a4ef2743/attachment.html>