Foundation Encoders

[itaiferber]

This implements the JSONEncoder, JSONDecoder, PropertyListEncoder, and PropertyListDecoder types proposed by SE-0167.

This pulls all Foundation-specific work out of #8124 for separate integration.

This PR depends on #9004 and should NOT be merged until #9004 is complete and merged.
rdar://problem/30471969
rdar://problem/30472028

[DougGregor]

@swift-ci please test

[itaiferber]

@DougGregor This isn't quite ready to be merged yet — I've got unit tests incoming either later this evening or early tomorrow. But when they make it I'll test and ping you if ready to merge

[swift-ci]

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - a1345ce4eceb35f1f76add66befdc6e189bd0d8c
Test requested by - @DougGregor

[DougGregor]

Ok!

[swift-ci]

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - a1345ce4eceb35f1f76add66befdc6e189bd0d8c
Test requested by - @DougGregor

[itaiferber]

@swift-ci Please test

[swift-ci]

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - a1345ce4eceb35f1f76add66befdc6e189bd0d8c
Test requested by - @itaiferber

[swift-ci]

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - a1345ce4eceb35f1f76add66befdc6e189bd0d8c
Test requested by - @itaiferber

[DougGregor]

@swift-ci please clean test

[swift-ci]

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - d177bdf
Test requested by - @DougGregor

[tkremenek]

@swift-ci clean test macOS

[itaiferber]

@DougGregor @tkremenek I think the linker errors are legitimate, no? The box types that I added were marked as fileprivate where I think they should actually be internal (I looked at the box types for Sequence, Iterator, etc. and they are all @_fixed_layout @_versioned internal). I'm about to push that change and test

[tkremenek]

@itaiferber great point; I wasn't certain if those failures were genuine or not. I'll cancel the job I just initiated.

[swift-ci]

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - d177bdf
Test requested by - @tkremenek

[itaiferber]

@swift-ci Please test

[swift-ci]

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - d177bdf
Test requested by - @itaiferber

[swift-ci]

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - d177bdf
Test requested by - @itaiferber

[itaiferber]

🎉 @DougGregor @tkremenek When either of you gets a chance, do you mind merging this in please? Thanks!

[itaiferber]

@tkremenek Thanks!

[DougGregor]

Congrats to @itaiferber for getting this landed

[itaiferber]

Thanks, @DougGregor! Looking forward to feedback when people start using it. :)

[norio-nomura]

@itaiferber I have another question.
What should codingPath contain on processing super and nestedContainer?
Current implementation seems not to contain both of super and key for nestedContainer.

[itaiferber]

@norio-nomura This is another bug: codingPath on nested containers and referencing encoders should include the codingPaths of their parents. I have a branch with a fix (https://github.com/itaiferber/swift/tree/foundation-codingpath-fixes) but haven't gotten the chance to put it up as a PR yet. Will do in the next few days.

[norio-nomura]

@itaiferber Thank you for clarifying. 🙏
FYI, I'm trying to implement YAMLEncoder and YAMLDecoder for SE-0166 at jpsim/Yams#43.
I'll check your branches to fix codingPath on that.

[Dimillian]

@itaiferber Is Codable support for NSKeyedArchiver part of another PR ? It's in the SE but can't seems to find it in code for now.

[itaiferber]

@Dimillian That integration hasn't been completed yet. This will be coming in in a future PR.

[bubski]

@itaiferber JSONDecoder & JSONEncoder are not yet available on Linux, or am I missing something?

[CodaFi]

@bubski How recent a snapshot of Swift are you using?

[bubski]

@CodaFi I tried both
swift-4.0-DEVELOPMENT-SNAPSHOT-2017-06-06-a-ubuntu16.04
and
swift-4.0-DEVELOPMENT-SNAPSHOT-2017-06-07-a-ubuntu16.04

[CodaFi]

It seems that because this has landed in the Darwin SDK that it will need to explicitly be put into Corelibs Foundation for it to be usable on Linux. The proposal does not make mention of platform lock-in, so I think this will need to be mirrored over.

[itaiferber]

@bubski As @CodaFi mentions, this will need to be introduced into swift-corelibs-foundation in order to be available on Linux. The code as-is should be easy to transfer over to there without requiring many changes (if at all), but we haven't gotten the chance to (and it seems that no one from the community has either).

[CodaFi]

I've filed SR-5195 about this.

[bubski]

@itaiferber I started hacking around the idea
http://github.com/bubski/JSONEncoderLinux
and managed to get it running on Linux.

But I don't have a good swift-corelibs-foundation dev environment to do proper implementation (that's why I copy-pasted some internals to expose them).

I'll try to clean it up and make a PR to swift-corelibs-foundation so it's easier for someone to verify.

[itaiferber]

@CodaFi Thanks for that!

[itaiferber]

@bubski Cool, thanks! The one annoyance will be keeping these things in sync. There are still some outstanding changes that will be landing in JSONEncoder/Decoder that will need to be ported over as well.

[bubski]

@itaiferber Yea, I figured. I'm open to any suggestions.
It's also tricky because of discrepancies in the way Foundation types casting works on Linux (or rather the fact that it doesn't) together with JSONSerialization producing different types on Linux vs macOS.

[itaiferber]

@bubski Yep, there are a few places where I've noted (in the Darwin implementation) places which may have this discrepancy. I think overall, it might be worth updating JSONSerialization to follow suit, but that will require some further review which we simply haven't had time for.

As for keeping things in sync, there's not much at the moment, unfortunately. We struggle with this ourselves. I think we need a general process/solution for improving this.

[Dimillian]

@itaiferber Is having a default value + support optional Codable property something planed?

Let me explain, you define a var as such in a Codable object:

var progressMax: Int = 0

But then if you decode your object with a JSON that does not contain progressMax it'll throw a missing key error. Which make sense, but it also make sense to have a default, hardcoded value, that is overwritten by a JSON defined one if available.

[itaiferber]

@Dimillian It's possible to support, but I don't know if that's something we'd want to do out of the box. It's impossible for us to tell whether the default value is assigned for the benefit of other initializers, or whether you meant for it to be a default value if the JSON couldn't be read. I don't know if we can read into the intent there — did you want us to assign a default, or were you expecting the error? No real way for us to know.

[Dimillian]

@itaiferber In this specific case, the JSON can be different and still represent the same object once decoded.
If you have a model defined as such

class Book: Codable {

var title: String
var author: String
var progress: Int = 0

}

The JSON will always have the title and author value defined, but the progress value will be in only in some cases (let's say it's an API and the server control it).
So it make sense in our case to work with non optional value, because we decided that when progress is not present, it should be equal to 0.

Is there a way in current implementation to represent that?

But I understand that you can't really define our intent at a lower level, and it make sense for other use case to throw an error.

[itaiferber]

@Dimillian You've got a few options here (of course, not all perfect):

1. Make progress an Int? — this will set it to nil by default instead of throwing an error, and you can treat nil as "no progress". Of course, this is a subpar experience, but it's the easiest solution.
2. Make progress a custom Progress type which can initialize from nil and has an Int raw value. Also not optimal, but it will work.
3. Since you're looking for custom behavior, you can always give a custom init(from:) which will call decodeIfPresent(_:forKey:) instead of decode(_:forKey:) and give a default value there of 0.

None of these solutions are great, but they are at least explicit. I'll think on this some more; perhaps there is a cleaner way to improve the experience of doing this.

[Dimillian]

@itaiferber Actually, your 1. is what we did. It's a bit subpar because we have to unwrap it each time yes. But that's acceptable. In our case we have a lot of var like that now, as we converted NSCoding/Reflection code to Codable. And we could write custom decoder and encoder but in that case we lose a bit of the benefit of Codable.
I think this should be an option in the decoding strategy of the JSONDecoder. Like a default value for optional decoding strategy, so I can set it to ignore missing JSON value if default a value is set in the class. Maybe I'll take a look at it.