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

Codable Range proposal #915

Merged
merged 5 commits into from Dec 14, 2018

Conversation

Projects
None yet
7 participants
@airspeedswift
Copy link
Member

airspeedswift commented Sep 27, 2018

Full proposal version of #914

@dlbuckley

This comment has been minimized.

Copy link

dlbuckley commented Nov 12, 2018

@airspeedswift is there anything else that I can do to help push this further along?

@dlbuckley

This comment has been minimized.

Copy link

dlbuckley commented Nov 20, 2018

@airspeedswift @tkremenek @benrimmington I assume that this has now missed being added to Swift 5 due to the branching now being completed?

@benrimmington

This comment has been minimized.

Copy link
Collaborator

benrimmington commented Nov 20, 2018

@dlbuckley The proposal is described as a "purely additive change", so my guess would be Swift 5.1, but that decision is nothing to do with me.

@airspeedswift Conditional conformance collisions might be a problem.

@airspeedswift

This comment has been minimized.

Copy link
Member Author

airspeedswift commented Nov 28, 2018

Hi @dlbuckley – while this is additive, we would still consider it post branch because Range really should be Codable as a batteries-included kind of thing, and having to gate codability with if #available would be a real shame.

The only issue is, as people have brought up, that others have now written their own conditional extensions. Worse, they might have used keyed containers with different keys and there's a risk of data corruption if they write with one and read with the other.

It feels like this might be solvable by having a slightly more advanced decoder that first tries unkeyed (the more appropriate form IMO) and then if that throws, catch and retry keyed. I'm not sure how much flexibility Codable has for probing an encoding like this though. It needs a bit of experimentation. If that can be done quickly, we could then run the review.

cc @moiseev who was also looking at this.

@dlbuckley

This comment has been minimized.

Copy link

dlbuckley commented Nov 28, 2018

@airspeedswift other people having written their own implementations of this is indeed an issue when updating to a newer version of the language with this included.

We actually had an issue recently where we were encoding values of Measurement type, and while the encoded data was correct, it was a pain to query in mongo. What we ended up doing was wrapping up all uses of Measurement in the Encode/Decode methods where it was used. This allowed us to use our own method of encoding and decoding Measurement types.

While that example was a work around, I do wonder if this highlights an extension point in Encoder/Decoder that allows for registering custom encode/decode methods for a specific type which then ignores the default Codable conformance of the type. Maybe this is something that @itaiferber can chime in with here?

As for the suggestion of first trying to decode the type with an unkeyed unarchiver and then trying again with the keyed unarchiver; I think there are a number of risks with that. Firstly, how do you know which keys that are being used? In my first version I used keys that helped to distinguish between all of the different Range types, but I know other extensions have just kept to the names of the properties (upperBound and lowerBound). The biggest problem I see is that if the data is read in from the keyed format, and then saved out as the unkeyed format, it could break whatever is relying on that data.

I think we are now past the point where this can be added in without causing a number of headaches. We just need to decide if causing the headaches is worth it, or it's best to leave Swift without the 'batteries-included' implementation of the Range types.

@parkera

This comment has been minimized.

Copy link
Member

parkera commented Nov 28, 2018

We're still in early days of Swift, so I think it would be a mistake to just give up on doing this permanently.

With respect to the extension point on Encoder for custom types, it was always our view that the way to do this in general was by making a wrapper struct for your type. I think it's still pretty important to the design of Codable to preserve encapsulation as much as possible. We made exceptions for a few types in various encoders like JSONEncoder because of the prevailing conventions of data encoded in those formats (e.g., URL as a single String, in that case, or Date stored as ISO8601).

@itaiferber

This comment has been minimized.

Copy link
Contributor

itaiferber commented Nov 28, 2018

As far as existing extensions go, I agree with @parkera. It's never been safe to extend others' types with Codable, and will never be safe: the only right thing to do is to create wrapper structs around others' types which you control (@dlbuckley mentions with Measurement). This is something I've mentioned a lot in discussion, but it's not something we can really force folks to do. (We could technically prevent extension Foo : Encodable and extension Foo : Decodable if Foo is defined outside of the current module, but not without a ton of breakage, and kludge in the compiler.)

So I think we need to consider to what extent we are worried about extensions like this. No matter what we do, adopting Codable on the Range types is going to be a source-compatibility issue for others. I personally thing that the right thing for them to do at that point is to resolve the source-breakage by going and wrapping their existing Codable ranges with the wrapper types that maintain their format. Or, if they really care to, they can keep their overriding implementations of init(from:) and encode(to:) and those keep working for them within their modules. (I would say this is brittle and pretty wrong, but it is at least easy.)

As for allowing the defeat of Codable implementations in general as an extension point on Encoder/Decoder — we discussed this during the initial design but decided pretty strongly against it; ignoring the design constraints of now forcing this capability on all Encoders and Decoders, there's a fundamental encouragement to breaking encapsulation that that facility offers. We break this already for some types with JSONEncoder in highly targeted cases, but even that feels wrong in a sense. (The philosophically-consistent solution might be to instead offer Codable wrappers for these types, like ISO8601CodableDateWrapper or something.)

In any case, I don't think we can start guessing at others' representations for Range and the like — as flexible as Codable is, we can't guess at keys used in keyed containers, or how to decode something like [12, 57, 96, 22].

@airspeedswift

This comment has been minimized.

Copy link
Member Author

airspeedswift commented Nov 28, 2018

We're still in early days of Swift, so I think it would be a mistake to just give up on doing this permanently.

I agree, and there's also a significant benefit to doing it in 5.0 to avoid having to gate on OS version to use it if we add it in the future.

I think a best efforts approach would be reasonable here. It's actually perfectly acceptable IMO to break code that had conformed std lib types to Codable manually. It's not OK to conform a library's types to that same library's protocols for exactly this reason. It's just nice of us not to if possible, depending on the combination of circumstances.

@moiseev

This comment has been minimized.

Copy link
Member

moiseev commented Nov 29, 2018

@dlbuckley

As for the suggestion of first trying to decode the type with an unkeyed unarchiver and then trying again with the keyed unarchiver; I think there are a number of risks with that. Firstly, how do you know which keys that are being used?

Since we're talking about the range, where the lower bound should be less than or equal than the upper bound, we don't really care about the exact values of the keys. It should be possible to just obtain all the keys, try them, and then assign the smaller value to lower bound. I'm not too comfortable with Codable to know for sure if it's at all possible to obtain all the values from a keyed container without specifying the keys, though.

@dlbuckley

This comment has been minimized.

Copy link

dlbuckley commented Dec 3, 2018

Since we're talking about the range, where the lower bound should be less than or equal than the upper bound, we don't really care about the exact values of the keys. It should be possible to just obtain all the keys, try them, and then assign the smaller value to lower bound. I'm not too comfortable with Codable to know for sure if it's at all possible to obtain all the values from a keyed container without specifying the keys, though.

Unfortunately it's not possible to just obtain all of the keys like that. Due to the nature of Codable you need to specify the keys you expect when getting the keyed container. Using the allKeys property on a keyed container only returns all of the keys you give it, not the other way around.

I've updated the PR with a change that was requested by @moiseev last week which hopefully mitigates his concerns around decode error handling.

@itaiferber thanks for chiming in. I totally agree with you and @parkera regarding the reasoning around protecting encapsulation here. I was simply thinking out loud on if there was a way around it to be nice to people who have already gone down the avenue of their own implementations.

@itaiferber

This comment has been minimized.

Copy link
Contributor

itaiferber commented Dec 3, 2018

Unfortunately it's not possible to just obtain all of the keys like that. Due to the nature of Codable you need to specify the keys you expect when getting the keyed container. Using the allKeys property on a keyed container only returns all of the keys you give it, not the other way around.

@dlbuckley Just FYI, that's not strictly true — you can get all of the keys in a given keyed container by getting that keyed container with a key type that allows any String or Int value. This is how Dictionary decodes itself, using _DictionaryCodingKey

@dlbuckley

This comment has been minimized.

Copy link

dlbuckley commented Dec 4, 2018

Unfortunately it's not possible to just obtain all of the keys like that. Due to the nature of Codable you need to specify the keys you expect when getting the keyed container. Using the allKeys property on a keyed container only returns all of the keys you give it, not the other way around.

@dlbuckley Just FYI, that's not strictly true — you can get all of the keys in a given keyed container by getting that keyed container with a key type that allows any String or Int value. This is how Dictionary decodes itself, using _DictionaryCodingKey

Ah ok, every day is a school day 😁

Update proposals/0000-codable-range.md
Co-Authored-By: airspeedswift <airspeedswift@users.noreply.github.com>
@tkremenek

This comment has been minimized.

Copy link
Member

tkremenek commented Dec 14, 2018

This proposal is now going to be run through a review. I will merge and patch up the document for review.

Given the updates @airspeedswift and @moiseev have done on this proposal as well, I think it makes sense to add them as proposal authors as well to address any questions related to technical concerns and data compatibility issues.

@tkremenek tkremenek merged commit 1c23fd1 into apple:master Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment