[SR-443] CGFloats parsed from string do not round trip

[lhoward]

Previous ID	SR-443
Radar	None
Original Reporter	@lhoward
Type	Bug
Status	Resolved
Resolution	Done

Additional Detail from JIRA

Votes	0
Component/s	Foundation
Labels	Bug
Assignee	@spevans
Priority	Medium

md5: 194c1ac7ca461fcc40bb81483d2a489b

Issue Description:

 let point1 = NSPointFromString("{1.0, 1.2234234234}")
 let point2 = NSPoint(x: CGFloat(1.0), y: CGFloat(1.2234234234))

    print("\(point1) \(point2) \(point1 == point2)")

prints:

{1.0, 1.2234234234} {1.0, 1.2234234234} false

Expected result is true.

[lilyball]

This isn't testing CGFloat string parsing at all. Ultimately, it's testing NSScanner's implementation of scanDouble, which is rather different than Double.init?(_ text: String).

[lhoward]

Ah got it, thanks. Presumably it would be better to have a single path?

[lilyball]

Probably, although unless we want to introduce something like Double.scanDouble(_ text: String) -> (Double, rest: String)? to allow for trailing characters (which we probably don't want to do), NSScanner still needs to know the exact format that the parser accepts so it can find the exact substring to hand to the Double parser (remember that Double.init?(_: String) requires there to be no leading/trailing characters around the double, including whitespace).

I'm actually not entirely sure offhand why NSScanner right now actually implements a form of floating-point parsing instead of just detecting the string and handing it off to Double.init?(_: String). Maybe NSScanner supports some format that strtod_l doesn't, though I'd actually be kind of surprised if that were true. Quickly skimming the current NSScanner implementation, I am a little dubious about its parsing accuracy (I notice that for every digit it multiplies it by 0.1 or 0.1 * 0.1 or {0.1 * 0.1 * 0.1}} or ... and adds it to the old value, and I expect that's a fantastic way to get tons of floating-point error to creep into the result). Incidentally, the actual scanner for floating-point values in NSScanner weighs in at a mere 58 lines, as compared with the implementation in Rust whose mere documentation comment describing how that implementation works is longer (and the Rust implementation itself is spread out across 6 lines)! For reference the Rust code is an implementation of the 1990 paper How to Read Floating Point Numbers Accurately.

[tbkka]

"skimming the current NSScanner implementation"

Where do you see this? The code I'm looking at uses strtod for double parsing.

[tbkka]

Oh. I was looking at CFScanner, not Scanner.swift.

Yes, that code should definitely be using either Double.init(_:String) or it should call strtod from the C library.

[spevans]

This has been fixed on macOS now at least since 5.1.3 on 10.14 and is fixed in swift-corelibs-foundation with PR #2820

$ cat jiras/sr-443.swift 
import Foundation

let point1 = NSPointFromString("\{1.0, 1.2234234234}")
let point2 = NSPoint(x: CGFloat(1.0), y: CGFloat(1.2234234234))
print("\(point1) \(point2) \(point1 == point2)")

print("\nTesting scanDouble")
let input = "1.2234234234"
let scanner = Scanner(string: input)
var d: Double = 0

if scanner.scanDouble(&d) {
 print("d: \(d), d == \(input), \(d == Double(input))")
} else {
 print("Failed to scan")
}

macOS:

$ swift jiras/sr-443.swift 
(1.0, 1.2234234234) (1.0, 1.2234234234) true

Testing scanDouble
d: 1.2234234234, d == 1.2234234234, true

Linux, as of swift-DEVELOPMENT-SNAPSHOT-2020-06-13-a

 ~/swift-build/toolchains/swift-DEVELOPMENT-SNAPSHOT-2020-06-13-a-ubuntu18.04/usr/bin/swift jiras/sr-443.swift 
CGPoint(x: 1.0, y: 1.2234234234) CGPoint(x: 1.0, y: 1.2234234234) true

Testing scanDouble
d: 1.2234234234, d == 1.2234234234, true