[SR-6753] CodingUserInfoKey's init?(rawValue:) shouldn't be failable

[hamishknight]

Previous ID	SR-6753
Radar	rdar://problem/36559619
Original Reporter	@hamishknight
Type	Bug

Environment

Swift version 4.1-dev (LLVM db86aaee13, Clang c336c4829f, Swift cd7600c)
Target: x86_64-apple-darwin17.3.0

Additional Detail from JIRA

Votes	1
Component/s	Standard Library
Labels	Bug, Codable
Assignee	@itaiferber
Priority	Medium

md5: f6c9f746f91348e1f3a619642dc94a77

Issue Description:

Currently, CodingUserInfoKey's init?(rawValue:) is failable (despite never returning nil), so you have to force unwrap the result. In addition, SE-0166 proposed a non-failable init(_ value: String), which is yet to be implemented.

IMO, the API should be changed to have both a non-failable init(rawValue:) and (convenience) init(_ rawValue:).

[hamishknight]

cc @itaiferber. I'm happy to fix this myself if you want, although what would be the best way to minimise source-breakage for the failable to non-failable init? Unfortunately, @available doesn't seem to currently play well with protocol requirements:

// error: type 'CodingUserInfoKey' does not conform to protocol 'RawRepresentable'
// note: multiple matching initializers named 'init(rawValue:)' with type '(rawValue: CodingUserInfoKey.RawValue)' (aka '(rawValue: String)')
public struct CodingUserInfoKey : RawRepresentable, Equatable, Hashable {
  public typealias RawValue = String

  /// The key's string value.
  public let rawValue: String

  /// Creates a new instance with the given raw value.
  ///
  /// - parameter rawValue: The value of the key.
  @_inlineable // FIXME(sil-serialize-all)
  public init(_ rawValue: String) {
    self.rawValue = rawValue
  }

  /// Creates a new instance with the given raw value.
  ///
  /// - parameter rawValue: The value of the key.
  @_inlineable // FIXME(sil-serialize-all)
  @available(swift, introduced: 5)
  public init(rawValue: String) {
    self.rawValue = rawValue
  }

  /// Creates a new instance with the given raw value.
  ///
  /// - parameter rawValue: The value of the key.
  @_inlineable // FIXME(sil-serialize-all)
  @available(swift, deprecated: 4.1, renamed: "init(_:)")
  @available(swift, obsoleted: 5, renamed: "init(_:)")
  public init?(rawValue: String) {
    self.rawValue = rawValue
  }
  // ...

Is there a way around this? Or is it simply not worth fixing this, and better to just add a non-failable init(_ rawValue:)?

[itaiferber]

@hamishknight Good catch, Hamish! I believe this is a typo in the proposal, though. CodingUserInfoKey is defined twice in the proposal (once inline, and once in the unabridged API) — the first definition looks like

/// Represents a user-defined key for providing context for encoding and decoding.
public struct CodingUserInfoKey : RawRepresentable, Hashable {
    typealias RawValue = String
    let rawValue: String
    init?(rawValue: String)
}

while the second looks like

/// Represents a user-defined key for providing context for encoding and decoding.
public struct CodingUserInfoKey : RawRepresentable, Hashable {
    typealias RawValue = String
    let rawValue: String
    init?(rawValue: String)
    init(_ value: String)
}

I think the non-failable init(_ rawValue🙂 was an accidental holdover from a previous revision.

Since RawRepresentable only requires the failable version (init?(rawValue🙂), we are at least fulfilling the contract of RawRepresentable. We can add the non-failable version, but I don't think it's worth the overhead of potential source breakage and having to carry it through further API review.

Is this preventing you from doing anything with the type?

[hamishknight]

@itaiferber It's not preventing any usage of the type, but having a failable initialiser that never fails feels pretty unsatisfactory. For any failable initialiser, the developer has to reason about the exact cases under which it returns nil, which shouldn't have to be the case here. It's worth noting that the fact that the initialiser never fails isn't mentioned in the documentation, I only found out by looking at the source. If we are going to leave the API as-is, I'd recommend at least updating the docs to note this.

It's also worth noting that there's an example in the evolution proposal that uses the non-failable init(_ value:):

public struct Person : Encodable {
    public static let codingUserInfoKey = CodingUserInfoKey("com.foocorp.person.codingUserInfoKey")

    public struct UserInfo {
        let shouldEncodePrivateFields: Bool
        // ...
    }

    func encode(to encoder: Encoder) throws {
        if let context = encoder.userInfo[Person.codingUserInfoKey] as? Person.UserInfo {
            if context.shouldEncodePrivateFields {
                // Do something special.
            }
        }

        // Fall back to default.
    }
}

So I do really think we should have at least an init(_ rawValue:). Such a convenience initialiser isn't un-common for String-key wrapper types, e.g UIApplicationLaunchOptionsKey. However, there are a few such types that don't, e.g NSLocale.Key. That being said, both types share a non-failable init(rawValue:) (and are both RawRepresentable), so it seems a shame for CodingUserInfoKey to not have that.

Although a quick Google search across GitHub repos shows that CodingUserInfoKey really doesn't seem to be used all that much, so maybe this isn't a big issue.

[itaiferber]

@hamishknight Fair! I tried to look search for how common `init?(rawValue🙂` vs. `init(rawValue🙂` is in the SDKs that ship with Swift, but GitHub's search functionality didn't differ between the queries. I think the easiest solution here would be to at least add `init(_ rawValue🙂`; I'll see about whether we can reasonably change `init?(rawValue🙂` to `init(rawValue🙂`.

[hamishknight]

@itaiferber Awesome, thanks 🙂

If it's of use to you, here's the result of a quick grep for both in /stdlib:

init(rawValue::

private/StdlibUnicodeUnittest/StdlibUnicodeUnittest.swift
19:    public init(rawValue: Int) {

public/core/OptionSet.swift
120:  init(rawValue: RawValue)

public/SDK/Dispatch/Block.swift
17: public init(rawValue: UInt) { self.rawValue = rawValue }

public/SDK/Dispatch/IO.swift
22:     public init(rawValue: UInt) { self.rawValue = rawValue }
29:     public init(rawValue: UInt) { self.rawValue = rawValue }

public/SDK/Dispatch/Queue.swift
29:     public init(rawValue: UInt64) { self.rawValue = rawValue }

public/SDK/Dispatch/Time.swift
32: fileprivate init(rawValue: dispatch_time_t) { 
100:    fileprivate init(rawValue: dispatch_time_t) {

public/SDK/Dispatch/Source.swift
104:        public init(rawValue: UInt) { self.rawValue = rawValue }
111:        public init(rawValue: UInt) { self.rawValue = rawValue }
121:        public init(rawValue: UInt) { self.rawValue = rawValue }
132:        public init(rawValue: UInt) { self.rawValue = rawValue }
139:        public init(rawValue: UInt) { self.rawValue = rawValue }

public/SDK/Foundation/JSONEncoder.swift
27:        public init(rawValue: UInt) {

public/SDK/Foundation/NSError.swift
606:    public init(rawValue: Int) {
1837:    public init(rawValue: Int) {
3279:  public init(rawValue: String) { self.rawValue = rawValue }

public/SDK/Foundation/NSStringEncodings.swift
21:    public init(rawValue: UInt) { self.rawValue = rawValue }

init?(rawValue::

public/core/Codable.swift.gyb
1077:  public init?(rawValue: String) {

public/core/CompilerProtocols.swift
126:  init?(rawValue: RawValue)

public/SDK/Dispatch/Dispatch.swift
88:     public init?(rawValue: qos_class_t) {

public/SDK/Foundation/NSError.swift
388:  public init?(rawValue: RawValue) {
418:  public init?(rawValue: RawValue) {

The only other ones that stands out among the failable ones are those in NSError.swift, but those are in an extension of an underscored protocol, so I would assume aren't meant to be used directly.

[itaiferber]

@hamishknight Interesting, thanks for looking through this! Looks like this might be worth fixing, then, for consistency. Although this is a small change, this will likely need to go through API review with other changes as well.

[itaiferber]

@swift-ci Create

[hamishknight]

@itaiferber No worries! By "API review", are we talking about a full blown round of Swift evolution, or just an internal review?

[itaiferber]

@hamishknight I'm not sure at the moment, but I think we're going to need to go through swift-evo for this, since CodingUserInfoKey is defined in the stdlib.