[Pitch] Move Function + "Use After Move" Diagnostic

I don't know how drop is implemented in Rust. I had a short look at the implementation in Swift and it is done in SIL with Builtin.move, with other words at a much lower level than lexical analysis and pretty far from scopes.

For my limited knowledge of SIL I am pretty sure that for an implementor it would be possible to introduce a rule, that the result of Builtin.move cannot be used more than once.

That said, the name move leaks implementation details.

Just an FYI, I am updating the toolchain with some new stuff I came up with and will update the pitch as well. Specifically, I added support for new inout semantics and proper handling for let where you define the value later. So be aware that one may read again and see new stuff. I will add a change log to the top after I do this.

3 Likes

A quick response that I hope will help the discussion:

I wouldn't think of move being as being passed a binding. Instead the way to think about it is that you are moving the current value of the binding out of the binding and returning that value as a return value. move is not returning a copy of the value in the binding... it is returning the actual value that was in the binding. That is why we need the diagnostic to enforce that one can no longer use the binding afterwards since the binding value would no longer be valid. Remember that move is applying __owned for its argument (which I am going to be formalizing soon hopefully into the language) meaning it takes in the value at +1. Thus its argument comes in at +1 and is returned at +1.

We are on purpose not using discardable result since we want the user to explicitly have to show that they do not want to use the result. This is an expert feature where we want users to be explicit. So discardable result is inappropriate.

4 Likes

You have this reversed. Both the implementation and the syntax reflect the semantics. Read y = move(x) as: the value is moved out of x and placed into y.

Because the value was removed from x, the variable can no longer be used. Whether the value now in y is the original value of x or a copy of that value is truly an implementation detail that is hidden.

8 Likes

I'm very confused by this. The move into y has no effect on what happens to y when you use it later. Your:

print(y)
print(y)

Is exactly the same as the pitch's:

useX(other)
consumeX(other)

Okay, I can get behind that way of thinking about it—but either way, move is still a function which somehow acts on (or is "applied to") bindings:

which isn't something that exists for Swift functions generally, so it just feels a bit weird to model this as a function when its semantics deviate pretty dramatically from functions in Swift today. I think @fclout has good questions in this regard:

AFAICT we don't really have the terminology to talk about how move behaves as a function in the language today except to say "oh you shouldn't really think of it like a normal Swift function, it's just compiler magic."

That said...

I'm glad to hear we are pulling some of these underscored qualifiers into the language proper. If the end goal is to have Swift end up in a place where the ownership features are fleshed out enough that any user could define their own equivalent move function, I don't have any qualms about the fact that in the interim it would have to be supported by some compiler magic.

Like I said, that sounds like a great justification for the drop function. It doesn't make sense to me to simultaneously argue that it's an expected/supported use case for the result of move to be discarded, but it's not appropriate to mark it @discardableResult. I'm also not that compelled by the argument that it's an expert feature—if that's the case, shouldn't we trust experts to use or discard the result as appropriate?

I guess I'm just not seeing the danger of an erroneously discarded move result. If a user meant to write

let y = move(x)

but instead wrote just

move(x)

what's the danger? It seems to me like the diagnostics would make it apparent quite quickly what the issue is.

6 Likes

TLDR: The compiler emits errors for moves used in code that it doesn't recognize. So if it compiles, all usages were supported by the compiler.

So the way this works is that in order to support this we introduce into SIL "marker" instructions at the Builtin.move callsite. These marker instructions are then in the process of being proven safe by the checkers converted to other normal SIL instructions. Then after we run the checkers, we look for any marker instructions that were not touched by a checker and thus remain in the IR and emit an error diagnostic saying that the checker doesn't support this usage of move. The error diagnostic is placed on the call to the offending move. So you will not have to guess. If the code compiles ok then all moves must have been properly resolved since no diagnostic was emitted (modulo compiler bugs of course). I also want to probably have more specific errors. Right now there is just one generic, not supported error msg.

4 Likes

:heart_eyes: I love it. It's such an important feature; I have so many things that could make use of this.

Some quick points:

Wait, what? Is Ownership SSA enabled for all code now? That's incredible! Congratulations!

ivars would be very nice, especially together with _modify. Both WebURL and Swift-System use a pattern where COW storage is "moved" in to a wrapper type and moved back out again. Example 1, Example 2.

Both of the lines I linked to show the original storage being occupied by dummy storage - because we can't really express the steps of:

  1. Move self.storage in to the wrapper
  2. Yield the wrapper
  3. In the defer block, transfer the wrapper's storage back to self.storage

What would it take for such a this kind of thing to be supported? Is the flow-sensitive checking able to handle defer?

4 Likes

@Michael_Gottesman this looks fantastic, I'm excited to see it! Much like @Karl I'm most excited to see it generalised to other usage sites: patterns where we move values into and out of long-term storage are the most important places where we could benefit from move. Regardless, I shouldn't criticise you building a foundation because I wish you'd built a house: one step at a time is great!

8 Likes

The diagnostic:

test.swift:7:15: error: 'x' used after being moved
  let x = ...
          ^

Is misleading. It looks like it’s saying the problem is here. Could the let line be marked as the original declaration?

1 Like

I don’t see how move-only types and other ownership markers would make it possible to write a move for regular (copyable) types, unless there’s an attribute that amounts to “this function behaves like move”. But maybe that’s just a failure of my imagination.

2 Likes

Thank you for this, this is really fantastic progress! I can't wait to use it!!

2 Likes

Once a binding is moved can it be re-initialized with a new value?

let x = 42.56
let y = move(x) // x is ended at this point
let x = 42 // is this reuse of the variable name valid?
1 Like

Inspired by the question above:

func f(x: Int) {
var x = x
x += 1
_ = move(x)
print(x) // is x now the original x? Or invalid?
}
1 Like

This is invalid. You would not get a move error, you would get a redeclare error. Also, this is a trivial type so move doesn't make sense.

1 Like

x is an int, so that doesn't make sense from a move perspective. If x was a class or an array, this example would be invalid. You need to reassign before you call print x again

1 Like

I'll admit I don't really have an idea of this either—though I don't have a great idea of exactly what the up-to-date plan for Swift ownership looks like.

I am not entirely sure how to interpret this: would the following code be invalid or not?

let y = move(someFunc())

Does "doesn't make sense" mean that this use of move would produce an error? Based on how move is described in this pitch it doesn't seem obviously wrong to me (though perhaps a bit useless): the binding x would cease to be valid, and y would refer to the value previously referred to by x.

1 Like

Is it legal to do this:

var x: Something
x = move(x)

I can't think of a reason for someone to do this, though.

Let me make sure I understand: this is basically a way to explicitly perform a task normally done automatically: removing a binding after it is last accessed. Since the compiler normally has discretion about when that is done, having guaranteed behavior in this form may be useful in some manner. More importantly, this allows developers to prevent accidental usage of a binding later in the program, potentially making it easier to reason about.

That all seems perfectly reasonable to me, but I think naming it Swift.move() is a serious mistake. There needs to be more information at the call site, particularly since most programmers are unlikely to ever use it. Maybe something like Swift.moveBinding(of:)?

3 Likes

yes. It would cause an error (I just checked... btw you should take a look at the toolchain I posted and try some of these questions out with it).