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

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).

This is legal. The right hand side will evaluate first before we do the initialization of x.

3 Likes

This is correct. You would need something to trigger the data flow analysis. That being said, I don't think that is really valuable and would just introduce duplication into the language. Being minimal in a language is really important.

This definitely feels like a good first step for move-only types. I do however find myself often needing something to move variables contained within types out. From what I can tell move(_:) can't be reasonably used on a property of a struct, class or actor right?

Would it be possible for an adaptation of this to take an inout parameter that is Optional to move out the value from where it is stored and replace it with nil?

Extra bonus points if this could move in a value to replace things. But perhaps that is a different thing...

2 Likes

I am actually updating the proposal with support for inout. The way it works is that you don't have to deal with optionals, I create a fake use in the checker on all terminators in the function. So you get the same semantics as if you were using a var, except with an implicit use.

public func performMoveOnInOut(_ p: inout Klass) { // expected-error {{'p' used after being moved}}                           
    let buf = _move(p) // expected-note {{move here}}                                                                         
    let _ = buf                                                                                                               
} // expected-note {{use here}}  

(I hinted about it above in one of my posts, but due to the outage over the weekend of swift-ci I wasn't able to land it yet/update the toolchain). Hopefully some time today.