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

conversion to unsigned causes error in VM; works in RT #12700

Closed
krux02 opened this issue Nov 21, 2019 · 6 comments · Fixed by #23930
Closed

conversion to unsigned causes error in VM; works in RT #12700

krux02 opened this issue Nov 21, 2019 · 6 comments · Fixed by #23930
Labels
Regression unsigned VM see also `const` label

Comments

@krux02
Copy link
Contributor

krux02 commented Nov 21, 2019

Example

proc foo1(arg: int): string =
  let x = uint32(arg)
  $x

proc foo2(arg: int): string {.compileTime.} =
  let x = uint32(arg)
  $x

echo foo1(-1)
echo foo2(-1)

Current Output

/tmp/scratch.nim(17, 17) Error: illegal conversion from '-1' to '[0..4294967295]'

But without the {.compileTime.} annotation foo1 works fine and returns "4294967295".

Expected Output

If conversation to unsigned integers should be unchecked as proposed in the RFC, then the conversation to unsigend should be unchecked at compile time as well.

4294967295
4294967295

Possible Solution

  • Remove range check in semfold and vm as well

Additional Information

The example above is a stripped down version. A real world example would be more that there is some source code out there that you do not have full control over. You would want to use it, but at compile time. But you can't use it at compile time, because interally it uses the the conversion that is range checked at compile time and not range checked at runtime. And then you have to make a PR request to change the conversation into a cast. And maybe get into descussions about cast being ugly and compile time evaluation isn't that important etc. It is just friction that can be avoided.

This problem is also very annoying for me personally, because I spent a lot of time in the Nim compiler to ensure that both the VM and the runtime behavior of integers is exactly the same. I wrote many tests that ensure in many placed exact same behavior at runtime and compile time. And here the behavior was explicitly broken for reasons that I don't I can't see as important.

This is the commit that introduced that regression c98e0e2

@Araq
Copy link
Member

Araq commented Nov 21, 2019

It's exactly as the spec says and you know it.

And then you have to make a PR request to change the conversation into a cast. And maybe get into descussions about cast being ugly and compile time evaluation isn't that important etc. It is just friction that can be avoided.

All that happens: You add an explicit bitmask, no need to discuss things, no need for cast.

@Araq Araq closed this as completed Nov 21, 2019
@krux02 krux02 reopened this Nov 21, 2019
@mratsim
Copy link
Collaborator

mratsim commented Nov 21, 2019

The idiomatic way should be high(uint32) especially now that high(uint64) is working.

Also -1 is signed and conversion from signed to unsigned is checked. It's when you stay in the unsigned domain that it is unchecked.

@Araq
Copy link
Member

Araq commented Nov 22, 2019

Also -1 is signed and conversion from signed to unsigned is checked. It's when you stay in the unsigned domain that it is unchecked.

No, now it's not checked at runtime and only checked at compile time.

@timotheecour timotheecour changed the title consistency break between VM and runtime behavior conversion to unsigned causes error in VM (not RT) Dec 6, 2020
@timotheecour
Copy link
Member

timotheecour commented Dec 6, 2020

  • this is a valid bug report, there's no reason to change semantics for VM, it should behave like RT to minimize the need for when nimvm: ... workarounds. VM bugs are getting fixed, and so should this.

  • note also that uint32(arg) returns -1 for arg = -1 in js, this is a related but different issue(edited: fixed by fix stringify unsigned integer in JS and JS VM #17086)

@timotheecour timotheecour added the VM see also `const` label label Dec 6, 2020
@timotheecour timotheecour changed the title conversion to unsigned causes error in VM (not RT) conversion to unsigned causes error in VM (and different results in js); works in RT Dec 6, 2020
ringabout added a commit to ringabout/Nim that referenced this issue Feb 19, 2021
@ringabout ringabout changed the title conversion to unsigned causes error in VM (and different results in js); works in RT conversion to unsigned causes error in VM; works in RT Feb 19, 2021
Araq pushed a commit that referenced this issue Feb 19, 2021
* fix js unsigned integer

* better

* ref #12700 add testcase
ardek66 pushed a commit to ardek66/Nim that referenced this issue Mar 26, 2021
* fix js unsigned integer

* better

* ref nim-lang#12700 add testcase
@ringabout
Copy link
Member

ringabout commented Nov 28, 2021

ref #18900

static: # illegal conversion from   '-2147483648' to '[0..4294967295]' 
  let x = low(int32) 
  echo uint32(x) 
static: # pass 
  let x = low(int64) 
  echo uint64(x)

While

const x = uint64(-1) # Error: -1 can't

@bung87
Copy link
Collaborator

bung87 commented Oct 30, 2022

when use nim check below case, I get messed errors.

static: 
  const x = low(int32) 
  echo uint32(x) 
static: # pass 
  const x = low(int64) 
  echo uint64(x)
t6549.nim(7, 14) Error: -2147483648 can't be converted to uint32
t6549.nim(7, 14) Error: conversion from int32 to uint32 is invalid
t6549.nim(7, 14) Error: conversion from int32 to uint32 is invalid
stack trace: (most recent call last)
t6549.nim(7, 14)         t6549
t6549.nim(7, 14) Error: illegal conversion from '-2147483648' to '[0..4294967295]'
t6549.nim(10, 14) Error: -9223372036854775808 can't be converted to uint64
t6549.nim(10, 14) Error: cannot convert -9223372036854775808 to uint64

@Araq Araq closed this as completed in f0e1eef Aug 11, 2024
narimiran pushed a commit that referenced this issue Sep 13, 2024
fixes #14522
fixes #22085
fixes #12700
fixes #23132
closes #22343 (succeeded by this PR)
completes nim-lang/RFCs#175

follow up #12688

(cherry picked from commit f0e1eef)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Regression unsigned VM see also `const` label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants