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

saiHemak · 2018-05-23T08:41:47Z

This PR is based on PR #1347 by @parkera .

@parkera I have applied some bridging fixes on top of your PR to make the tests pass.

saiHemak · 2018-05-23T08:44:36Z

@parkera All the JSONEncoder tests are passing successfully.However, two tests ( TestCodable.test_URL_JSON and TestCodable.test_Decimal_JSON) are failing still. I have commented out these two tests as of now.
Please review

parkera · 2018-05-23T16:25:47Z

Foundation/JSONEncoder.swift


        /// Produce JSON with dictionary keys sorted in lexicographic order.
-        @available(macOS 10.13, iOS 11.0, watchOS 4.0, tvOS 11.0, *)
+        @available(OSX 10.13, iOS 11.0, watchOS 4.0, tvOS 11.0, *)


I think this reverts some earlier changes made to normalize the availability macros.

Yes @saiHemak this needs to match the version in the overlay: https://github.com/apple/swift/blob/master/stdlib/public/SDK/Foundation/JSONEncoder.swift#L35

parkera · 2018-05-23T16:26:41Z

TestFoundation/TestCodable.swift

            ("test_PersonNameComponents_JSON", test_PersonNameComponents_JSON),
            ("test_UUID_JSON", test_UUID_JSON),
-            ("test_URL_JSON", test_URL_JSON),
+           // ("test_URL_JSON", test_URL_JSON),


I think we'll need to figure out the root cause of these before we can do a final merge. What were the failures you were seeing? Any clue as to what's causing them to fail?

FYI, the two tests that now fail here are the only two tests in TestCodable that fail when run against Darwin's native foundation, so the tests could be wrong (although they don't look to be).

parkera · 2018-05-23T16:27:12Z

@millenomi is working on bringing bridging to SCL-foundation in parallel, so we may be able to simplify this patch if we can get that to land first.

saiHemak · 2018-05-24T10:45:33Z

@parkera Sure ..Thanks ..

ianpartridge · 2018-06-02T19:00:54Z

Now that #1526 is merged we should rework this - hopefully it's easier now!

saiHemak · 2018-06-13T08:58:35Z

Thanks @millenomi for the work on bridging. @ianpartridge I am working on this PR with bridging changes in place

millenomi · 2018-06-13T13:14:38Z

@saiHemak A thing that may not be obvious and I need to document is that you still need to use _SwiftValue.store()/.fetch() in the bridging code for this class to avoid bringing in non-Swift Foundation values when this code runs on Darwin.

ianpartridge · 2018-07-25T08:19:14Z

@swift-ci please test

ianpartridge · 2018-07-25T08:43:03Z

Foundation/JSONEncoder.swift


    fileprivate mutating func popContainer() -> NSObject {
-        precondition(!self.containers.isEmpty, "Empty container stack.")
+        precondition(self.containers.count > 0, "Empty container stack.")


@saiHemak this is also wrong: https://github.com/apple/swift/blob/master/stdlib/public/SDK/Foundation/JSONEncoder.swift#L359

ianpartridge · 2018-07-25T08:45:03Z

Foundation/JSONEncoder.swift

    /// - returns: A value of the requested type.
    /// - throws: `DecodingError.dataCorrupted` if values requested from the payload are corrupted, or if the given data is not valid JSON.
    /// - throws: An error if any value throws an error during decoding.
    open func decode<T : Decodable>(_ type: T.Type, from data: Data) throws -> T {


This function looks very different to the overlay - https://github.com/apple/swift/blob/master/stdlib/public/SDK/Foundation/JSONEncoder.swift#L1102 ??

I have applied overlay changes on top of #1347. However still this implementation depends on bridgeToObjectiveC instead of ConevertedKey method.Hence only the changes pertaining to the ConvertedKey have been ignored.

saiHemak · 2018-08-08T16:03:28Z

@ianpartridge please review

ianpartridge · 2018-08-08T16:10:48Z

Hi @itaiferber this is our latest effort to update JSONEncoder in accordance to the current version in the overlay. Could you review please?

itaiferber

Content-wise, this looks good! My main question is: given the state of bridging on Linux right now, what (if anything) is now preventing us from merging the implementations? (/cc @millenomi as well)

itaiferber · 2018-08-08T16:20:01Z

Foundation/JSONEncoder.swift

    fileprivate func box<T : Encodable>(_ value: T) throws -> NSObject {
+        return try self.box_(value) ?? NSDictionary()
+    }
+    //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.


Tiny style nit: missing newline before comment and space after //

itaiferber · 2018-08-08T16:22:37Z

Foundation/JSONEncoder.swift

-            overflow = true
-        */
-
+            


Another tiny nit: unless GitHub's diff view is showing the new comment incorrectly, looks like the actual code here should be reformatted along with the new indentation.

millenomi · 2018-08-08T17:46:35Z

Unfortunately we need slightly different code because the bridging syntax on Darwin can produce Darwin Foundation objects — cfr.
#1579 (comment). But there's no reason we can't have helpers for this on both the overlay and Swift Foundation sides to help until we figure out how to switch those classes out at runtime.

millenomi · 2018-08-08T17:48:01Z

@swift-ci please test

itaiferber · 2018-08-08T17:54:15Z

@millenomi Got it. How much do we support running s-cl-f on Darwin? (Makes sense for testing but also feels a bit unfortunate that it prevents us from unifying these sorts of things right now.)

millenomi · 2018-08-08T18:15:42Z

For the record, the result of that discussion so far is:

for now, we don't want people using SCF on Darwin for general purpose applications; but
not letting SCF run on Darwin is a significant barrier of entry for community members who want to contribute

thus it's not acceptable to break it on Darwin.

The longer-term solution is to move the compile-time conditionals for Linux bridging to be runtime checks, so that we can e.g. have a -bridging-framework SwiftFoundation flag or similar somewhere to change which classes are used for .none and boxing. (Or runtime hooks that SCF can call into.) But we can create e.g. box(), unbox() helpers in the overlay that use the bridging syntax, and in SCF to delegate to _SwiftValue.store()/.fetch(), and have the same code in both places that way.

itaiferber · 2018-08-08T18:20:33Z

@millenomi Fair enough, and sounds good! This is one such place that suffers in our code due to the bridging split, so if we can get to source stability internally it'll make things significantly easier to apply from the overlay, and vice versa.

spevans · 2018-08-09T16:32:27Z

@millenomi I agree that SCF on Darwin only has value for developing SCF itself (inside Xcode) which is easier than on Linux. Maybe an alternative solution is to build an Xcode toolchain with SWIFT_OBJC_INTEROP=0 to make it the same as Swift running on any other platform.

This should mean that any bridging code that has to work with the ObjectiveC runtime can be removed and reduces the maintenance burden for SCF on Darwin.

A secondary advantage may be that anyone targeting Swift on the (Linux) Server can use it to develop in Xcode using SCF so there are no surprises when they move to their code to Linux as the Foundation functionality will be the same (hopefully).

Im not sure if a toolchain can be built with SWIFT_OBJC_INTEROP=0 but it might be worth examining. Even now the SCF tests don't actually build/run 100% on Darwin due to bridging issues.

millenomi · 2018-08-09T17:18:12Z

Building a toolchain with -DSWIFT_OBJC_INTEROP=0 on Darwin requires a tiny bit of nonpublic information, which makes it a problem on a number of levels. (It can be done, just not easily outside Apple.)

There is little value in bifurcating the toolchain builds, though. It is infinitely more valuable to have a single toolchain that can be set up to understand either mode (so, turning the #if SWIFT_OBJC_INTEROP changes I wrote into runtime checks) than there is in shipping two different binaries for such a narrow scope.

ianpartridge · 2018-08-14T15:21:04Z

@swift-ci please test

parkera · 2018-08-14T15:30:51Z

To summarize the thread: we can pursue the objc interop discussion separately because it can't be resolved in this particular PR.

parkera reviewed May 23, 2018

View reviewed changes

saiHemak force-pushed the jsonencoder-overlay branch from 2946c2d to 8c78bea Compare July 23, 2018 08:45

ianpartridge reviewed Jul 25, 2018

View reviewed changes

saiHemak force-pushed the jsonencoder-overlay branch 17 times, most recently from fb8688f to 340727c Compare August 8, 2018 15:03

ianpartridge requested a review from itaiferber August 8, 2018 16:10

itaiferber approved these changes Aug 8, 2018

View reviewed changes

saiHemak force-pushed the jsonencoder-overlay branch from 340727c to 8cb7f29 Compare August 8, 2018 17:11

JSONEncoder overlay changes

3c33746

saiHemak force-pushed the jsonencoder-overlay branch from 8cb7f29 to 3c33746 Compare August 8, 2018 17:17

parkera mentioned this pull request Aug 8, 2018

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

Closed

ianpartridge merged commit 24e43de into swiftlang:master Aug 14, 2018

ianpartridge pushed a commit to ianpartridge/swift-corelibs-foundation that referenced this pull request Aug 14, 2018

JSONEncoder overlay changes (swiftlang#1561)

2e9d601

parkera pushed a commit that referenced this pull request Aug 24, 2018

JSONEncoder overlay changes (#1561) (#1661)

692aa6f

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

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

Uh oh!

Conversation

saiHemak commented May 23, 2018

Uh oh!

saiHemak commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

parkera commented May 23, 2018

Uh oh!

saiHemak commented May 24, 2018

Uh oh!

ianpartridge commented Jun 2, 2018

Uh oh!

saiHemak commented Jun 13, 2018

Uh oh!

millenomi commented Jun 13, 2018

Uh oh!

ianpartridge commented Jul 25, 2018

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!

saiHemak commented Aug 8, 2018

Uh oh!

ianpartridge commented Aug 8, 2018

Uh oh!

itaiferber left a comment

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!

millenomi commented Aug 8, 2018

Uh oh!

millenomi commented Aug 8, 2018

Uh oh!

itaiferber commented Aug 8, 2018

Uh oh!

millenomi commented Aug 8, 2018

Uh oh!

itaiferber commented Aug 8, 2018

Uh oh!

spevans commented Aug 9, 2018

Uh oh!

millenomi commented Aug 9, 2018

Uh oh!

ianpartridge commented Aug 14, 2018

Uh oh!

parkera commented Aug 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

saiHemak commented May 23, 2018 •

edited

Loading