Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
regexident opened this issue Oct 20, 2020 · 8 comments
Open

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

regexident opened this issue Oct 20, 2020 · 8 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior.

Comments

@regexident
Copy link
Contributor

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
Copy link
Member

hborla commented Oct 20, 2020

@swift-ci create

@typesanitizer
Copy link

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

@hborla
Copy link
Member

hborla commented Oct 20, 2020

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
Copy link
Member

hborla commented Oct 20, 2020

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
Copy link
Collaborator

xwu commented Nov 7, 2020

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
Copy link
Contributor Author

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

@hborla
Copy link
Member

hborla commented Nov 11, 2020

@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
Copy link
Contributor Author

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

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@xwu @regexident @typesanitizer @hborla