swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.28k stars 10.33k forks source link

[SR-6005] Dictionary's Codable implementation should handle RawRepresentable keys #48562

Open jlukas opened 6 years ago

jlukas commented 6 years ago
Previous ID SR-6005
Radar None
Original Reporter @jlukas
Type Improvement
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 1 | |Component/s | Standard Library | |Labels | Improvement | |Assignee | None | |Priority | Medium | md5: 19a5ec66fc8e8769c187bbca9bf5478a

Issue Description:

If I have a type which is RawRepresentable where RawValue == String or Int, and I use it as the Key in a Dictionary, Dictionary will fall back to using an unkeyed container of pairs instead of a keyed container. I think Dictionary should use the rawValue as the key in the keyed container instead.

Without this change, it is annoying to work with some APIs (which are out of my control).

For example:

import Foundation

struct ID: RawRepresentable, Hashable, Codable {
  let rawValue: String
  var hashValue: Int { return rawValue.hashValue }
}

struct Person: Codable {
  let name: String
  // other properties...
}

let peopleByID: [ID:Person] = [ID(rawValue: "jlukas"): Person(name: "Jacob")]
let jsonEncoder = JSONEncoder()
let data = try jsonEncoder.encode(peopleByID)
print(String(data: data, encoding: .utf8)!)

Expected output:

{"jlukas":{"name":"Jacob"}}

Actual output:

["jlukas",{"name":"Jacob"}]
belkadan commented 6 years ago

@itaiferber, I feel like there was a reason not to do this, but I can't remember it now. What do you think?

itaiferber commented 6 years ago

@belkadan There was a reason to not do this with CodingKey — namely, that it would encourage folks to access payloads in a dictionary-oriented way instead of using containers appropriately (see rdar://problem/32948271). This, however, is arguably a different case: the key is a real Codable type whose encoded representation will likely effectively be a String or Int. The reason this optimization might not be safe to apply is if the encoded representation does not end up encoding as a String or an Int, e.g., if ID were to write its own Codable implementation instead of inheriting the RawRepresentable one. In the general case, we might need to leave this as-is.

itaiferber commented 6 years ago

@jlukas This works for cases where the value uses the default RawRepresentable implementation, but what is the expected case for when it overrides that implementation? e.g.

// ID is used to look up strings in a cache built up by my app and are valid
// only during runtime.
struct ID : RawRepresentable, Hashable, Codable {
    // For lookup in my running process.
    let rawValue: Int

    var hashValue: Int {
        return self.rawValue
    }

    // For persistence, the actual string value needs to be written out.
    public func encode(to encoder: Encoder) throws {
        var container = encoder.singleValueContainer()
        try container.encode(lookupInStringsTable(self))
    }

    public init(from decoder: Decoder) throws {
        let container = try decoder.singleValueContainer()
        let stringValue = try container.decode(String.self)
        self = insertOrRetrieveCachedString(stringValue)
    }
}
jlukas commented 6 years ago

Hmm... Yeah, I see how this can't be done in the general case. I could imagine conforming to a new protocol to enable this behaviour, but I'm not convinced that's the right way to go. I just wish there was a better way than creating a new dictionary with Int/String keys when encoding or decoding.

itaiferber commented 6 years ago

I agree — I'll think on this a bit; if we can improve the experience or writing out that sort of data, we should definitely do it.