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

Incorrect overflow/underflow error in case statements #11551

Closed
kaushalmodi opened this issue Jun 20, 2019 · 12 comments

Comments

@kaushalmodi
Copy link
Contributor

commented Jun 20, 2019

Hello,

The below snippet used to work correctly before Nim 0.20.0:

Example

proc negativeOrNot(num: int): string =
    result = case num
    of low(int) .. -1:         
      "negative"
    else:
      "zero or positive"
  echo negativeOrNot(-1)
  echo negativeOrNot(10000000)
  echo negativeOrNot(0)

Current Output

Error: unhandled exception: over- or underflow [OverflowError]

Expected Output

negative
zero or positive
zero or positive

Additional Information

Interestingly, this works:

proc negativeOrNot(num: int): string =
    result = case num
    # of low(int) .. -1:         # Error: unhandled exception: over- or underflow [OverflowError]
    # of (low(int)+1) .. -1:     # Error: unhandled exception: over- or underflow [OverflowError]
    of (low(int)+2) .. -1:       # works
      "negative"
    else:
      "zero or positive"
  echo negativeOrNot(-1)
  echo negativeOrNot(10000000)
  echo negativeOrNot(0)

It's not clear why low(int) and low(int)+1 cause underflow, but not low(int)+2.

$ nim -v
Nim Compiler Version 0.20.99 [Linux: amd64]
Compiled at 2019-06-20
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: 678beb8ef982a3477018ff7c8669c4a9c6f77545
active boot switches: -d:release
@kaushalmodi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

I get a similar overflow error for of 1 .. high(int):, of 1 .. (high(int)-1): and of 1 .. (high(int)-2):, but not for of 1 .. (high(int)-3):.

@dumjyl

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

It just silently overflowed before :-/

@kaushalmodi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@dumjyl I am fine with that error if I understand why high(int), high(int)-1 and high(int)-2 overflow but not high(int)-3.

@kaushalmodi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Moved the older comment here to a separate issue: #11552.

@kaushalmodi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@dumjyl

It just silently overflowed before :-/

If you go to https://scripter.co/notes/nim/#case-statements and scroll down a bit to the positiveOrNegative proc, that whole snippet actually worked and returned the correct results.

That example is originally from https://nim-by-example.github.io/case/.

@dumjyl

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Before when we overflowed the result would be requiring an else even if not necessary, which is why your example worked. Edit: to clarify, this error is just about getting the code through sem, and doesn't effect runtime results.

There a couple places where improvements could be made to case, ones i've found:

# Error: unhandled exception: over- or underflow [OverflowError]
case 1:
of low(int)..high(int): discard # use to require else instead of erroring.

# Invalid values for type, there are a couple variations with invalid types.
case range[1..1](1):
of 2: discard

# Error: expression '"empty"' is of type 'string' and has to be discarded
# use efWantValue for a better error
var x = case "1":
of "": "empty"

The way case coverage is checked is seeing if the length of the ordinal type matches the sum of the lengths of the branch values. Possible solutions: using BiggestUInt as the counter, saturating arithmetic, or storing values as integer ranges.

@narimiran

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@kaushalmodi, @dumjyl is right when he says:

It just silently overflowed before :-/

Bootstrap your Nim with -d:danger flag (which turns off overflow checks, like release was doing pre-0.20) and you'll see it still compiles.

@kaushalmodi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@narimiran I wouldn't want to have code deployed with overflow checks disabled.

The root question is why is it overflowing/underflowing at low(int) and high(int), the low and high values are technically within the bounds of the given type, right?

Also the mysterious behavior with ..

It's not clear why low(int) and low(int)+1 cause underflow, but not low(int)+2.

@GULPF

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

The issue is that the number of cases covered in a case statement/expression is stored in a int64, and the number of covered cases can be larger than high(int64). Here's an example where it causes a bug even with -d:danger:

proc foo(T: typedesc) =
    var x: T
    case x
    of low(T) .. high(T): discard
foo(int8) # Works
foo(int64) # 'not all cases are covered' with -d:danger, otherwise overflow
@krux02

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Hmm, well I guess we finally need to have an integer type in the compiler that exceeds the capacity of all natively supported integer types.

@kaushalmodi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@GULPF

The issue is that the number of cases covered in a case statement/expression is stored in a int64

Should that be stored in uint64 instead? That will automatically double the count storing capacity.

@kaushalmodi kaushalmodi changed the title [regression] Incorrect overflow/underflow error? Incorrect overflow/underflow error in case statements Jun 21, 2019

@krux02 krux02 self-assigned this Jun 26, 2019

@kaushalmodi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

@krux02 This is fixed too, by your 7d5d9f7 PR.

Can you please add this as a test case and close this too?

krux02 added a commit to krux02/Nim that referenced this issue Jul 9, 2019

@Araq Araq closed this in eb059fa Jul 9, 2019

narimiran added a commit that referenced this issue Jul 15, 2019

closes #11551 (#11693)
(cherry picked from commit eb059fa)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.