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

clarify spec/implementation for let: move or copy? #13140

Closed
timotheecour opened this issue Jan 14, 2020 · 9 comments
Closed

clarify spec/implementation for let: move or copy? #13140

timotheecour opened this issue Jan 14, 2020 · 9 comments
Labels
Documentation Content Related to documentation content (not generation). Garbage Collection

Comments

@timotheecour
Copy link
Member

timotheecour commented Jan 14, 2020

should let always move, always copy, or sometimes move and sometimes copy (and in which conditions, and for which --gc flags)?
should an object passing by let argument make a move or copy?

Clarifying this matters as APIs may make assumptions that may not reflect current/future reality and break at some point.

This is not clear right now and the current compiler at least sometimes makes a move instead of a copy.
If --newruntime or --gc:arc is used, whether a move or copy is used is currently obscure

my intuition was: for let b = expr(a), if nothing can modify a after that statement, then make a move, else make a copy. However that's not what I'm observing (neither with --gc:arc nor without that flag).

Example

this came up here: #13116 (comment)

type Foo = distinct string
type Bar = ref object
  foo: Foo

proc main(a: Bar)=
  let b = a.foo
  doAssert cast[int](a.foo.string[0].unsafeAddr) == cast[int](b.string[0].unsafeAddr)

let a = Bar(foo: "abc".Foo)
main(a)

it seems to me in this case at least that the move is safe, so the assert should (and in fact does) pass, but in #13116 (comment) @Araq says:

That's a bug, finally addressed in --gc:arc, the move is unsound.

however, the assert passes even with --gc:arc or --newruntime

Additional Information

However in the top-level snippet of current issue, both --newruntime , --gc:arc and no flags cause let to move instead of copy (ie the assert passes)

proposal

one simple proposal is as follows:

the only tricky point is backward compatibility, but at least it's simple to grok

@andreaferretti
Copy link
Collaborator

This is an important point to clear

@Clyybber
Copy link
Contributor

Clyybber commented Jan 14, 2020

There is no difference between let and var for the analysis.
We only do moves for non-global variables.
Your proposal misses the point (eliding destructors by moving).
EDIT: Checkout https://github.com/nim-lang/Nim/blob/devel/doc/destructors.rst

@timotheecour
Copy link
Member Author

There is no difference between let and var for the analysis.

see #2314, let and var behave differently

@Araq
Copy link
Member

Araq commented Jan 14, 2020

Back then let was designed to be compatible with parameter passing semantics and this means "do not copy". Now as it turns out, what is fine for parameter passing (even though strictly speaking that's unsound too) is way more dangerous for first class entities like "let variables", consider for example:

let x = obj.s
setLen(obj.s, 0) 
echo x.len # old obj.s sequence or the updated one?

What Nim tries to do here is to borrow from obj.s unless obj.s is mutated during the span of x's lifetime. So that's what my ideal Nim would do: Borrow unless considered unsafe, then do a copy instead. In other words, use Rust's model but remove its pain points. That's also what we effectively do with the design of sink parameters.

@Araq
Copy link
Member

Araq commented Jan 14, 2020

Oh and one more thing: For version the spec pretty much should say "whether a copy or a move is done is a quality of implementation issue". This has been true since a long time because let x = someGlobal always resulted in a copy, you cannot rely on a move.

@mratsim mratsim added the Documentation Content Related to documentation content (not generation). label Jan 18, 2020
@timotheecour
Copy link
Member Author

timotheecour commented Feb 5, 2020

but this seems like a bug, following code should use a move, not a copy since the last access to s is an assignment to a:

template toStr(x): untyped = $cast[int](x[0].unsafeAddr)
var a = block:
  let s = @[10, 11, 12]
  echo s.toStr
  s
echo a.toStr

but instead it makes a copy: addresses are different:
4456222808
4456222864

EDIT

actually I found a good workaround for this particular point, using move: following code works!

    var a = block:
      var s = @[10, 11, 12]
      echo s.toStr
      move(s)
    echo a.toStr

4489019480
4489019480 # same address

@timotheecour
Copy link
Member Author

timotheecour commented Mar 30, 2020

This has been true since a long time because let x = someGlobal always resulted in a copy, you cannot rely on a move

let x = someGlobal always resulted in a copy doesn't seem like it:

when true: # example from https://github.com/nim-lang/Nim/issues/13771
  var myGlobal = @[1,2,3,4,5]
  proc foo() =
    let myGlobalCopy = myGlobal # this currently does not make a copy
    myGlobal[0] = 123
    echo myGlobalCopy # @[123, 2, 3, 4, 5]
  foo()

@Araq
Copy link
Member

Araq commented Mar 30, 2020

Well it's what I remembered from the bug reports. I probably shouldn't have said "always". ;-)

@Araq
Copy link
Member

Araq commented Aug 27, 2023

The spec is clear now about this.

@Araq Araq closed this as completed Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Content Related to documentation content (not generation). Garbage Collection
Projects
None yet
Development

No branches or pull requests

5 participants