swiftlang / swift

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

[SR-8649] Conform Range to Codable when Bound conforms to Codable #51164

Open aciidgh opened 6 years ago

aciidgh commented 6 years ago
Previous ID SR-8649
Radar rdar://problem/36460457
Original Reporter @aciidb0mb3r
Type Bug
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 2 | |Component/s | Standard Library | |Labels | Bug, Codable | |Assignee | None | |Priority | Medium | md5: dc1975fb85524422eb8278fd8aed41fc

Issue Description:

It is possible to conform Range to Codable when Bound is Codable:

extension Range: Codable where Bound: Codable {

    private enum CodingKeys: CodingKey {
        case lowerBound
        case upperBound
    }

    public init(from decoder: Decoder) throws {

        let container = try decoder.container(keyedBy: CodingKeys.self)

        let lowerBound = try container.decode(Bound.self, forKey: .lowerBound)
        let upperBound = try container.decode(Bound.self, forKey: .upperBound)

        self.init(uncheckedBounds: (lower: lowerBound, upper: upperBound))
    }

    public func encode(to encoder: Encoder) throws {

        var container = encoder.container(keyedBy: CodingKeys.self)

        try container.encode(self.lowerBound, forKey: .lowerBound)
        try container.encode(self.upperBound, forKey: .upperBound)
    }
}

I think this conformance should be provided by the standard library.

itaiferber commented 6 years ago

+1 to this — we can likely go through parts of the stdlib and now conditionally conform more things that are useful.
One implementation note: I would not recommend using init(uncheckedBounds🙂 in the implementation as there is no guarantee that the bounds are valid (it's trivial to modify the payload to do this).

We should do this for the other variants (ClosedRange, PartialRangeFrom, PartialRangeThrough, PartialRangeUpTo), and maybe insert markers to distinguish between them. (i.e. if you encode a Range, you might not want to be able to accidentally decode it as a ClosedRange as the lower and upper bounds look identical but you'd get off-by-one errors)

swift-ci commented 6 years ago

Comment by Dale Buckley (JIRA)

@itaiferber I was thinking about what you suggested with the markers and I don't think they are needed if we name the keys correctly. If you think about server side implementation of this that yields a JSON document that's sent back to a client, then there needs to be an obvious way to distinguish what the `lowerBound` and `upperBound` actually relates to.

So for `Range` you would have the keys `from` and `upTo` and `ClosedRange` would be something like `from` and `through`. A subtile difference, but enough to make it more obvious to the consumer of a JSON document how the data is intended to be used and for the decoder to distinguish the difference.

Type

lowerBound

upperBound

Range

{code:java}
"from"

swift-ci commented 6 years ago

Comment by Dale Buckley (JIRA)

I've started a PR for this feature here: https://github.com/apple/swift/pull/19532

itaiferber commented 6 years ago

dlbuckley (JIRA User) That works equivalently, yes. Thanks for starting a PR! However, @airspeedswift, this should be going through Swift Evolution, yeah? Or are we considering this a bug fix?

swift-ci commented 6 years ago

Comment by Dale Buckley (JIRA)

@itaiferber I didn't know whether it had to go through swift evolution or not. I started a discussion on the forum here a little while ago and there wasn't any real decision if it was a bug or enhancement that needed a proposal or not.

Anyway if it needs a proposal I will go that route, I just need some clarification one way or the other 🙂