-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
proposal: spec: add integer types with explicit overflow behavior, and remove unchecked operations on built-in integers #30209
Comments
Note that part (1) would be trivial if we had operator overloading, and part (3) could really just be a The assignability rule could perhaps be eliminated from the proposal without causing too much damage. That would make the code much more verbose, but not fundamentally alter its structure. Similarly, the conversion check might be implementable using generic constructor functions, and can be implemented (inefficiently and without type-safety) using reflection. IMO, the interesting part of this proposal is part (2), the aggregation of overflow checks to the whole expression. That would otherwise require constructing an overflow-tracking object, using its methods in place of the existing arithmetic expressions, and remembering to check its result at the end — a lot of syntax, and much more error-prone than a regular old |
How come the |
@ericlagergren An unbounded signed integer type can represent all of the unsigned values too, and is closed under subtraction. In contrast, an unsigned type carries the risk of overflow: what would the behavior be for |
I’ll admit I haven’t read this too carefully, but I hope you’ll permit me some questions anyway:
|
Catching up with openshift/installer@dafc79f0 (Generate Network.cluster config instead of NetworkConfig.networkoperator, 2019-01-15, openshift/installer#1013) and openshift/installer@3b393da8 (pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30, openshift/installer#1154). The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values [1,2]. But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting. [1]: golang/go#19624 [2]: golang/go#30209
The The |
I think that would be fine. I don't know of many cases where it's actually useful, but it doesn't seem actively harmful either. |
Indeed. I don't feel strongly about that either way — I'm happy to go with whatever makes the proposal more palatable to the folks making the final decision. 🙂 |
Catching up with openshift/installer@dafc79f0 (Generate Network.cluster config instead of NetworkConfig.networkoperator, 2019-01-15, openshift/installer#1013) and openshift/installer@3b393da8 (pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30, openshift/installer#1154). The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values [1,2]. But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting. [1]: golang/go#19624 [2]: golang/go#30209
Catching up with openshift/installer@dafc79f0 (Generate Network.cluster config instead of NetworkConfig.networkoperator, 2019-01-15, openshift/installer#1013), openshift/installer@3b393da8 (pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30, openshift/installer#1154), and openshift/installer@9ad20c35 (pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31, openshift/installer#1170). The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values [1,2]. But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting. [1]: golang/go#19624 [2]: golang/go#30209
Catching up with openshift/installer@dafc79f0 (Generate Network.cluster config instead of NetworkConfig.networkoperator, 2019-01-15, openshift/installer#1013), openshift/installer@3b393da8 (pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30, openshift/installer#1154), and openshift/installer@9ad20c35 (pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31, openshift/installer#1170). The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values [1,2]. But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting. [1]: golang/go#19624 [2]: golang/go#30209
Catching up with openshift/installer@dafc79f0 (Generate Network.cluster config instead of NetworkConfig.networkoperator, 2019-01-15, openshift/installer#1013), openshift/installer@3b393da8 (pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30, openshift/installer#1154), and openshift/installer@9ad20c35 (pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31, openshift/installer#1170). The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values [1,2]. But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting. [1]: golang/go#19624 [2]: golang/go#30209
Catching up with openshift/installer@dafc79f0 (Generate Network.cluster config instead of NetworkConfig.networkoperator, 2019-01-15, openshift/installer#1013), openshift/installer@3b393da8 (pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30, openshift/installer#1154), and openshift/installer@9ad20c35 (pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31, openshift/installer#1170). The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values [1,2]. But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting. [1]: golang/go#19624 [2]: golang/go#30209
I'm sorry I haven't read everything carefully but how exactly is this backwards compatible?
EDIT: I guess it's not intended to be? Is there any idea of how much existing code this would break? |
If I'm reading this correctly essentially every existing Go program would become invalid, because they all have arithmetic operations on builtin integer types. That seems like a very heavy lift. |
@ianlancetaylor do you consider backwards-incompatible features which are trivially accommodated by go fix to be problematic? (Not sure whether this particular concept is easily go-fixable...) |
Running |
I'd prefer to see plain Even in the latter case, I feel like this proposal would make bounded arithmetic code overly verbose. As an aside, why disallow XOR and complement on unbounded ints? Assuming they're signed, aren't both operations well-defined and guaranteed to yield a finite result? |
Probably every program, but not every package: in particular, packages that use Also note that it would be possible to adopt parts (1) and (2) of this proposal — the new packages and checked assignment — without the breaking change of part (3). That would provide a much smaller benefit, but at a much smaller corresponding cost. |
Note that that proposal does not comply with the migration strategy outlined in https://github.com/golang/proposal/blob/master/design/28221-go2-transitions.md#language-redefinitions. |
I amended the proposal. Now I propose the range check is on compile type for constants, and run time for variables, although the compiler may still do the range checking at compile time (implementation defined). I also detailed the spec of len(range), worked out other details more. I also disallowed complex underlying types for range types and for range boundaries since for complex64 and complex128, the meaning of "range" becomes quite complex, indeed. |
Catching up with openshift/installer@dafc79f0 (Generate Network.cluster config instead of NetworkConfig.networkoperator, 2019-01-15, openshift/installer#1013), openshift/installer@3b393da8 (pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30, openshift/installer#1154), and openshift/installer@9ad20c35 (pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31, openshift/installer#1170). The uint32 -> int32 cast is slightly dangerous, because it will silently wrap overflowing values [1,2]. But I'll try and get the installer updated to use unsigned types as well, and then we won't have to worry about converting. [1]: golang/go#19624 [2]: golang/go#30209
I would argue that the comma-ok idea suggested here, part 2 of the proposal, should not be done initially. I argue this on two points. First, I think that few people will actually want that feature. Second, for those people who really do want it, they can already write it by using a deferred function that calls |
I would be ok with omitting the I think its major use-cases are for down-casting |
This is why I feel ranged types are an important feature of Ada and other Wirth-like languages: they help writing correct programs without overflow. As it stands now, in Go, like other C inspired languages, it is still too easy to accidentally cause overflow. Go would do well to adopt range types to prevent this. |
My immediate impression is that having logical shifts not effect the It is inconsistent with the semantics/"feel" of every other operation on integers that can cause logical result bits outside the size of the integer type, in a non-obvious way that I guarantee people will accidentally forget. (From all my experience carefully paying attention to how human cognition interacts with code, I predict we'd see non-negligible amounts of situations where people accidentally intuitively rely on Also, if we wanted to do a logical shift while checking that we shifted bits out, we'd have to do our own before-doing-the-operation checks. But if instead logical shift did effect the Maybe that doesn't matter because most code just doesn't do the kind of bit twiddling that would make this matter, and it looks from the other comments that the checked assignment part of the proposal is maybe already on the chopping block anyway, but I wanted to voice that. |
XOR was discussed earlier (and since math/big.Int has, I think this should have it too). Why no clear? Once again, math/big.Int has this: https://golang.org/pkg/math/big/#Int.SetBit |
I’d like to push back on this. I think the comma-ok idea is very useful, even if just by itself.
I think more folks would use checked arithmetic were it more approachable. There isn’t any easy way to do it right now. You can use math/bits, but it returns the results in the opposite order you’d expect for checked multiplication: (hi, lo) vs (x, ok). And it doesn’t make your intentions clear. You can use a third-party package, but now you’re writing x, ok := checked.Add(a, b) which gets very tiresome if you have more than just two integers, like a + b * c / d. (math/bits suffers from this as well.) Worse, it makes the code more difficult to read, and one of Go’s strengths is its readability. I’ve written a lot of math/big code and even I frequently lose my place trying to follow code with more than a little arithmetic.
I think using defer and recover to handle overflow is too clever and too subtle. It also makes the code more difficult to follow since defer blocks are almost always far away from the code causing the panic. It’ll be difficult to handle some overflows in one way, and some in others. Additionally, using defer and recover has a much more significant performance impact than comma-ok. Open-coded defers are pretty cheap, but they’re not free. However, comma-ok is free: the only cost is checking the overflow flag, which you have to do anyway with the defer recover strategy. |
This looks interesting. Would there be a way, with Go 1, to build code which makes integer overflows panic ? |
Can this be implemented as a simple bound checks? It could be more versatile since it should allow users to specify their custom bound range. It don't even have to be new types (though I like Beoran's suggestion), just few functions should do it, and no need to change how the language works. Simplified idea:
This might also allow us to mix different Integer types, and operate on them safely:
EDIT: It's basically the same as the one proposed in 60858 and 24853. Should have checked before posting, sorry. |
While this is an interesting idea, we can't realistically make such a large breaking change to the behavior of int. A similar argument is made regarding arbitrary precision ints in #19623 (comment). By contrast, issue #30613 proposes a potential path forward for checked ints that does not change the behavior of existing programs. Therefore, this is a likely decline. Leaving open for four weeks for final comments. |
No change in consensus, so declined. |
This proposal is intended to accomplish the same goals as #19624, but in a way that complies with the migration strategy outlined in https://github.com/golang/proposal/blob/master/design/28221-go2-transitions.md#language-redefinitions.
See #19624 for background.
Summary
This proposal would:
checked
,wrapped
, andunbounded
, containing integer types with distinguished semantics to be defined in the language spec., ok
form (as described in proposal: spec: change all int types to panic on wraparound, overflow #19624) to explicitly check for overflow in expressions of any bounded numeric type.New packages
The three new packages each provide a set of integer types:
Defined types
Defined types that have
checked
,wrapped
, orunbounded
types as their underlying type have the same operations and behavior as the underlying type.Defined types that have builtin integer types as their underlying type have the same behavior as the underlying type, except that they are not mutually-assignable with
checked
orwrapped
types.Checked assignment
A checked assignment uses the form
x, ok = <expression>
orx, ok := <expression>
, where<expression>
can comprise any number of arithmetic, bitwise, logical, and/or conversion operations yielding achecked
,wrapped
,unbounded
, user-defined, or builtin integer type. The operations in a checked assignment do not panic even if they involvechecked
or builtin integer types. Theok
result, which may be assigned to any boolean type, indicates whether any arithmetic operation or conversion within the expression overflowed. Thex
result has the type of<expression>
and a value computed using two's-complement wrapping.Bitwise operations and logical shifts do not overflow, and therefore do not set the
ok
result. (Recall that signed shifts are defined to be arithmetic, while unsigned shifts are defined to be logical.)Unchecked arithmetic operations on builtin integer types are a compile-time error.
Conversion
Any integer type can be explicitly converted to a
checked
,wrapped
, orunbounded
integer type.A
checked
,wrapped
, orunbounded
integer type can be converted to a builtin type only if either the conversion is checked or the destination type can represent all possible values of the source type.An unchecked conversion to a
checked
type panics if the value cannot be represented in the destination type.A conversion to a
wrapped
type wraps if the value cannot be represented in the destination type, even if the operand is of a largerchecked
type.A conversion to
unbounded.Int
from any integer type always succeeds. The conversion is applied after the operand is fully evaluated.Assignability
Each sized type in the
checked
andwrapped
package is mutually assignable with the corresponding builtin sized type, but not with the type in the opposing package, nor with defined types of any underlying integer type.This allows functions to perform operations on
checked
orwrapped
types, but to expose and use the corresponding builtin types at API boundaries (with less syntactic overhead for all involved).Arithmetic operators
The type of an arithmetic expression depends on its operands. If both operands are of
checked
,wrapped
,unbounded
, or defined types, then they must have exactly the same type. If one operand is achecked
,wrapped
, orunbounded
integer or a defined type with one of those types as its underlying type, and the other is either a builtin integer or untyped constant assignable to the first, then the result of the operation is thechecked
,wrapped
,unbounded
, or defined type.If both operands are of a builtin integer type or a defined type with a builtin integer as its underlying type, the expression must be a checked assignment, bitwise operator, or logical shift, and its result is the same type as the operands.
Constants
An untyped integer constant can be assigned to any
checked
,wrapped
, orunbounded
integer type that can represent its value.Unfortunately, in order to comply with https://github.com/golang/proposal/blob/master/design/28221-go2-transitions.md#language-changes, the inferred type of a variable initialized from an untyped constant must remain the built-in
int
.The text was updated successfully, but these errors were encountered: