[SR-13755] Convenience operators on BinaryInteger causing subtle bugs due to unintuitive type inference

[regexident]

Previous ID	SR-13755
Radar	rdar://problem/70485231
Original Reporter	@regexident
Type	Bug

Additional Detail from JIRA

Votes	2
Component/s
Labels	Bug
Assignee	None
Priority	Medium

md5: 75125d4cd126a1caa0e7e05edfa98e00

relates to:

• SR-4984 Integer literal not inferred as unsigned in != comparison

Issue Description:

The existence of this method on BinaryInteger …

static func == <Other>(lhs: Self, rhs: Other) -> Bool where Other : BinaryInteger

… has caused me quite some headache today, and wasted several hours spent debugging.

The fact that this method (and its brothers & sisters <=, >=, etc.) performs the following conversion on the rhs …

let rhsAsSelf = Self(truncatingIfNeeded: rhs)

… makes the following code behave in rather unexpected ways:

func nonGeneric(_ t: UInt8) {
    precondition(t != 0b0)
    precondition(t == ~0b0)
}

func generic<T: BinaryInteger>(_ t: T) {
    precondition(t != 0b0)
    precondition(t == ~0b0)
}

let t: UInt8 = ~0b0

nonGeneric(t)
generic(t)

One would expect both calls to succeed (and most importantly: behave identical!).
Alas the second one has its second precondition fail:

Terminated due to signal: ILLEGAL INSTRUCTION (4)
Precondition failed: file Untitled.swift, line 8

Why? Let's investigate.

First we'll add the following helper function to help in inspecting rhs's type:

func spyType<T>(_ t: T) -> T {
    print("-", type(of: t))
    return t
}

Then we wrap rhs with spyType(…):

func nonGeneric(_ t: UInt8) {
    print(#function)
    precondition(t != spyType(0b0))
    precondition(t == spyType(~0b0))
}

func generic<T: BinaryInteger>(_ t: T) {
    print(#function)
    precondition(t != spyType(0b0))
    precondition(t == spyType(~0b0))
}

… to find this printed on the console:

nonGeneric(_:)
- Int
- UInt8
generic(_:)
- Int
- Int

The type-checker seems to infer rhs to be T: BinaryInteger, and then picks the default: Int, which then overflows on Self(truncatingIfNeeded: rhs).

The reason we get the correct UInt8 in there at all is that Int does not have an ~ operator, forcing the type-checker to infer the correct type of UInt8, which does.

While this is correct behavior from the perspective of the type-checker (thanks to the existence of above operator on BinaryInteger), it leads to subtle and utterly unexpected bugs and thus should be considered a bug, I think.

This is bad. So bad I wonder if above (convenience!) operator should exist at all, as it's the cause for all of this. There are just too many ways this could lead to unexpected behavior.

At the very least the compiler should emit a warning à la "The type of rhs is ambiguous, use an explicit type".

But even then naïvely wrapping the literal in a `UInt8(…)` to make it explicit doesn't solve the issue either and actually causes a fatal error now (but hey, at least it crashes now!):

func nonGeneric(_ t: UInt8) {
    print(#function)
    precondition(t != UInt8(spyType(0b0)))
    precondition(t == UInt8(spyType(~0b0)))
}

func generic<T: BinaryInteger>(_ t: T) {
    print(#function)
    precondition(t != T(spyType(0b0)))
    precondition(t == T(spyType(~0b0)))
}

nonGeneric(_:)
- Int
- Int
Fatal error: Negative value is not representable: file Swift/Integers.swift, line 3439

One needs to actually promote the rvalues to explicitly typed lvalues to make the above code behave as expected:

func nonGeneric(_ t: UInt8) {
    print(#function)
    let emptyMask: UInt8 = 0b0
    precondition(t != spyType(emptyMask))
    let fullMask: UInt8 = ~0b0
    precondition(t == spyType(fullMask))
}

func generic<T: BinaryInteger>(_ t: T) {
    print(#function)
    let emptyMask: T = 0b0
    precondition(t != spyType(emptyMask))
    let fullMask: T = ~0b0
    precondition(t == spyType(fullMask))
}

Which now FINALLY produces the expected behavior …

nonGeneric(_:)
- UInt8
- UInt8
generic(_:)
- UInt8
- UInt8

As a developer this is not an acceptable workaround to me, tbh.

Any user who does not already have a solid understanding, of how type-inference in Swift works, would be utterly lost here. And even those who do will have a hard time finding the culprit.

It should not behave this way and it should definitely should not require me to bend over backwards to make it work at all.

(ALSO, I CAN HAZ TYPE ASCRIPTION? KTHXBAI!)

[hborla]

@swift-ci create

[typesanitizer]

Swift has as for type ascription; what's the difference between as and what you want?

[hborla]

It looks like the type checker does find a solution that infers the literal type as `T`, but it's ranked lower than the solution that infers the type to be `Int` even though there's an implicit conversion in that solution. It seems to me like a solution with the default literal type + implicit conversion is given higher priority than a solution with non-default literal type, which IMO makes literals without type annotations kind of unusable in a generic context like this...

[hborla]

Oh whoops I misread the description, it's not an implicit conversion - `Self(truncatingIfNeeded: rhs)` happens in the body of the operator. That makes a lot more sense. I do still think it's unexpected that this overload is preferred. I think this means you can never use any of the comparison operators with literals and rely on the other side of the expression providing context for the literal.

[xwu]

I could swear that I filed a bug for this years ago, but it appears not. I described it here in my review of Swift numeric types and protocols.

The issue was originally encountered in the standard library itself. The bug is very subtle to debug; it is rarely observable because this PR added concrete overloads so that you don't encounter the same problem if you're not using generics. When it happens though, it is very bad. I tried a couple of times to address this with standard library changes, but it's really not possible.

@rudkx's designated type proposal for operators would have fixed the issue, but the design (which I thought was fantastic) never advanced and the change was backed out of the compiler. Hopefully @hborla can push this forward with her upcoming changes in this area!

[regexident]

@hborla am I right in assuming that PR 34399 might be providing a fix or at least an improvement on this issue?

[hborla]

@regexident That PR improves performance in some common cases of expressions with operators, but it does not change the behavior of operator overload resolution.

The issue here is partially due to the overload set on BinaryInteger (the only overload of == on BinaryInteger takes in two different generic parameter types, which means there would be no way for the compiler to infer the type of a literal based on the other side of ==), and partially due to the ranking rules between overloads (even if that overload on == did exist, the compiler would pick a different overload because of the current ranking rules).

[regexident]

I see. Thanks for the clarification, @hborla. Much appreciated!