Possible issue with SE-0166 Swift Archival & Serialization implementation


(James Froggatt) #1

I've just been trying out the new Coding protocol, and was rather surprised when trying to implement the `encode(to encoder: Encoder)` method.

The Swift evolution proposal provides the following example code:

    public func encode(to encoder: Encoder) throws {
        // Generic keyed encoder gives type-safe key access: cannot encode with keys of the wrong type.
        let container = encoder.container(keyedBy: CodingKeys.self)

        // The encoder is generic on the key -- free key autocompletion here.
        try container.encode(latitude, forKey: .latitude)
        try container.encode(longitude, forKey: .longitude)
    }

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)

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?


(Itai Ferber) #2

Hi James,

Good catch. This is a holdover from an older version of the proposal, and is now a typo.
It should be `var container` — the container can be a `struct`, and shouldn’t require reference semantics.

— Itai

···

On 8 Jun 2017, at 7:51, James Froggatt via swift-evolution wrote:

I've just been trying out the new Coding protocol, and was rather surprised when trying to implement the `encode(to encoder: Encoder)` method.

The Swift evolution proposal provides the following example code:

    public func encode(to encoder: Encoder) throws {
        // Generic keyed encoder gives type-safe key access: cannot encode with keys of the wrong type.
        let container = encoder.container(keyedBy: CodingKeys.self)

        // The encoder is generic on the key -- free key autocompletion here.
        try container.encode(latitude, forKey: .latitude)
        try container.encode(longitude, forKey: .longitude)
    }

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)

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?
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


#3

I've just been trying out the new Coding protocol, and was rather surprised when trying to implement the `encode(to encoder: Encoder)` method.

The Swift evolution proposal provides the following example code:

   public func encode(to encoder: Encoder) throws {
       // Generic keyed encoder gives type-safe key access: cannot encode with keys of the wrong type.
       let container = encoder.container(keyedBy: CodingKeys.self)

       // The encoder is generic on the key -- free key autocompletion here.
       try container.encode(latitude, forKey: .latitude)
       try container.encode(longitude, forKey: .longitude)
   }

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

You make me think that those structs should swiftly be refactored into reference types.

Gwendal

···

Le 8 juin 2017 à 16:51, James Froggatt via swift-evolution <swift-evolution@swift.org> a écrit :


(Itai Ferber) #4

Sorry, meant for that to be a reply-all.

···

On Jun 8, 2017, at 9:45 AM, Itai Ferber <iferber@apple.com> wrote:

Hi Gwendal,

On Jun 8, 2017, at 8:27 AM, Gwendal Roué via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Le 8 juin 2017 à 16:51, James Froggatt via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> a écrit :

I've just been trying out the new Coding protocol, and was rather surprised when trying to implement the `encode(to encoder: Encoder)` method.

The Swift evolution proposal provides the following example code:

  public func encode(to encoder: Encoder) throws {
      // Generic keyed encoder gives type-safe key access: cannot encode with keys of the wrong type.
      let container = encoder.container(keyedBy: CodingKeys.self)

      // The encoder is generic on the key -- free key autocompletion here.
      try container.encode(latitude, forKey: .latitude)
      try container.encode(longitude, forKey: .longitude)
  }

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)
  }

Why? It’s perfectly reasonable for the container to maintain some internal state as it’s encoding. It shouldn’t have to sacrifice value semantics for that.

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

You make me think that those structs should swiftly be refactored into reference types.

Gwendal

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


(Brent Royal-Gordon) #5

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.

···

On Jun 8, 2017, at 8:27 AM, Gwendal Roué via swift-evolution <swift-evolution@swift.org> wrote:

Le 8 juin 2017 à 16:51, James Froggatt via swift-evolution <swift-evolution@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

You make me think that those structs should swiftly be refactored into reference types.

--
Brent Royal-Gordon
Architechies


(James Froggatt) #6

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?

···

On 8 Jun 2017, at 17:48, Brent Royal-Gordon <brent@architechies.com> wrote:

On Jun 8, 2017, at 8:27 AM, Gwendal Roué via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Le 8 juin 2017 à 16:51, James Froggatt via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@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

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


#7

No big trouble, Itai: just that a value type is usually mutated before being used elsewhere.

Take this code snippet for example:

  var a = [1]
  a.append(2)
  // if a is not used any longer, something is wrong, don't you agree?

And now this other one:

  var x = SomeValueType()
  x.append(2)
  // why would it be different for x?

One of the *possible* interpretations of value types is that they are (or look like, if you prefer) purely local, and without side effect.

I know that values can hold references and vice-versa, and that the exact distinction between value and reference types is thin, even generally in the hand of the library developer: one can expose a reference type that behaves like a value type, and one can write a value type that obviously hides some references (like the containers above).

That's all. No big deal, really.
Gwendal

···

On Jun 8, 2017, at 9:45 AM, Itai Ferber <iferber@apple.com <mailto:iferber@apple.com>> wrote:

Hi Gwendal,

On Jun 8, 2017, at 8:27 AM, Gwendal Roué via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Le 8 juin 2017 à 16:51, James Froggatt via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> a écrit :

I've just been trying out the new Coding protocol, and was rather surprised when trying to implement the `encode(to encoder: Encoder)` method.

The Swift evolution proposal provides the following example code:

  public func encode(to encoder: Encoder) throws {
      // Generic keyed encoder gives type-safe key access: cannot encode with keys of the wrong type.
      let container = encoder.container(keyedBy: CodingKeys.self)

      // The encoder is generic on the key -- free key autocompletion here.
      try container.encode(latitude, forKey: .latitude)
      try container.encode(longitude, forKey: .longitude)
  }

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)
  }

Why? It’s perfectly reasonable for the container to maintain some internal state as it’s encoding. It shouldn’t have to sacrifice value semantics for that.