JSONEncoder implementation #1048

bubski · 2017-06-18T14:42:35Z

Resolves: https://bugs.swift.org/browse/SR-5195

This PR mirrors JSONEncoder implementation from SDK for Darwin, so that it's available on Linux.
swiftlang/swift#9005

platformConsistentCast

There are two major discrepancies between macOS and Linux.

On macOS JSONSerialization uses only NSNumber to represent numbers in json objects. On Linux however, standard Swift types like Double or Int are used.
Casting between NSNumber and Swift numeric types behaves very differently on macOS and Linux.

To keep matters consistent, I use func platformConsistentCast in JSONEncoder.swift instead of as until the above issues are resolved.

New swift compiler flag

-DTEST_TARGET
This flag is used when compiling TestFoundation sources.

Because @testable doesn't work on Linux, I had to add ConsistentCasting.swift to both Foundation and TestFoundation targets, to expose internal func platformConsistentCast for tests.

-DTEST_TARGET is used to only import Foundation in ConsistentCasting.swift if it's compiled as a part of TestFoundation target.

Mirrored files (with slight changes to resolve compilation errors on Linux):

JSONEncoder.swift from:
https://github.com/apple/swift/blob/c5ff1f2cac8da6a14330f4b033b94c7c926d2126/stdlib/public/SDK/Foundation/JSONEncoder.swift

Codable.swift from:
https://github.com/apple/swift/blob/012ea9373b5d2f72c59e16e18bd077c3ee86c745/stdlib/public/SDK/Foundation/Codable.swift

Data.swift (Data : Codable implementation) from:
https://github.com/apple/swift/blob/c40ba96328129d322a41afbd018b2a4081409e5e/stdlib/public/SDK/Foundation/Data.swift#L1759

Date.swift (Date : Codable implementation) from:
https://github.com/apple/swift/blob/012ea9373b5d2f72c59e16e18bd077c3ee86c745/stdlib/public/SDK/Foundation/Date.swift#L288

URL.swift (URL: Codable implementation) from:
https://github.com/apple/swift/blob/c5ff1f2cac8da6a14330f4b033b94c7c926d2126/stdlib/public/SDK/Foundation/URL.swift#L1210

NSError.swift (new error codes) from:
https://github.com/apple/swift/pull/9005/files#diff-a25f757c0d5c775e5b48e180b4a24efa

bubski · 2017-06-19T16:16:59Z

@swift-ci please test

parkera · 2017-06-19T16:38:45Z

Thanks for taking this on!

itaiferber · 2017-06-19T16:46:10Z

I'll be able to review this more in depth later today — thanks for bringing this over to Linux! At first glance, looks reasonable to me.

itaiferber · 2017-06-19T16:46:27Z

@phausler What do you think about the numeric casting abstractions here?

phausler · 2017-06-19T16:47:04Z

Foundation.xcodeproj/project.pbxproj

@@ -2778,7 +2808,7 @@
 				LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/../Frameworks @loader_path/../Frameworks";
 				LIBRARY_SEARCH_PATHS = "$(inherited)";
 				MACH_O_TYPE = mh_execute;
-				OTHER_SWIFT_FLAGS = "-DDEPLOYMENT_ENABLE_LIBDISPATCH -swift-version 3";
+				OTHER_SWIFT_FLAGS = "-DDEPLOYMENT_ENABLE_LIBDISPATCH -swift-version 3 -DTEST_TARGET";


why do we need to add this?

So that we import Foundation/import SwiftFoundation here
https://github.com/apple/swift-corelibs-foundation/pull/1048/files#diff-fa28084658370ec9069bda50577d8ab1R14
only when build for TestFoundation

platformConsistentCast is internal, I cannot expose it to TestFoundation, so needed to add this source file to TestFoundation target.

phausler · 2017-06-19T16:48:27Z

Foundation/ConsistentCasting.swift

+    return nil
+}
+
+protocol _PlatformConsistentCasting {


This should be encompassed in the Bridging.swift, also protocols are a bit slippery for scope - we should annotate this as internal

@phausler could you say a bit more why protocols are slippery for scope?

we have had issues where declarations accidentally leak out beyond the module of Foundation (however I haven't seen that since).

Even though internal is the default application of scope, it just seems to be good hygiene to make certain they are marked as such.

phausler · 2017-06-19T16:48:59Z

Foundation/ConsistentCasting.swift

+    }
+}
+
+extension Int8: _PlatformConsistentCasting {


We should pull the implementation from NSNumber.swift from the overlay to make it have parity

phausler · 2017-06-19T16:52:41Z

Overall this looks like the right direction, however I think we should try and figure out a consistent way of implementing the casting via using the exactly or the bridge methods.

I don't think it is out of scope to potentially change the JSONEncoder/JSONDecoder on Darwin to match if we can figure out a good way of implementing it so that we have parity for numeric types.

bubski · 2017-06-19T16:58:39Z

@phausler Thanks for the review.
I'll have time later today to take a deeper look into NSNumber.swift and Bridging.swift.

phausler · 2017-06-19T17:01:18Z

let me know if you need any help; I think we can accomplish this pretty easily via the _SwiftValue.fetch method which simulates as? casts and make this work for not just JSONEncoder but have it work better for all numeric bridges.

bubski · 2017-06-19T20:15:16Z

I can't really see how casting would benefit from using _SwiftValue.fetch.
Could you explain a bit more @phausler?

From what I see, _SwiftValue does rather, so to say, bridging than casting, between Swift and NS types.

Also, _SwiftValue.fetch returns Any. The way I see it, a generic function would be suited to do casting.

And, if I'm not mistaken, _SwiftValue revolves around _StructBridgeable and _ObjectBridgeable, which describe one-to-one bridging, but for casting we need to be able to have multiple output types, e.g. NSNumber can be casted to not only Int but also UInt, Double, Int8 etc.

I'd appreciate any guidance here.

phausler · 2017-06-19T20:46:21Z

So the problem is going to be recursive type conversions; e.g. Dictionaries that contain other collections. We will always have some mismatch of parity of some of the elements but I think we can get a 90% use case solution here with _SwiftValue.fetch.

Here is the idea:
We add a new flag to JSONSerialization to produce reference types instead of value types.
That will make the parser emit reference semantics and contain NSNumbers, NSStrings, NSDictionaries etc (which all of those types exist today in swift-corelibs-foundation as either simple wrappers like NSString as a wrapper for String, or CF types like NSNumber and CFNumber).

Since the numeric type accessors have type (bound to the bridgeable type) being sent in we can construct the requested type via the bridge method _conditionallyBridgeFromObjectiveC etc. That way when client code requests a Double or an Int8 they get the expected behavior.

This also means that we need to bring across the updated NSNumber.swift bridge in the overlay (which the entire intent of changing how that was bridged was done so specifically for Codable support)

The long-tail advantage here is that we can likely get better behavior out of JSONSerialization or PropertyListSerialization for developers by going down this route; ideally we should share as much code and behavior between the Darwin versions and the swift-corelibs version as possible.

If it would help I can knock out a rough draft of bringing the NSNumber changes over. I think that going down that route will aide ensuring the JSONEncoder to be as close as possible.

phausler · 2017-06-19T20:47:12Z

@parkera What is your weigh in on this? I know we have limited working schedule for things but I think it is pretty important that we get this right.

bubski · 2017-06-20T08:37:46Z

@phausler right, recursion would break…

As for JSONSerialization, I would expect it to be aligned with Darwin's one by default. In the end, it is supposed to be NSJSONSerialization mirror, so it should return the same data types, don't you think?
My thinking is, returning reference types should be opt-out rather than opt-in, if there is to be such a switch at all.
Am I wrong?

@phausler could you help me out and explain what overlay means?
I'm only now getting familiar and up to speed with the project, so forgive my basic questions.

parkera · 2017-06-20T17:37:00Z

@bubski I discussed this with @phausler some more and we had an idea which I think will simplify this PR quite a bit by removing the need for the casting. We can add an internal-only entry point to JSONSerialization to decode with an option set to prefer NSNumber output instead of Int, Float, Bool, and friends.

Then, when the encoder/decoder would have called platformConsistentCast it can instead still use as?, because the type returned from decoding is actually an NSNumber - and this cast will work across platforms.

The reason we decided to return the Swift numeric types from JSONSerialization in the first place was because of this same issue, that casting didn't work the same. We thought people would usually want the Swift types and not the Foundation types, so we were trying to remove the need to do some kind of Linux-specific as cast.

Taking a quick look at the JSON decoder, I think this option would be as simple as passing another option through to the parseValue function and using it to return NSNumber(value: whatever) instead of the built-in bool types and Swift numerics.

Hopefully this makes sense. Please let me know if you have any questions.

bubski · 2017-06-20T22:20:43Z

@parkera Yes, this makes sense and sounds like a plan 👍 I'll get around to it.
And thanks for the story behind types in JSONSerialization.

Is casting to/from Foundation types on Linux considered? If there's any useful reading around that subject that you know, please share.

parkera · 2017-06-21T16:59:27Z

I'd love to support using as to "bridge-cast", but not everyone agrees that this functionality should be available on Linux. In my mind, it's a key part of the cross-platform source compatibility story. But in any case, it's not happening in the short term so we should assume it won't exist.

bubski · 2017-06-23T14:23:53Z

@parkera Followed your advise and pushed changes.
cc: @phausler @itaiferber

phausler · 2017-06-23T15:10:53Z

Foundation/NSJSONSerialization.swift

@@ -23,6 +23,7 @@ extension JSONSerialization {
        public static let mutableContainers = ReadingOptions(rawValue: 1 << 0)
        public static let mutableLeaves = ReadingOptions(rawValue: 1 << 1)
        public static let allowFragments = ReadingOptions(rawValue: 1 << 2)
+        internal static let useReferenceNumericTypes = ReadingOptions(rawValue: 1 << 3)


it might be a good idea to use some other value than 1 << 3 since it would be quite likely that we could add a public value that uses that definition.

my suggestion is use a higher value...

How high would you recommend?

Something like 1 << 12 or 1 << 15 should suffice

We're unlikely to need to add that many options; in the future, we can modify this value if need be, since it's just internal here.

…nto json-encoder # Conflicts: # Foundation.xcodeproj/project.pbxproj # Foundation/Data.swift

phausler · 2017-06-23T22:46:03Z

As a follow up task; I know Itai is working on some other additional changes to the encoders. We should figure out a way to be able to copy/paste the code easily between the two implementations. That way when changes are made in one location we can just do a simple duplication instead of needing to re-port the changes over again.

…nto json-encoder

bubski · 2017-06-26T19:19:27Z

@itaiferber @phausler should I make any more changes in this PR?

phausler · 2017-06-26T19:48:10Z

I think it looks good as a starting point, lets revisit the follow up task once Itai lands some of the other updates.

I will let Itai do the final review and merge.

itaiferber

LGTM overall! We'll probably want to start porting over some Codable implementations from other Foundation types (swiftlang/swift#9715, swiftlang/swift#10404).

Also, I'm not sure how JSONSerialization handles Decimal if at all, but we'll probably need to apply swiftlang/swift#10553 here too.

@phausler Good to merge, though; these changes can come in future PRs.

itaiferber · 2017-06-26T19:49:09Z

Foundation/Codable.swift

+
+// Adding the following extensions to EncodingError and DecodingError allows them to bridge to NSErrors implicitly.
+
+fileprivate let NSCodingPathErrorKey = "NSCodingPath"


Since we don't have error bridging in Linux, I think this whole section (short of the helper methods on DecodingError) can be removed.

bubski · 2017-06-26T20:46:29Z

Thanks @itaiferber and @phausler.

I'll continue working around the things you mentioned in coming PRs.

parkera · 2017-06-26T21:05:47Z

@swift-ci test and merge

ianpartridge · 2017-07-06T09:42:27Z

Hi @bubski - thank you so much for working on this! Do you have any news on adding Codable conformances to the other Foundation types?

bubski · 2017-07-06T09:54:06Z

@ianpartridge credit goes to @itaiferber . He's done all the heavy lifting.

Yes, I'm porting other types to Foundation, but hit some bugs which blocked me.

Here's one: #1092
I couldn't test CharacterSet : Codable properly without isEqual working, so I need that to be merged first.

My approach in #1092 might not be the best though. I know that @phausler is reworking some NSCF stuff which might solve this problem as well.

ianpartridge · 2017-07-06T10:03:00Z

Sounds great - glad to hear you're making progress. Is there a public branch you're pushing to, so I can follow along?

bubski · 2017-07-06T10:04:36Z

Sure @ianpartridge https://github.com/bubski/swift-corelibs-foundation/tree/codable
But be warned, it's all WIP and messy :)

JSONEncoder implemented

eac219a

parkera requested a review from itaiferber June 19, 2017 16:38

itaiferber requested a review from phausler June 19, 2017 16:45

phausler requested changes Jun 19, 2017

View reviewed changes

JSONEncoder now using JSONSerialization with reference numeric types

f90dacf

bubski force-pushed the json-encoder branch from 4a68c3f to f90dacf Compare June 23, 2017 14:22

phausler reviewed Jun 23, 2017

View reviewed changes

bubski added 2 commits June 23, 2017 19:01

Changed value of useReferenceNumericTypes flag

6147753

Merge branch 'master' of github.com:apple/swift-corelibs-foundation i…

1b36a51

…nto json-encoder # Conflicts: # Foundation.xcodeproj/project.pbxproj # Foundation/Data.swift

phausler approved these changes Jun 23, 2017

View reviewed changes

bubski added 2 commits June 25, 2017 01:17

Fixed strict bool matching in JSONEncoder

43e0e4a

Merge branch 'master' of github.com:apple/swift-corelibs-foundation i…

b23ca90

…nto json-encoder

itaiferber approved these changes Jun 26, 2017

View reviewed changes

swift-ci merged commit d54fcb9 into swiftlang:master Jun 26, 2017

ianpartridge mentioned this pull request Jul 7, 2017

JSONSerialization: add WritingOptions.sortedKeys #1102

Merged

swift-ci mentioned this pull request Jun 18, 2017

[SR-5195] Codable/Decodable should be available on Linux swiftlang/swift#47771

Open

NoahKamara mentioned this pull request Nov 7, 2022

JSON decoding issues supabase-community/storage-swift#7

Open


		// Adding the following extensions to EncodingError and DecodingError allows them to bridge to NSErrors implicitly.

		fileprivate let NSCodingPathErrorKey = "NSCodingPath"

JSONEncoder implementation #1048

JSONEncoder implementation #1048

Uh oh!

Conversation

bubski commented Jun 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

platformConsistentCast

New swift compiler flag

Mirrored files (with slight changes to resolve compilation errors on Linux):

Uh oh!

bubski commented Jun 19, 2017

Uh oh!

parkera commented Jun 19, 2017

Uh oh!

itaiferber commented Jun 19, 2017

Uh oh!

itaiferber commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phausler commented Jun 19, 2017

Uh oh!

bubski commented Jun 19, 2017

Uh oh!

phausler commented Jun 19, 2017

Uh oh!

bubski commented Jun 19, 2017

Uh oh!

phausler commented Jun 19, 2017

Uh oh!

phausler commented Jun 19, 2017

Uh oh!

bubski commented Jun 20, 2017

Uh oh!

parkera commented Jun 20, 2017

Uh oh!

bubski commented Jun 20, 2017

Uh oh!

parkera commented Jun 21, 2017

Uh oh!

bubski commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phausler commented Jun 23, 2017

Uh oh!

bubski commented Jun 26, 2017

Uh oh!

phausler commented Jun 26, 2017

Uh oh!

itaiferber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bubski commented Jun 26, 2017

Uh oh!

parkera commented Jun 26, 2017

Uh oh!

ianpartridge commented Jul 6, 2017

Uh oh!

bubski commented Jul 6, 2017

bubski commented Jun 18, 2017 •

edited

Loading