Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
hamishknight opened this issue Jan 13, 2018 · 9 comments · May be fixed by #63172
Open

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

hamishknight opened this issue Jan 13, 2018 · 9 comments · May be fixed by #63172
Assignees
@itaiferber
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. Codable Area → standard library: `Codable` and co. standard library Area: Standard library umbrella

Comments

@hamishknight
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Contributor

@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
Copy link
Collaborator Author

@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
Copy link
Contributor

@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
Copy link
Collaborator Author

@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
Copy link
Contributor

@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
Copy link
Contributor

@swift-ci Create

@hamishknight
Copy link
Collaborator Author

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

@itaiferber
Copy link
Contributor

@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.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@benrimmington benrimmington linked a pull request Mar 1, 2023 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@itaiferber itaiferber

Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. Codable Area → standard library: `Codable` and co. standard library Area: Standard library umbrella
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[SE-NNNN] Simplify internals of Codable-related APIs benrimmington/swift
2 participants
@itaiferber @hamishknight