Removing unsafe arithmetic methods


(Ben Cohen) #1

During the implementation of SE-104 in Swift 4, a handful of methods were added to FixedWidthInteger: unsafeAdding(_:), unsafeSubtracting(_:), unsafeDivided(by:) and unsafeMultiplied(by:).

These were not part of the SE proposal – they were part of early prototypes, and ended up getting merged alongside the changes that were accepted.

These methods are easily confused with the overflow operators, except that they'll trap in debug. But they are very different: if they fail in release builds they produce undefined behavior. They might wrap, or crash, or produce garbled results, depending on what the compiler does with this instruction. This is very dangerous (despite having "unsafe" in the name) and is fairly out of keeping with the rest of the standard library, where this kind of unsafe undefined behavior is usually reserved for pointers and low level memory operations. But they do this while offering a tempting suggestion of safety: that they assert in debug builds on overflow (similar to how UnsafeBufferPointer asserts on out-of-bounds access in debug builds). But in the case of arithmetic operations, it would probably be better written explicitly in user code, with a combination of an assert and either addingReportingOverflow or &+, both of which have well-defined results in cases of overflow.

Since these methods were not introduced as part of an evolution proposal, the core team feels it's probably appropriate to deprecate/remove them (ideally before they make it into the ABI) without a further proposal. A search of GitHub suggests they are barely used (most hits are copies of the std lib). But I'm starting a thread in case anyone has views on their benefits we might be missing.


(Steve Canon) #2

Make it so.


(^) #3

is there ever a reason to use unsafeAdding(_:) instead of &+ at all?


(Steve Canon) #4

The semantics of these are totally different from &op. It would never make sense to use one in place of the other (this is a big part of why we want to remove them--this point is incredibly easy to mixup):

  • &+ is a "wrapping" addition. The result wraps around modulo 2^N, where N is the number of bits in the type.
  • + is a "trapping" addition; if overflow would occur, the operation traps instead.
  • unsafeAdding is a promise from the programmer that the computation does not overflow, and that you are OK with nasal demons if it does. This is fundamentally unlike either of the other two operations, which are fully defined in all cases.

(Chris Lattner) #5

FWIW, there are important reasons to support "nsw" and "nuw" in something like LLVM IR and key reasons to use them in C. I don't see any reason to expose them to Swift. The most important use cases were eliminated by making Int track the size of the machine pointer.


(Thomas Roughton) #6

I was curious about this, and so I tried looking at the generated assembly for three methods (Godbolt).

func test1(n: UInt16) -> UInt16 {
    let x = n * n
    let x2 = x * n
    let x3 = n - x2 + n
    return x3
}

func test2(n: UInt16) -> UInt16 {
    let x = n &* n
    let x2 = x &* n
    let x3 = n &- x2 &+ n
    return x3
}

func test3(n: UInt16) -> UInt16 {
    let x = n.unsafeMultiplied(by: n)
    let x2 = x.unsafeMultiplied(by: n)
    let x3 = n.unsafeSubtracting(x2).unsafeAdding(n)
    return x3
}

On x64, test3 and test2 generate the exact same code. In fact, test3 just calls directly into test2's implementation!

If we compile with -Ounchecked instead of -O, we see that all three methods get compiled down to the same code.

I was a little concerned we might be losing out on performance, but looking at the generated assembly (and assuming we'd see similar results on other platforms) I'm 100% behind removing the unsafe... integer methods.


(Xiaodi Wu) #7

Given that this idea has marinated for several months and that the final branching for Swift 5 is tomorrow, is the core team going through with this?


(Steve Canon) #8

(Ben Cohen) #9

Yeah sorry, my bad. I should have posted back to this thread to say this change went ahead.