Port JSON key strategy to swift-corelibs-foundation #1347

parkera · 2017-11-29T17:50:38Z

This is a work in progress branch.

First step is to bring JSONEncoder.swift up to date with changes to the file in the overlay before the key strategy changes landed.

parkera · 2017-11-29T17:50:46Z

@swift-ci please test

parkera · 2017-11-29T17:51:29Z

@itaiferber I put two sections of code in an #if block that look to me like they are intentionally different, at least for the time being.

ianpartridge · 2017-11-29T18:12:18Z

Nice. I wonder if we could use the DEPLOYMENT_RUNTIME_SWIFT define for the differences, then share an identical file in the overlay?

parkera · 2017-11-29T18:18:28Z

Yah that may be possible. @itaiferber and I believe we have a few errors lurking in here since there are some as casts. Maybe we can wrap those.

itaiferber

Looks good overall, just need to remove the parts that don't bridge over correctly to Linux.

itaiferber · 2017-11-29T18:27:21Z

Foundation/JSONEncoder.swift

    }

    public mutating func nestedContainer<NestedKey>(keyedBy keyType: NestedKey.Type, forKey key: Key) -> KeyedEncodingContainer<NestedKey> {
        let dictionary = NSMutableDictionary()
-        self.container[key.stringValue._bridgeToObjectiveC()] = dictionary


We should keep _bridgeToObjectiveC() here

itaiferber · 2017-11-29T18:27:29Z

Foundation/JSONEncoder.swift

    }

    public mutating func nestedUnkeyedContainer(forKey key: Key) -> UnkeyedEncodingContainer {
        let array = NSMutableArray()
-        self.container[key.stringValue._bridgeToObjectiveC()] = array


itaiferber · 2017-11-29T18:28:23Z

Foundation/JSONEncoder.swift

+
+    // This method is called "box_" instead of "box" to disambiguate it from the overloads. Because the return type here is different from all of the "box" overloads (and is more general), any "box" calls in here would call back into "box" recursively instead of calling the appropriate overload, which is not what we want.
+    fileprivate func box_<T : Encodable>(_ value: T) throws -> NSObject? {
+        if T.self == Date.self || T.self == NSDate.self {


All of these casts will fail on Linux because of bridging. We can keep the behavior by turning all of the NS cases into actual conversions

itaiferber · 2017-11-29T18:31:01Z

Foundation/JSONEncoder.swift

@@ -1750,6 +1750,25 @@ extension _JSONDecoder {
    fileprivate func unbox(_ value: Any, as type: Bool.Type) throws -> Bool? {
        guard !(value is NSNull) else { return nil }

+#if false


The #if falsees are good candidates for converting to DEPLOYMENT_RUNTIME_SWIFT

itaiferber · 2017-11-29T18:31:25Z

Foundation/JSONEncoder.swift

    }

    fileprivate func unbox<T : Decodable>(_ value: Any, as type: T.Type) throws -> T? {
        let decoded: T
-        if T.self == Date.self {
+        if T.self == Date.self || T.self == NSDate.self {


Same here — all of the || T.self == ... lead to as!-casts which are not valid on Linux

parkera · 2017-11-29T18:48:39Z

@swift-ci smoke test

parkera · 2017-11-29T18:50:05Z

@swift-ci please test

parkera · 2017-11-29T21:01:51Z

@swift-ci please test

itaiferber · 2017-11-29T21:51:16Z

@swift-ci Please test

parkera · 2017-11-29T23:47:33Z

@swift-ci please test

alblue · 2017-11-30T10:55:39Z

Looks like this failed with a core dump:

Test Case 'TestJSONEncoder.test_encodingTopLevelSingleValueEnum' started at 2017-11-30 00:49:44.323
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift/utils/build-script-impl: line 264: 26232 Illegal instruction     (core dumped) "$@"
/home/buildnode/jenkins/workspace/swift-corelibs-foundation-PR-Linux/swift/utils/build-script: fatal error: command terminated with a non-zero exit status 132, aborting

parkera · 2017-12-04T00:50:13Z

Fixed the core dump (we needed Codable.swift changes too), but have some bridging failures to figure now now.

alblue · 2017-12-06T07:19:20Z

@swift-ci please test

parkera · 2017-12-06T17:24:58Z

I already know this fails. Just need some time to come back to this and sort out the bridging issues.

Andrewangeta · 2018-03-20T22:23:02Z

@parkera Any updates on this? I'm hoping this makes into 4.1 keyDecodingStragety is a really nice API and cleans up so much code.

parkera · 2018-03-21T20:28:07Z

Unfortunately I got stuck on this one, having trouble figuring out why the tests don't pass after merging in the prerequisite changes from the Swift overlay version.

Lukasa · 2018-03-22T17:00:05Z

So in case it helps, this seems to be a problem initially isolated to the decoder. Here's basically the smallest repro I can give you:

import Foundation

struct Test: Codable {
    var field: Int
}

let json = "{\"field\": 5}".data(using: .utf8)!
let decoder = JSONDecoder()
let result = try decoder.decode(Test.self, from: json)
print(result.field)

In Swift 4.0, this works fine and prints 5, as expected. In Swift 4.1, it explodes with

typeMismatch(Swift.Int, Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "field", intValue: nil)], debugDescription: "Expected to decode Int but found Int instead.", underlyingError: nil))

This seems to be related to the underlying parsing producing an Int instead of an NSNumber. As identified before, this is likely a bridging issue, and of the two of us I suspect you're vastly better equipped to go further with this than I am @parkera.

parkera · 2018-03-22T21:50:44Z

@itaiferber I know we are still talking about the returned types from JSONSerialization in a few other PRs, but I don't recall any changes here for 4.1 specifically. Do you?

itaiferber · 2018-03-22T21:56:17Z

None that I've seen; I can't find any PRs that changed JSONSerialization in Swift 4.1 on here either (but I'll keep looking). @Lukasa What platform are you on? Which toolchain are you running there?

Lukasa · 2018-03-23T08:44:05Z

@itaiferber Sorry, I could really have made that clearer. That's Ubuntu 16.04, using a hand-compiled Swift toolchain from the swift-4.1-DEVELOPMENT-SNAPSHOT-2018-03-21-a tag, with a Foundation branched from that tag with this patch merged on top of it.

If I just use the swift-4.1-DEVELOPMENT-SNAPSHOT-2018-03-21-a tag directly without this patch, the above test passes just fine. So it's this patch that is affecting the return type.

parkera · 2018-03-23T16:37:24Z

Ah, ok.

Andrewangeta · 2018-03-27T13:15:42Z

Any updates on this issue? @parkera

_{Sent with GitHawk}

gperdomor · 2018-06-04T19:38:59Z

Any update on this?

millenomi · 2018-06-04T20:03:51Z

We are dealing with some bridging landing fallout and WWDC. My plan is to look at this starting late this week, unless someone else can.

gperdomor · 2018-07-13T15:02:14Z

We can expect this merged in swift 4.2? :(

alblue · 2018-08-07T12:03:53Z

@parkera can you rebase this so that we can run the tests again, or close it if it has gone stale?

parkera · 2018-08-08T19:39:09Z

I'm going to close this, since I think #1561 should cover it.

parkera requested a review from itaiferber November 29, 2017 17:51

itaiferber suggested changes Nov 29, 2017

View reviewed changes

parkera force-pushed the parkera/json_key_strategy_impl branch from e6b2667 to a177ddc Compare November 29, 2017 18:47

Bring JSONEncoder.swift up-to-date with previous changes from overlay

22dace8

parkera force-pushed the parkera/json_key_strategy_impl branch from a177ddc to 22dace8 Compare November 29, 2017 21:01

itaiferber approved these changes Nov 29, 2017

View reviewed changes

Update Codable.swift to match overlay

66cd0fd

parkera mentioned this pull request Apr 12, 2018

Address exclusivity warnings in JSONEncoder #1512

Merged

saiHemak mentioned this pull request May 23, 2018

Port JSONEncoder overlay changes to swift-corelibs-foundation #1561

Merged

parkera closed this Aug 8, 2018

itaiferber mentioned this pull request Mar 19, 2018

[SR-7180] Using JSONDecoder's keyDecodingStrategy doesn't compile on linux #4041

Closed

Port JSON key strategy to swift-corelibs-foundation #1347

Port JSON key strategy to swift-corelibs-foundation #1347

Uh oh!

Conversation

parkera commented Nov 29, 2017

Uh oh!

parkera commented Nov 29, 2017

Uh oh!

parkera commented Nov 29, 2017

Uh oh!

ianpartridge commented Nov 29, 2017

Uh oh!

parkera commented Nov 29, 2017

Uh oh!

itaiferber left a comment

Choose a reason for hiding this comment

Uh oh!

itaiferber Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

itaiferber Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

itaiferber Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

itaiferber Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaiferber Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

parkera commented Nov 29, 2017

Uh oh!

parkera commented Nov 29, 2017

Uh oh!

parkera commented Nov 29, 2017

Uh oh!

itaiferber commented Nov 29, 2017

Uh oh!

parkera commented Nov 29, 2017

Uh oh!

alblue commented Nov 30, 2017

Uh oh!

parkera commented Dec 4, 2017

Uh oh!

alblue commented Dec 6, 2017

Uh oh!

parkera commented Dec 6, 2017

Uh oh!

Andrewangeta commented Mar 20, 2018

Uh oh!

parkera commented Mar 21, 2018

Uh oh!

Lukasa commented Mar 22, 2018

Uh oh!

parkera commented Mar 22, 2018

Uh oh!

itaiferber commented Mar 22, 2018

Uh oh!

Lukasa commented Mar 23, 2018

Uh oh!

parkera commented Mar 23, 2018

Uh oh!

Andrewangeta commented Mar 27, 2018

Uh oh!

gperdomor commented Jun 4, 2018

Uh oh!

millenomi commented Jun 4, 2018

Uh oh!

gperdomor commented Jul 13, 2018

Uh oh!

alblue commented Aug 7, 2018

Uh oh!

parkera commented Aug 8, 2018

Uh oh!

Uh oh!

itaiferber Nov 29, 2017 •

edited

Loading