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.

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: Go 2: make int type panic on overflow #31500

Closed
ianlancetaylor opened this issue Apr 16, 2019 · 23 comments
Closed

proposal: Go 2: make int type panic on overflow #31500

ianlancetaylor opened this issue Apr 16, 2019 · 23 comments
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

Proposal: change the type int to panic on overflow.

The type int does not have a defined size. That means that people do not expect a particular overflow behavior. It is also the default type of integer constants, and the result type of len, cap and copy, and the key type of range on a slice, array, or string, so it appears frequently in Go programs.

Catching integer overflow will catch many cases where a program behaves incorrectly. Overflow in int values is unexpected but does sometimes occur. When it does occur, it is normally a bug in the program. Panicking rather than wrapping will help Go programmers find these bugs.

This idea arose in the consideration of adding checked integer types, which panic rather than wrap on overflow. A problem with adding new checked integer types is that, as noted above, int is the implied type for several operations. New types would require explicit conversions from int to the new type. It's unlikely that people would use them, so code would not become safe.

This proposal does not preclude adding more checked integer types in the future, as suggested in #30209 and #30613. It only addresses the handling of the special type int itself.

This would be a change in current program behavior, but since the type does not have a defined size, a program that relies on the wrapping behavior of int is likely to be subtly buggy when run on systems with different sizes of int.

This proposal is a simpler path forward compared to #19623, #19624, #30209, #30613.

A disadvantage of changing only int would be that people might assume that all integer types panic on overflow. Unfortunately, changing the behavior of the other integer types seems much more likely to break existing programs.

@ianlancetaylor ianlancetaylor added LanguageChange v2 A language change or incompatible library change Proposal labels Apr 16, 2019
@ianlancetaylor ianlancetaylor added this to the Proposal milestone Apr 16, 2019
@networkimprov
Copy link

networkimprov commented Apr 17, 2019

Any refs to research on the frequency of int-overflow defects?
Do they tend to occur with other bugs, e.g. infinite loop?

Any thoughts on performance impact of checking every change to every int?

Could one enable/disable this with a build flag?

@alanfo
Copy link

alanfo commented Apr 17, 2019

What about the uint and uintptr types which also don't have a defined size? It would seem odd for the int type to panic on overflow but for the corresponding unsigned types to wrap.

A possible compromise between this and your other proposal (#30613) would be to leave int alone but introduce just one new type oint (or whatever one wants to call it) which panics on overflow. An explicit conversion from int would still be required but that might be a price worth paying in the minority of cases where one was worried about a 'silent' overflow occurring.

@ianlancetaylor
Copy link
Contributor Author

I do not have any stats on the frequency of integer wrapping errors. Good question. Not sure how to get those stats.

On x86 the performance impact is an extra instruction, a correctly predicted branch, after most arithmetic operations. I doubt the performance impact would be significant, but we will have to see.

I do not think there should be a build flag for this, any more than there is for other language features.

The uint and uintptr types are both unsigned. For unsigned types wrapping is generally considered less surprising.

The purpose of this proposal is to make most programs avoid overflow without requiring extra work. Adding a new type would defeat that goal. If people are worried about overflow, then they will write code that does not overflow, regardless of whether we add a new type or not. The point is to help people who are not worried about overflow, but should be.

@kybin
Copy link
Contributor

kybin commented Apr 19, 2019

could we check this on compile time instead of panicking in runtime?

I mean something like this.

func add(i int) int {
    i += 1
    return i // overflow of an integer not checked error
}

func add(i int) int {
    i += 1
    if overflowed(i) {
        ...
    }
    return i // ok
}

@ianlancetaylor
Copy link
Contributor Author

@kybin There is a proposal along those lines at #30209. This proposal is not that one.

Personally I not convinced that such a check needs to be built into the language. I think it's rare that people want to check for overflow. And for people who do want to check, it can be checked before the operation. And of course, with this proposal, one could write

func CheckedAdd(a, b int) (sum int, ok bool) {
    defer func() {
        if recover() != nil {
            sum = 0
            ok = false
        }
    }()
    return a + b, true
}

although it would be more efficient to write an explicit check beforehand as one can already do today.

@kybin
Copy link
Contributor

kybin commented Apr 20, 2019

@ianlancetaylor I've missed it, sorry.

Another thought is, we can add -overflow flag to build command, so overflows are checked if the flag is on. (just like go build -race).

I support your proposal, which is panicking on integer overflow. It might be little better if we could check early when we want.

@ianlancetaylor
Copy link
Contributor Author

In Go we try to avoid adding knobs like a -overflow flag. We try to make things work as best we can without requiring people to figure out what options to use.

@kybin
Copy link
Contributor

kybin commented Apr 22, 2019

I agree to you said. Thank you.

@StephanVerbeeck
Copy link

I vote against this functionality.
Motivation

  • other programming languages don't have it (some even requiring inline assembler to detect the overflow CPU flag being raised)
  • the performance impact is not neglectable.
  • functionality might not be easy to add consistently cross-platform

Although I do see the reason why this is asked. The reasons why GOlang should not do it is the same as all the other languages earlier reached the same conclusion (not to support it).

@seebs
Copy link
Contributor

seebs commented May 12, 2019

coming from a C background, where int has always been undefined-on-overflow, and most users are still surprised by it: i think i like this idea, but find it slightly-surprising for it to apply to int, but not to int32 or int64. i'd be concerned about impact on existing code, but also i think it would indeed make code safer and mostly catch actual errors.

@bcmills
Copy link
Contributor

bcmills commented May 13, 2019

The type int does not have a defined size. That means that people do not expect a particular overflow behavior.

That is true in general, but note that it is possible today to detect the size of int and vary the behavior of the program accordingly. (For example, the math/big package does something similar with uint.) So I don't think we can assume that all uses of int today are agnostic to overflow behavior.

As much as I would like us to be able to adopt this proposal, it certainly seems to run against the constraints described in https://github.com/golang/proposal/blob/master/design/28221-go2-transitions.md#language-redefinitions.

@ianlancetaylor
Copy link
Contributor Author

That is true in general, but note that it is possible today to detect the size of int and vary the behavior of the program accordingly.

Fair enough. If there are plausible examples of programs that do this for int, not uint, then we can not adopt this proposal.

To expand, it is clear that this proposal changes the behavior of existing programs. And clearly it will fix some programs, or, at least, avoid certain classes of incorrect behavior. If we can convince ourselves that it will fix some real programs and not break any real programs, then perhaps we can proceed. But I'm open to counter-arguments.

@ianlancetaylor
Copy link
Contributor Author

A counter-argument to this proposal is that it introduces a panic path to code that currently doesn't have one. That raises the possibility of unexpectedly crashing a long-running server on an unlikely and untested path. In some cases that can lead to a denial of service attack via a query of death. That is, this proposal can change code that is silently incorrect into code that loudly crashes. While that is often a good tradeoff, sometimes it is not.

@bcmills
Copy link
Contributor

bcmills commented May 14, 2019

That raises the possibility of unexpectedly crashing a long-running server on an unlikely and untested path.

The usual example of such paths is logging statements, which may very well be untested and unchecked in existing code:

	log.Printf("wrote %d bytes", fooLen + barLen)

@bcmills
Copy link
Contributor

bcmills commented May 15, 2019

#30613 (comment) points out the similarity between dynamic overflow checks and race-detection.

So perhaps we could enable the overflow check for int only in -race mode, since race conditions are another case in which a subtle bug can unexpectedly crash a long-running server on an unlikely and untested path.

@ianlancetaylor
Copy link
Contributor Author

It's an interesting idea, but from a language perspective it means that integer overflow is no longer clearly specified. That might be OK but it seems awkward.

@MichaelTJones
Copy link
Contributor

Lots of answers here, all shared before.

How common is overflow? Find out for yourself in the Playground...
https://play.golang.org/p/xc8TdVBgmf
49.8047% (32640/65536) of additions overflow
96.9971% (63568/65536) of multiplications overflow

How central is this issue? It defines Niklaus Wirth's worldview on the matter, with much code in PASCAL and successors shaped by it. (The "math view" vs the "computer view)
https://groups.google.com/forum/#!searchin/golang-nuts/uint$20type$20safety$20in$20go/golang-nuts/zxwXc9EfvFU/T2ijCGyyAgAJ

Scroll down there to my post "Yes, there are cases for integer domain wraparound just as there are cases for exception at the limits."

Summary: (1) Silent wraparound is the right answer in general. (2) A special kind of variable that always checks (as an option in variable declaration or as a special checking-type) is a very good thing. (3) A way to test for exception is also a good thing.

#3 is always possible. So the question there is more about how much help to provide. I'd early on proposed:

sum,carry := a+b
difference,borrow := a-b
product,carry := a*b
quotient,remainder := a/b

which did not win favor, though it has come back around via the bits library and special function calls.

@bcmills
Copy link
Contributor

bcmills commented May 29, 2019

@MichaelTJones, when people are asking “How common is overflow?”, I suspect they mean “How often do variables overflow in Go programs in production?”, not “What fraction of the space of possible operands results in overflow?”.

@bcmills
Copy link
Contributor

bcmills commented May 29, 2019

The borrow / carry / remainder approach works great for multi-precision arithmetic, but that's not what most people are writing. For simple overflow checks, it seems more useful to have an operation or program behavior that applies to a whole expression tree, not just one individual operation.

@seebs
Copy link
Contributor

seebs commented May 29, 2019

I do like the idea of having the borrow/carry/remainder functionality; I would absolutely use it if it existed, even though that's separate from the desireability of checked-overflow (or for that matter saturating) math.

@MichaelTJones
Copy link
Contributor

If the question is “how often in apps” then it is too general to answer technically. It would seem the soft answer would be “never in cases that matter, because that would be considered a bug in program behavior.” The occurrence in unnoticed places, not seen or addressed as bugs, is clearly not “never” in a smart way but it is a kind of virtual never. On a pragmatic note, people use 64 bit signed ints these days as they loop from 1 to 10 and multiply by 365 days, say, so there is quite a bit of headroom to the 63 bit upper limit of 9,223,372,036,854,775,807.

@seebs
Copy link
Contributor

seebs commented May 29, 2019

I think there's probably a fair amount of intentional-overflow happening in code, but i'm not honestly sure how much. Obviously, much more in unsigned.

I suppose one strategy would be (1) implement it, (2) find out what happens.

@ianlancetaylor
Copy link
Contributor Author

Based on the objections mentioned above in

#31500 (comment)
#31500 (comment)

we are not going to adopt this proposal.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

9 participants