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

Variable can be moved while it's being passed as var #23768

Open
alex65536 opened this issue Jun 27, 2024 · 4 comments
Open

Variable can be moved while it's being passed as var #23768

alex65536 opened this issue Jun 27, 2024 · 4 comments

Comments

@alex65536
Copy link
Contributor

alex65536 commented Jun 27, 2024

Description

Repro:

type
  O = object
    l: proc(s: string)
    a: string
    b: string

proc dovar(a: var O, b: sink O) =
  echo "a: ", a.a, " ", a.b
  echo "b: ", b.a, " ", b.b
  
proc doit(o: sink O) =
  o.l(o.a)
  dovar(o, o)

let l = proc(s: string) = echo "l: ", s
doit(O(l: l, a: "1", b: "2"))

Nim Version

2.0.6. Also reproducible on current devel as of 2024-06-28.

Current Output

l: 1
a:  
b: 1 2

Expected Output

l: 1
a: 1 2
b: 1 2

Possible Solution

The compiler should not move the variable if it's being passed as var into the same function.

Currently, the possibility of move vs copy is checked with a simple DFA in compiler/dfa.nim, which handles only sequential variable use + branches. We should probably add something like borrow and endborrow in DFA, to indicate that the variable is already borrowed into a var argument and cannot be moved while the borrow is active.

Additional Information

expandArc shows that move occurs:

try:
  o.l(o.a)
  dovar(o):
    let blitTmp = o
    `=wasMoved`(o)
    blitTmp
finally:
  `=destroy`(o)

Various changes to the code can actually fix it. For example, adding discard o to the end leads to correct result, because it prevents moving. Also, if doit() is rewritten as

proc do2(s: string) = echo s
proc doit(o: sink O) =
  do2(o.a)
  dovar(o, o)

then =wasMoved is elided for come reason, and the program works fine. expandArc in this case works as follows:

do2(o.a)
dovar(o):
  let blitTmp = o
  blitTmp
@alex65536
Copy link
Contributor Author

alex65536 commented Jun 27, 2024

Kinda related to #23748. Though, it seems to be a more general bug, and #23748 can be fixed separately without fixing the general one.

@Araq
Copy link
Member

Araq commented Jun 28, 2024

dovar(#[byvar]# o, o) is UB in general. Simple cases should be forbidden at compile-time and the complex cases need a better borrow checker. Doing the borrow checking via the DFA might be a good idea.

@alex65536
Copy link
Contributor Author

dovar(#[byvar]# o, o) is UB in general.

Is it documented anywhere that it's UB?

@Araq
Copy link
Member

Araq commented Jun 29, 2024

Sure: https://nim-lang.org/docs/manual_experimental.html#aliasing-restrictions-in-parameter-passing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants