[swift-evolution] Possible issue with SE-0166 Swift Archival & Serialization implementation

[James Froggatt]

> On 8 Jun 2017, at 17:48, Brent Royal-Gordon <brent at architechies.com> wrote:
> 
>> On Jun 8, 2017, at 8:27 AM, Gwendal Roué via swift-evolution <swift-evolution at swift.org <mailto:swift-evolution at swift.org>> wrote:
>>> 
>>> Le 8 juin 2017 à 16:51, James Froggatt via swift-evolution <swift-evolution at swift.org <mailto:swift-evolution at swift.org>> a écrit :
>>> 
>>> Here, container is stored as a `let` value, and uses reference semantics, while the proposal also clearly lists these `encode` methods as mutating. With the current implementation of the proposal, the container must be stored as a `var`, which leads to code like the following:
>>> 
>>>   var container = encoder.singleValueContainer()
>>>   try container.encode(data)
>> 
>> Yes, practically speaking and with latest Swift 4, the container needs to be declared as `var`.
>> 
>> I admit it's weird, and feels unnatural:
>> 
>>   public func encode(to encoder: Encoder) throws {
>>       // A mutated value that nobody consumes: so weird.
>>       var container = encoder.container(keyedBy: CodingKeys.self)
>>       try container.encode(latitude, forKey: .latitude)
>>       try container.encode(longitude, forKey: .longitude)
>>   }
>> 
>>> This clearly wont work as expected if the container were to have value semantics, and writing code like this feels plain wrong. Is SE-0166 really intended to work with referrence-type encoders only?
>> 
>> Actually, it can work with encoder/containers that have value semantics, and forward the mutations somewhere else (for example to a closure which fills a mutating container).
>> 
>> But this is again bizarre, and contrieved: https://github.com/groue/GRDB.swift/blob/15bfe5f6cf76070cfb17216223bdebc6b158d654/GRDB/Record/Persistable%2BEncodable.swift <https://github.com/groue/GRDB.swift/blob/15bfe5f6cf76070cfb17216223bdebc6b158d654/GRDB/Record/Persistable%2BEncodable.swift>
>> 
>> You make me think that those structs should swiftly be refactored into reference types.
> 
> 
> I made precisely this comment during the review. The response was that the container could potentially defined by a struct that contains some state which it uses to write into a reference. For instance, imagine a type like this:
> 
> 	class MyEncoder: Encoder {
> 		var buffer: Data
> 		
> 		struct KeyedEncodingContainer<K: CodingKey>: KeyedEncodingContainerProtocol {
> 			let encoder: MyEncoder
> 			var bufferOffset: Int
> 			
> 			mutating func encode(_ value: String, forKey key: K) throws {
> 				let newData = try makeDataFrom(value, forKey: key)
> 				
> 				encoder.buffer.insert(newData, at: offset)
> 				bufferOffset += newData.count
> 			}
> 			…
> 		}
> 		…
> 	}
> 
> I argued that foreclosing designs like this would be acceptable—you could either make `KeyedEncodingContainer` a class or move `offset` into `MyEncoder` to get around it—but the proposers preferred to preserve this flexibility.
> 
> -- 
> Brent Royal-Gordon
> Architechies
> 

I see. In this context the documentation's list of preconditions makes sense, though perhaps it could be updated to more directly explain this kind of use case? This is an important detail to take into account, as it means that making a copy of the container could lead to unexpected behaviour:

    var container2 = container
    container.encode(data1, forKey: CodingKeys.key1)
    container2.encode(data2, forKey: CodingKeys.key2) // oops, what does .key1 point to now?


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