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.
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
[SR-6820] Literal 0
triggers spurious compile-time error, breaks source compatibility with Swift 4.0
#49369
Comments
/cc @stephentyrone |
cc @moiseev |
@swift-ci create |
I wonder is that's related to the CS changes by @xedin I am having hard times remembering any stdlib changes around integers since 4.0, apart from those that support the CS work. |
It looks like it might be related to removal of _Strideable, most likely in -swift-version 3 there is conflicting overload which we don't have in >= 4.1 mode... |
Out of curiosity, how is _Strideable related to compiler diagnostics for literals? |
@xwu `0` literal is ExpressibleByIntegerLiteral which means that it can be any integral of floating-point type, and since you have operators involved in here because of the _Strideable conformance we might have been picking a different overload. "arithmetic operation 'x + 1' (on T) results in an overflow" error happens down the line from type-checker (in think on the SIL optimizer stage). |
There aren't any operators involved in the second example, nor in the case of 0.magnitude cited in the other bug report. I'm struggling to see the connection here as there should be nothing attempting to add 1 (of any type). Moreover, the same bug is triggered without literals, as Int(),magnitude will apparently cause the same error, so it appears not to be a type inference issue. It seems the compiler is trying to compute the value 0 by performing (in the case of UInt8) 255 + 1, which would work if instead it were 255 &+ 1, but this is in either case a curious thing to do behind the scenes for the value 0. |
Sorry I only looked at the NumericAnnex examples you have in the description. `0.magnitude` is interesting but I don't think it has anything to do with type inference because 0 is inferred to be Int and 0.magnitude result is inferred as UInt and I can see that the whole expression type-checks effectively into `Int(0).magnitude` which then fails down the line in optimizer. |
/cc @eeckstein aschwaighofer@apple.com (JIRA User) shajrawi (JIRA User) |
Xiaodi, are you still seeing these issues with 4.2? |
I'll test again this weekend. I've been delinquent in maintaining my projects lately. |
@stephentyrone Yes, these issues are still manifesting with 4.2. |
@xwu can you provide a self-contained example of the failures that you're seeing? The code snippets provided in the bug description here are not sufficient to see what's going wrong. |
OK, here's a self-contained example. Apologies that it's not reduced further. Interestingly, my not-at-all kosher use of '@_transparent' here is required to expose the problem. import XCTest
extension UnsignedInteger {
public static func gcd(_ a: Self, _ b: Self) -> Self {
// An iterative version of Stein's algorithm.
if a == 0 { return b }
if b == 0 { return a }
var a = a, b = b, shift = 0 as Self
while ((a | b) & 1) == 0 {
a >>= 1
b >>= 1
shift += 1
}
while (a & 1) == 0 { a >>= 1 }
repeat {
while (b & 1) == 0 { b >>= 1 }
if a > b { swap(&a, &b) }
b -= a
} while b != 0
return a << shift
}
}
public struct Rational<T : SignedInteger> where T.Magnitude : UnsignedInteger {
public var numerator: T
public var denominator: T
@_transparent
public init(numerator: T, denominator: T) {
self.numerator = numerator
self.denominator = denominator
}
}
extension Rational {
@_transparent
public var isCanonical: Bool {
if denominator > 0 {
return T.Magnitude.gcd(numerator.magnitude, denominator.magnitude) == 1
}
return denominator == 0 && numerator.magnitude <= 1
}
}
public typealias Ratio = Rational<Int>
XCTAssertFalse(Ratio(numerator: 2, denominator: 0).isCanonical)
// Error: Arithmetic operation '18446744073709551615 + 1' (on unsigned 64-bit integer type) results in an overflow |
OK, that's really weird, and it's not totally obvious that this is even a stdlib bug; there's no explicit addition in the code, and we get this failure at compile time, not runtime. @xedin any idea what's happening here? |
@xwu This continues to be really weird, but it does not reproduce on master, and 0.magnitude behaves as expected there as well (it does reproduce on 4.2). I can't point to exactly what changed or when, but I think that this is fixed. If you have the interest, adding a couple test cases to make sure this doesn't regress might be worthwhile. |
@eeckstein Any idea what changed to resolve this? We see it in 4.2 but not master, and you've made some semi-recent changes to ConstantFolding.cpp. |
Sorry, no idea |
While working on SE-0213 I've encountered a problem with |
Interesting. Why is constant propagation is evaluating the unused side of a ternary? Naively I would think that it would evaluate the condition first and short-circuit if it can. |
Yes, that's exactly what I have expected as well! ravikandhadai (JIRA User) has investigated it and says that it would require a lot of work to make it that way... |
@stephentyrone my recollection of discussions with Erik back when we discovered it is: constant propagation just had not been implemented to differentiate between branches, it tries to simplify all the code, even the branches-not-taken. |
FWIW I did not raise any bugs/radars for that issue. Perhaps @xedin did. |
Comment by Ravichandhran Kandhadai Madhavan (JIRA) There is a radar tracking the bug: <rdar://39429767> Yes @moiseev is right. Sadly, constant propagation does not identify infeasible branches, and @_transparent functions aggravate this problem. Hopefully, in the near future, we can make constant propagation use compile-time interpretation (#19579 and get rid of @_transparent functions. |
Thanks. @xwu, I'm going to close this SR, as the issue reported here is resolved on master. |
ravikandhadai (JIRA User) I put a note in the radar, but it seems like maybe we shouldn't report static overflows within conditionals unless we can prove that the path is taken. Does that seem reasonable? |
Some time ago I included constant folding into SimplifyCFG (commit 4f8cccf). This should solve the general problem (maybe with some more little changes in SimplifyCFG). Another option would be to implement sparse conditional constant propagation. It's on our to-do list for a while with low priority. |
Comment by Ravichandhran Kandhadai Madhavan (JIRA) I wonder whether restricting to provably feasible paths would be very conservative e.g. if we have function calls along the CFG path, or if there are guards that are too complex for the constant folder to know that it is a constant etc. Perhaps, @eeckstein idea of treating every node in the Simplified CFG as feasible/reachable seems reasonable. |
Additional Detail from JIRA
md5: a9726c96f9cb0d194a1df694ad41f0ba
Issue Description:
This bug is related to SR-6602, or it is the same bug with a different manifestation.
In that report, it was thought that the bug would affect essentially no one in the wild because people won't be writing `0.magnitude`.
However, I have just discovered that my own, in-the-wild test cases for NumericAnnex break terribly with the latest builds of Swift. For example:
or
both fail to compile with the warning "arithmetic operation 'x + 1' (on T) results in an overflow" (where x == T.max).
I had to disable the statements that failed to compile, of which there were many. See: xwu/NumericAnnex@c5816a9
The text was updated successfully, but these errors were encountered: