[SR-7054] JSONDecoder Decimal precision error

[swift-ci]

Previous ID	SR-7054
Radar	rdar://problem/33491336
Original Reporter	tiborbodecs (JIRA User)
Type	Bug

Environment

Xcode 9.2 (9C40b)
Apple Swift version 4.0.3 (swiftlang-900.0.74.1 clang-900.0.39.2)

Same output with 4.1 swift-DEVELOPMENT-SNAPSHOT-2018-02-20-a-ubuntu16.04.

Additional Detail from JIRA

Votes	38
Component/s	Foundation
Labels	Bug
Assignee	bendjones (JIRA)
Priority	Medium

md5: 4d586dd9920250512d8b01ea1b3597c9

Issue Description:

Decoding decimals from a JSON structure returns incorrect numbers.

See the following example:

#!/usr/bin/env swift

import Foundation

struct DoubleItem : Codable \{
 var name: String
 var price: Double
}

struct DecimalItem : Codable \{
 var name: String
 var price: Decimal
}

let jsonString = """
\{ "name": "Gum ball", "price": 46.984765 }
"""
let jsonData = jsonString.data(using: .utf8)!

let decoder = JSONDecoder()
do \{
 let doubleItem = try decoder.decode(DoubleItem.self, from: jsonData)
 print(doubleItem)

let decimalItem = try decoder.decode(DecimalItem.self, from: jsonData)
 print(decimalItem)
} 
catch \{
 print(error)
}

Output:

DoubleItem(name: "Gum ball", price: 46.984765000000003)

DecimalItem(name: "Gum ball", price: 46.98476500000001024)

Expected result for the DecimalItem should be: 46.984765

I know that floating point values can not be represented precisely, but decimals should keep their original values, am I wrong? Is this a Swift foundation bug or an intended behavior? Note that encoding the same value with the JSONEncoder will provide the correct value in the JSON.

My actual problem is that the JSONEncoder and JSONDecoder classes are working inconsistently.

If the encoder will output the value as a number, I'd expect that the decoder can decode the exact same value from that if I use the decimal type in my model. My other idea is that the encoder should transform and save the value into a string type inside the JSON instead of the current representation, this could be supported with a decimal coding strategy.

let encoder = JSONEncoder()
encoder.decimalEncodingStrategy = .string //outputs as string: "46.984765"
encoder.decimalEncodingStrategy = .precise //outputs as number: 46.984765
encoder.decimalEncodingStrategy = .lossy //current output like: 46.984765


let encoder = JSONDecoder()
encoder.decimalDecodingStrategy = .string //decoded value from json string:  46.984765
encoder.decimalDecodingStrategy = .precise //decoded value from json number: 46.984765
encoder.decimalDecodingStrategy = .lossy //current value from number:  46.98476500000001024

Please share your thoughts about this idea.

[belkadan]

cc @itaiferber

[itaiferber]

This is a known issue that's been discussed at length in the past — I'll try to dig up some prior discussion about this. The core of the matter is that the implementations of JSONEncoder and JSONDecoder sit atop JSONSerialization, which provides the actual serialization to and from JSON data.

At the JSONSerialization layer, there's no type hint available to know whether the user expects to decode the value as a Double or an Int or a Float or a Decimal; a representation needs to be chosen for the value at deserialization time. JSONDecoder can then get that value, and convert it to the requested result, e.g. Decimal. The issue is that a string of digits like 46.984765 has differing representations as a Double or a Decimal. Since the value can't be represented precisely as a Double, stuffing the value into a Double gets you 46.98476500000000300 in the full expanded representation (which is what you see as the DoubleItem above). On the other hand, Decimal can represent that string of digits precisely, but when you might go to use the value (say, by getting the decimal's doubleValue), the calculated result is different than what you might expect, due to a propagation of many floating point losses of precision. (The specific result you get, BTW, will be further different depending on whether the Decimal is created from a string or from an existing Double value.)

You can see this with the following ObjC code:

#import <Foundation/Foundation.h>

NS_FORMAT_FUNCTION(1, 2)
static void print(NSString *format, ...) {
    va_list arguments;
    va_start(arguments, format);
    puts([[NSString alloc] initWithFormat:format arguments:arguments].UTF8String);
    va_end(arguments);
}

int main(int argc, char *argv[]) {
    @autoreleasepool {
        double d = 46.984765;
        print(@"%.*f\n", DBL_DECIMAL_DIG, d); // => 46.98476500000000300
        
        NSDecimalNumber *number = [NSDecimalNumber decimalNumberWithString:@"46.984765"];
        print(@"%@", number); // => 46.984765
        print(@"%.*f\n", DBL_DECIMAL_DIG, number.doubleValue); // => 46.98476499999999589
        
        number = [NSDecimalNumber numberWithDouble:d];
        print(@"%@", number); // => 46.98476500000001024
        print(@"%.*f\n", DBL_DECIMAL_DIG, number.doubleValue); // => 46.98476500000002432
    }
}

All of these results are "correct" for some definition of "correct". If JSONSerialization always produced Decimal values, folks grabbing the doubleValue from the results in order to perform calculations might be surprised by the inexactness of the result; in the same vein, if we prefer to produce accurate Double values, the original string of digits is lost and we can only create a Decimal from the Double, further losing precision.

At the end of the day, we preferred to always accurately round-trip Double values, i.e. if you encode a Double with JSONSerialization and decode it, you should always get the same exact (bitwise-equal) value back. This means that for values which lose precision when converted to a Double, the decimal value you see will be slightly different.

[itaiferber]

There are a few ways we could potentially address this, potentially requiring changes in JSONSerialization itself:

1. Add a flag for JSONSerialization which requests always producing a Decimal on its end, and JSONDecoder can make the decision about how to process and return that

2. Adding a similar flag which requests returning all numbers as strings to really defer the problem to a different layer

3. Moving JSONEncoder and JSONDecoder off of JSONSerialization and onto a custom serializer

Each of these approaches have their tradeoffs that we'd need to weigh if we want to address this.

[swift-ci]

Comment by Tibor Bödecs (JIRA)

@itaiferber thank you for the deep explanation, I'm aware of the root cause of the issue.

If I take the following example:

let x = Decimal(46.984765)

print(x) // 46.98476500000001024

let y = Decimal(string: "46.984765")

print(y) // 46.984765

Both values are "correct", but from my point of view, however if I encode, decode, encode the second decimal number to JSON, the final value is going to be an "incorrect" (46.98476500000001024) decimal number value. That's clearly not the expected representation because of the underlying JSONSerialization problem. I would love to see a better solution, because the current behaviour of the coder classes can be misleading if you are working with decimals. Also it would be nice to save decimals to JSON as numbers instead of strings, because currently that's the only alternative to hold the original value.

[itaiferber]

tiborbodecs (JIRA User) I agree on all points — this should be something that "just works" and it's unfortunate that we leak the implementation details here in this way. This is one of the many improvements to our implementations we'd like to make.

[swift-ci]

Comment by Sergej Jaskiewicz (JIRA)

@itaiferber I see the following in the _JSONDecoder current implementation:

fileprivate func unbox(_ value: Any, as type: Decimal.Type) throws -> Decimal? {
    guard !(value is NSNull) else { return nil }
    
    // Attempt to bridge from NSDecimalNumber.
    if let decimal = value as? Decimal {
        return decimal
    } else {
        let doubleValue = try self.unbox(value, as: Double.self)!
        return Decimal(doubleValue)
    }
}

If we change it to

fileprivate func unbox(_ value: Any, as type: Decimal.Type) throws -> Decimal? {
    guard !(value is NSNull) else { return nil }
    
    // Attempt to bridge from NSDecimalNumber.
    if let decimal = value as? Decimal {
        return decimal
    } else if let number = value as? NSNumber,
        number !== kCFBooleanTrue,
        number !== kCFBooleanFalse {

        return number.decimalValue
    } else {
        let doubleValue = try self.unbox(value, as: Double.self)!
        return Decimal(doubleValue)
    }
}

then everything is fine — the JSON data is decoded correctly.

For example, this test from the test suite:

lazy var decimalValues: [Decimal] = [
        Decimal.leastFiniteMagnitude,
        Decimal.greatestFiniteMagnitude,
        Decimal.leastNormalMagnitude,
        Decimal.leastNonzeroMagnitude,
        Decimal(),
        Decimal(string: "67.52")!, // This was added

        // Decimal.pi does not round-trip at the moment.
        // See rdar://problem/33165355
        // Decimal.pi,
    ]

    func test_Decimal_JSON() {
        for decimal in decimalValues {
            // Decimal encodes as a number in JSON and cannot be encoded at the top level.
            expectRoundTripEqualityThroughJSON(for: TopLevelWrapper(decimal))
        }
    }

was failing after I had added `Decimal(string: "67.52")!` to the `decimalValues` array. However, after the aforementioned fix, it passes and no other test fails.

Or am I missing something?

I would be happy to create a PR and write some tests for it.

[swift-ci]

Comment by Tibor Bödecs (JIRA)

broadway_lamb (JIRA User) sounds good, it'd be good to se a possible fix for this issue! 😉

[itaiferber]

broadway_lamb (JIRA User) I meant to respond to this earlier — this fix might work for some numbers, but not all:

import Foundation

let decimal = Decimal(string: "46.984765000000003")!
print(decimal) // => 46.984765000000003

let number = NSNumber(value: 46.984765000000003)
print(number.stringValue) // => 46.984765
print(String(format: "%0.17f", number.doubleValue)) // => 46.98476500000000300

print(number.decimalValue == decimal) // => false

number.decimalValue gets the (imprecise) string value of the number, then creates a Decimal from that. What you're seeing is essentially Decimal(string: "46.984765000000003") != Decimal(string: "46.984765").

I think this gets us closer to the right answer, but is inefficient and still doesn't provide the correctness we need, unfortunately.

[swift-ci]

Comment by Petr Dvorak (JIRA)

Hello @itaiferber, I was briefly looking at the source code (I admit that building Swift is more than I can currently chew - you guys are doing a great job!) and I was wondering: Why the [Decimal type uses the Double conversion for unboxing|https://github.com/apple/swift/blob/master/stdlib/public/SDK/Foundation/JSONEncoder.swift#L2417] and not String? Is this for the performance reason (and if yes, is the performance hit so unacceptable in typical scenarios)?

Solving this issue would help us a lot. We have APIs that send correct double values in JSON and all our other projects (usually built in Java, leveraging the veteran JSON mapper Jackson) work correctly (they map the JSON value to BigDecimal via String representation). We only encounter issues with Swift...

[itaiferber]

petrdvorak (JIRA User) I think that my original comment on this explains things best — specifically, that there is unfortunately no correct answer here because Double and Decimal are not interchangeable. Due to the numeric representation of Decimal, even if we preferred it over Double always, it produces different results in mathematical operations than Double would due to differences in magnitude and precision. This means that even if you got the full range of values in Decimal, it could potentially not be as useful mathematically as Double. This is a hairy problem that doesn't have a clear solution because everyone's use case is different.

In a perfect world, my preferred solution would be to truly use a BigDecimal-like type if we had it because it would more perfectly preserve the representation in almost all cases. We've got some thoughts internally on doing things in this area (or at least offering higher-precision/magnitude representation types) to alleviate this problem, but for the moment, we're constrained by backwards compatibility on a lot of things. Given that we don't yet have a BigDecimal-like type in either Obj-C or Swift hampers a more perfect solution.

/cc @stephentyrone for some visibility on this in light of some of our previous discussions.

[swift-ci]

Comment by Petr Dvorak (JIRA)

@itaiferber Thank you for the comment - I completely forgot there is no BigDecimal-like type in Obj-C/Swift. I got a bit rusty since the last time I did some iOS coding... 🙁

To provide yet another hint of possible direction... Maybe having some "hooks" - alongside the Jackson Streaming API would allow for a more precise (de)serialization?

(EDIT: I am not suggesting making a streaming API - instead, I would like to have the callback when reading attributes so that I have some control over the writing / parsing...)

[itaiferber]

petrdvorak (JIRA User) That's certainly another way of solving the issue that we've considered — but it quickly hits the fact that parsing is done up-front by JSONSerialization, which doesn't itself have hooks for this sort of thing right now. This precision deficiency is a sort of leaky implementation detail that we want to resolve, which can be done either by having JSONSerialization add hooks for this sort of thing (and JSONEncoder/JSONDecoder allow you to use those hooks), or by using a different serializer (also something that's considered). At the moment, we don't have a great answer, but we have given serious thought to moving off of JSONSerialization and onto an internal serializer which gives us flexibility and allows us to parse with the intent of the developer known to us.

[swift-ci]

Comment by Eddie LukeAtmey (JIRA)

Hi @itaiferber, I just saw this pull request that has been merged into `apple:master`. I'd like to know if the pull request has fixed the issue. If so, when will the fix be distributed?

[itaiferber]

eddiemarvin (JIRA User) I believe that change should make it into the Swift 5 release. However, please keep in mind that (a) this change is in swift-corelibs-foundation, and thus will not affect Darwin platforms, and (b) even that fix will not resolve the issue; although it improves parsing of values on non-Darwin platforms, the base issue remains: parsing numbers as Decimal leads to precision loss on conversion to Double, and parsing numbers as Double leads to precision loss on conversion to Decimal (or up-front on parsing). As long as JSONDecoder relies on JSONSerialization making some decision up-front about how to parse numbers, some use cases will still see imprecise values.

[swift-ci]

Comment by Charles Maria Tor (JIRA)

Anyone adversely affected by this issue like I was (I work for a bank, precision is very important) can use this custom JSONDecoder for the time being. Attached are tests written with SwiftCheck that prove the issue exists in JSONDecoder and does not exist in my replacement Decoder.