SR-5640: JSONEncoder misrepresents UInt.max #1173

spevans · 2017-08-13T20:23:58Z

This is a workaround rather than a full fix. However it should make it easier to create the correct fix.

NSNumber(value: UInt.max).stringValue was returning "-1"
instead of "18446744073709551615" because NSNumber holds any
value > Int64.max as a 128Bit quantity using a high Int and
low UInt. It marks this type as an 'SInt128Type'.
CFNumberFormatterCreateStringWithNumber() uses CFNumberGetType()
to get this type however CoreFoundation wants to hide SInt128Type
(probably for backwards compatibilty) and instead returns it
as an SInt64Type. Thus only the low part of the NSNumber is used
for the number and this is interpreted as an Int64.
The workaround is simply in NSNumber.description(withLocale:)
to test to see if the value is of type SInt128Type and if so
use String(format: "%@") to convert it to a string instead of
using CFNumberFormatterCreateStringWithNumber(). This should be
ok for most situations since it is only used for positive
integers and there are no issues with formatting leading zeros
or decimal points.
For JSONWriter._serializationString(for: NSNumber) the value
is tested to see if it is an SInt128Type and if so the .stringValue
method is used to create a string.

ianpartridge · 2017-08-14T08:07:56Z

Foundation/NSNumber.swift

@@ -256,6 +256,10 @@ open class NSNumber : NSValue {
            fatalError("unsupported CFNumberType: '\(numberType)'")
        }
    }
+
+    public var isSInt128Type: Bool {


We can't add new public API unfortunately.

spevans · 2017-08-14T08:25:21Z

Ah, didn't realise that. Ive removed the function and inlined the test instead

ianpartridge · 2017-08-14T08:29:27Z

TestFoundation/TestNSNumber.swift

+    func test_stringValue() {
+        XCTAssertEqual(NSNumber(value: UInt.min).stringValue, "0")
+        XCTAssertEqual(NSNumber(value: UInt.min + 1).stringValue, "1")
+        XCTAssertEqual(NSNumber(value: UInt.max).stringValue, "18446744073709551615")


On a 32-bit platform UInt.max is 4294967295 so this will break.

ianpartridge · 2017-08-14T08:29:59Z

TestFoundation/TestNSNumber.swift

+        XCTAssertEqual(NSNumber(value: UInt64.max).stringValue, "18446744073709551615")
+        XCTAssertEqual(NSNumber(value: UInt64.max - 1).stringValue, "18446744073709551614")
+
+        XCTAssertEqual(NSNumber(value: Int.min).stringValue, "-9223372036854775808")


Same comment here - Int.min and Int.max are different on 32-bit platforms.

is there any easy test for platform wordsize apart from Int.max == Int32.max etc ?

I'm only aware of testing MemoryLayout<Int>.size == 32, which isn't much better.

ianpartridge · 2017-08-14T08:34:37Z

TestFoundation/TestJSONEncoder.swift

@@ -401,6 +401,68 @@ class TestJSONEncoder : XCTestCase {
        test_codingOf(value: URL(string: "https://swift.org")!, toAndFrom: "\"https://swift.org\"")
    }

+
+    // UInt and Int
+    func test_uintMinMax() {


Can the naming of this be consistent with the other tests, so test_codingOfUIntMinMax()?

Also we should add this testcase on the Darwin side too: https://github.com/apple/swift/blob/master/test/stdlib/CodableTests.swift

ianpartridge · 2017-08-14T08:44:17Z

I'm wondering if the upcoming merge of code from Apple might have a better fix for this...

ianpartridge · 2017-08-14T08:46:47Z

@swift-ci please test

spevans · 2017-08-14T09:00:05Z

I fixed up the test names. I left the CodableTests.swift out for now as I will do a separate JSON PR for some other minor fixes I want to do.

I suspect you are correct about there being a better fix in the 10.13 code dump but won't that happen after 4.0 has been released, i.e. when Xcode9 and High Sierra are publicly available?

spevans · 2017-08-14T09:06:17Z

Could you please retest, I added another commit after you kicked off the last test, thanks.

ianpartridge · 2017-08-14T09:09:22Z

I think the code drop is due pretty soon: after we stop auto-merging from master to swift-4.0-branch but before Swift 4 ships. It's possible we might be able to extract the full fix (if it exists!) into a PR against swift-4.0-branch, I don't know.

ianpartridge · 2017-08-14T09:42:45Z

@swift-ci please test

parkera · 2017-08-14T15:46:07Z

Foundation/JSONSerialization.swift

    //[SR-2151] https://bugs.swift.org/browse/SR-2151
    private mutating func _serializationString(for number: NSNumber) -> String {
+        if _CFNumberGetType2(number._cfObject) == kCFNumberSInt128Type {


It seems like an encapsulation violation to have JSONSerialization calling this function.

@parkera I agree, but this seems like a reasonable fix for the moment until changes come in from the CF side, no? Or is it possible to get CF changes in for this?

Since the stringValue can be used for any integer value, I could replace it with:

if !CFNumberIsFloatType(number._cfObject) { return number.stringValue }

The problem I have is the way this ties the implementation detail of how NSNumber stores its value to JSONSerialization - the two shouldn't really be coupled to each other that tightly if we can help it. Maybe the formatter is a better place to encapsulate that...

@parkera So ideally you want all references to _cfObject removed from JSONSerialization?

No - it's the _CFNumberGetType2 that is the problem. _cfObject is just our way to get toll-free bridging to work here.

so is !CFNumberIsFloatType(number._cfObject) ok to use instead?

spevans · 2017-08-14T21:08:45Z

I replaced the _CFNumberGetType2(number._cfObject) == kCFNumberSInt128Type

ianpartridge · 2017-08-15T08:15:23Z

@swift-ci please test

parkera · 2017-08-15T16:22:13Z

Yes, using the public CFNumber API is much better. Thanks.

spevans · 2017-08-15T16:28:56Z

Looks like the test failed due to a build timeout, Ive just tested it against swift-DEVELOPMENT-SNAPSHOT-2017-08-14 on Xcode and it worked ok

ianpartridge · 2017-08-16T07:55:15Z

@swift-ci please test

djones6 · 2017-08-17T14:02:01Z

I think there's a performance problem with the latest change - by routing the stringification of all int types through NSNumber.description, with the exception of the special case for SInt128, every conversion will create a new CFNumberFormatter.

A quick benchmark of JSONEncoder (on Linux) with a struct containing 10 Ints ran 2x slower.
(benchmark: https://github.com/djones6/swift-benchmarks/blob/master/jsonEncoderTest.swift)

ianpartridge · 2017-08-17T14:17:04Z

I guess we could change NSNumber.description(withLocale:) so it caches a static CFNumberFormatter and only creates a new one if the locale changes?

spevans · 2017-08-17T14:59:28Z

It could just use a fixed one when locale is nil as that is the case we care about.

spevans · 2017-08-17T16:21:11Z

@djones6 I added a cache for the CFNumberFormatter when locale is nil, can you see if that helps with the performance regression?

ianpartridge · 2017-08-17T19:41:42Z

@swift-ci please test

xwu · 2017-08-18T01:47:12Z

Foundation/NSNumber.swift

    open func description(withLocale locale: Locale?) -> String {
+        // CFNumberFormatterCreateStringWithNumber() doesnt like SInt128Type as


Drive-by nit: can you correct the spelling of "doesn't" and throw in a few punctuation marks (before the first "as" and before "so")?

xwu · 2017-08-18T01:47:37Z

Foundation/NSNumber.swift

+        // so special case them.
+        if _CFNumberGetType2(_cfObject) == kCFNumberSInt128Type {
+            return String(format: "%@", unsafeBitCast(_cfObject,
+                                                      to: UnsafePointer<CFNumber>.self))


Drive-by nit no. 2: can you put this on the same line?

djones6 · 2017-08-20T15:49:27Z

@spevans Thanks - this does address the performance regression. In fact, it seems to perform slightly better than the original code.

This is a workaround rather than a full fix. - NSNumber(value: UInt.max).stringValue was returning "-1" instead of "18446744073709551615" because NSNumber holds any value > Int64.max as a 128Bit quantity using a high Int and low UInt. It marks this type as an 'SInt128Type'. - CFNumberFormatterCreateStringWithNumber() uses CFNumberGetType() to get this type however CoreFoundation wants to hide SInt128Type (probably for backwards compatibilty) and instead returns it as an SInt64Type. Thus only the low part of the NSNumber is used for the number and this is interpreted as an Int64. - The workaround is simply in NSNumber.description(withLocale:) to test to see if the value is of type SInt128Type and if so use String(format: "%@") to convert it to a string instead of using CFNumberFormatterCreateStringWithNumber(). This should be ok for most situations since it is only used for positive integers and there are no issues with formatting leading zeros or decimal points. - For JSONWriter._serializationString(for: NSNumber) the value is tested to see if it is not a floating point value and if so the .stringValue method is used to create a string. - NSNumber.description(withLocale:) - cache the CFNumberFormatter when locale is nil as a small speedup.

spevans · 2017-08-20T17:28:36Z

Thanks for running those tests @djones6.

I have squashed and rebased and tidied up the comment so I think this may be ready to go.

spevans · 2017-08-23T18:12:32Z

@swift-ci please test

spevans · 2017-08-24T07:13:28Z

@swift-ci please test

ianpartridge · 2017-08-29T11:48:25Z

@swift-ci please test

ianpartridge · 2017-08-30T14:08:35Z

I think this is good for merge - @spevans @djones6 @itaiferber @parkera?

spevans · 2017-08-30T14:12:52Z

I think it is. Its fixes the NSNumber bug and I will have a follow up PR soon to hopefully speed up some of the integer serialisation.

ianpartridge reviewed Aug 14, 2017

View reviewed changes

parkera assigned itaiferber Aug 14, 2017

parkera reviewed Aug 14, 2017

View reviewed changes

xwu reviewed Aug 18, 2017

View reviewed changes

spevans force-pushed the pr_sr_5640 branch from f8f39d2 to 8d05411 Compare August 20, 2017 17:26

itaiferber merged commit aea30ad into swiftlang:master Aug 30, 2017

ianpartridge mentioned this pull request Aug 31, 2017

[4.0] SR-5640: JSONEncoder misrepresents UInt.max #1196

Merged

spevans deleted the pr_sr_5640 branch September 19, 2017 15:05

spevans mentioned this pull request Aug 13, 2017

[SR-5640] JSONEncoder misrepresents UInt.max on Linux #4417

Closed

		open func description(withLocale locale: Locale?) -> String {
		// CFNumberFormatterCreateStringWithNumber() doesnt like SInt128Type as

SR-5640: JSONEncoder misrepresents UInt.max #1173

SR-5640: JSONEncoder misrepresents UInt.max #1173

Uh oh!

Conversation

spevans commented Aug 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spevans commented Aug 14, 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!

ianpartridge commented Aug 14, 2017

Uh oh!

ianpartridge commented Aug 14, 2017

Uh oh!

spevans commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spevans commented Aug 14, 2017

Uh oh!

ianpartridge commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianpartridge commented Aug 14, 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!

Choose a reason for hiding this comment

Uh oh!

spevans commented Aug 14, 2017

Uh oh!

ianpartridge commented Aug 15, 2017

Uh oh!

parkera commented Aug 15, 2017

Uh oh!

spevans commented Aug 15, 2017

Uh oh!

ianpartridge commented Aug 16, 2017

Uh oh!

djones6 commented Aug 17, 2017

Uh oh!

ianpartridge commented Aug 17, 2017

Uh oh!

spevans commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spevans commented Aug 17, 2017

Uh oh!

ianpartridge commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djones6 commented Aug 20, 2017

Uh oh!

spevans commented Aug 20, 2017

Uh oh!

spevans commented Aug 23, 2017

Uh oh!

spevans commented Aug 24, 2017

spevans commented Aug 14, 2017 •

edited

Loading

ianpartridge commented Aug 14, 2017 •

edited

Loading

spevans commented Aug 17, 2017 •

edited

Loading