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

[ARC] compile-time move() on a sink-annotated argument empties the original value even when it's unsafe #17199

Closed
ghost opened this issue Feb 28, 2021 · 4 comments · Fixed by #17348
Labels
ARC/ORC Memory Management VM see also `const` label

Comments

@ghost
Copy link

ghost commented Feb 28, 2021

This code works fine with refc but fails with arc because system has a different implementation for https://nim-lang.org/docs/system.html#%26%2Cseq%5BT%5D%2CT and with ARC the compiler seems to always pass a variable to a sink-annotated argument even when it's unsafe. Found in https://github.com/PMunch/protobuf-nim

Example

Original example

proc passSeq(data: seq[string]) = 
  # used the system.& proc initially
  let wat = data & "hello"

proc test = 
  let name = @["hello", "world"]
  passSeq(name)
  doAssert name == @["hello", "world"]

# works at runtime but not compile-time
static:
  test()

Example with & taken out of system (fails with both refc/arc):

proc merge(x: sink seq[string], y: sink string): seq[string] =
  newSeq(result, x.len + 1)
  for i in 0..x.len-1:
    result[i] = move(x[i])
  result[x.len] = move(y)

proc passSeq(data: seq[string]) = 
  # used the system.& proc initially
  let wat = merge(data, "hello")

proc test = 
  let name = @["hello", "world"]
  passSeq(name)
  doAssert name == @["hello", "world"]

# works at runtime but not compile-time
static:
  test()

Current Output

stack trace: (most recent call last)
/home/dian/Stuff/protobuf-nim/krr.nim(12, 7) krr
/home/dian/Stuff/protobuf-nim/krr.nim(8, 11) test
/home/dian/Things/Nim/lib/system/assertions.nim(68, 26) failedAssertImpl
/home/dian/Things/Nim/lib/system/assertions.nim(58, 11) raiseAssert
/home/dian/Things/Nim/lib/system/fatal.nim(53, 5) sysFatal
/home/dian/Things/Nim/lib/system/fatal.nim(53, 5) Error: unhandled exception: /home/dian/Stuff/protobuf-nim/krr.nim(8, 12) `name == @["hello", "world"]`  [AssertionDefect]

Expected Output

Successful compilation

$ nim -v
Nim Compiler Version 1.5.1 [Linux: amd64]
Compiled at 2021-02-28
Copyright (c) 2006-2021 by Andreas Rumpf

git hash: 26a6ceb34eb2bfca4e39dec2dac2d0a2cdc1bade
active boot switches: -d:release -d:danger
@ghost ghost changed the title [ARC] Sequence becomes empty after using system.& in compile-time context [ARC] move on a sink-annotated argument empties the original value even when it's unsafe Feb 28, 2021
@ghost
Copy link
Author

ghost commented Feb 28, 2021

I'm not sure if this can actually be considered an ARC bug since it can happen with both arc and refc - so maybe we should either make the move proc copy if it's executed in compile-time context, or just don't use move in any compile-time procs?

@ghost ghost changed the title [ARC] move on a sink-annotated argument empties the original value even when it's unsafe [ARC] compile-time move() on a sink-annotated argument empties the original value even when it's unsafe Feb 28, 2021
@ghost
Copy link
Author

ghost commented Feb 28, 2021

Example from #16847 :

macro Bug() =
  let a = @[123]
  discard @[456] & a
  let c = @[789] & a
  assert c[1] == 123

Bug()

@ghost
Copy link
Author

ghost commented Mar 8, 2021

Example from #17294:

proc recurse(strings:seq[string],recursionsLeft:int) =
  if recursionsLeft != 0:
    echo "Before call: " & $strings
    recurse(strings & @["another string"],recursionsLeft-1)
    echo "After call: " & $strings
  else:
    discard

static:
  recurse(@["string"],1)

@Araq
Copy link
Member

Araq commented Mar 8, 2021

Sink parameters are currently under the ARC/ORC umbrella.

Clyybber added a commit to Clyybber/Nim that referenced this issue Mar 12, 2021
@Clyybber Clyybber mentioned this issue Mar 12, 2021
Clyybber added a commit to Clyybber/Nim that referenced this issue Mar 25, 2021
Clyybber added a commit that referenced this issue Mar 26, 2021
* don't zero out in a move in the VM

* Add testcases for #17199

* Update tests/arc/tarcmisc.nim

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

* Update tests/vm/tissues.nim

Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>

* Fix test failures

* Fix test

* Fix tests

Co-authored-by: Andreas Rumpf <rumpf_a@web.de>
Co-authored-by: Timothee Cour <timothee.cour2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARC/ORC Memory Management VM see also `const` label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant