SE-0366: Move Function + "Use After Move" Diagnostic

tera · 2022-07-28T21:52:11.016Z

ksluder:

You are proposing new behavior for return inside a do block, right? That’s a pretty big change to existing syntax.

For me it feels a smaller and simpler change (both to understand and to implement) compared to the pitch proposal.

Besides there are a couple of options available today, that allow ending variables lifetime early.

// 1. not so nice, but available now:
let other: T
do {
	let x = ...
	useX(x)
	other = x
} // `x` lifetime ended
// `x`  is moved to `other`

// 2. quite ugly, but available now:
let x = ...
useX(x)
let other = x  // `x`  is moved to `other`
guard let x = () as Void? else { fatalError() }  // previous `x` lifetime ended

// 3. available today, not so bad:
var other = { () -> T in
    let x = ...
    useX(x)
    return x
}()   // `x` lifetime ended
// `x`  is moved to `other`

// 4. might be available in the future (e.g. https://github.com/apple/swift/blob/main/userdocs/diagnostics/complex-closure-inference.md):

var other = {
    let x = ...
    useX(x)
    return x
}()  // `x` lifetime ended
// `x`  is moved to `other`

// 5. future ideal, listing for completeness:
var other = do {
	let x = ...
	useX(x)
	return x
} // `x` lifetime ended
// `x`  is moved to `other`

By a way of analogy, it feels like having a programming language that only has a higher level "forEach" statement, and as we sometimes need a lower level alternative we are now having a discussion on introducing a "goto" statement without considering "while" and "switch" statements first, which might be just enough for a typical task at hand. (And remember, even if goto can do much more compared to what "while"/"switch" can - goto is still considered evil and we don't have it in modern programming languages.)

I'd be very happy to be proven wrong and see some killer use cases that show superiority of "move" approach compared to nesting, just what I've seen so far (e.g. in the pitch description) doesn't look like a killer use case example.

Joe_Groff · 2022-07-29T03:53:09.840Z

I'd like to offer the following revisions in response to the discussion so far:

github.com/apple/swift-evolution

Revisions in response to review feedback

apple:main ← jckarter:move-review-revisions

opened 03:52AM - 29 Jul 22 UTC

jckarter

+142 -97

- `move x` is now proposed as a contextual keyword, instead of a magic function … `move(x)`. - The proposal no longer mentions `__owned` or `__shared` parameters, which are currently an experimental language feature, and leaves discussion of them as a future direction. `move x` is allowed to be used on all function parameters. - `move x` is allowed as a statement on its own, ignoring the return value, to release the current value of `x` without forwarding ownership without explicitly assigning `_ = move x`.

move x is now proposed as a contextual keyword, instead of a magic function
move(x).
The proposal no longer mentions __owned or __shared parameters, which
are currently an experimental language feature, and leaves discussion of them
as a future direction. move x is allowed to be used on all function
parameters.
move x is allowed as a statement on its own, ignoring the return value,
to release the current value of x without forwarding ownership without
explicitly assigning _ = move x.

ksluder · 2022-07-29T05:00:42.175Z

Joe_Groff:

move x is allowed as a statement on its own, ignoring the return value,
to release the current value of x without forwarding ownership without
explicitly assigning _ = move x.

Does this result in a parse ambiguity? What’s the type of { x in move x }?

If the only reason for this concession is to avoid _ = move, I don’t think that’s a strong motivation._ = «expr» is idiomatic Swift. If there’s a more fundamental reason a non-expression version of move is needed, then I suggest the spelling drop x.

Nikolozi · 2022-07-29T05:27:29.858Z

Joe_Groff:

move x is now proposed as a contextual keyword, instead of a magic function
move(x).

I'm happy to see this change. I think, a contextual keyword makes more sense. In the roadmap post, yield is written as yield _x and not as yield (_x). So, to me, move x looks consistent with it.

Also, in the roadmap, there was this bit of code:

// Here, we can `move` the second argument, since it is the final
    // use of `x`
    consumeTwice(first: copy(x), second: move(x))

I'm wondering about the copy() function. Is this going to be a feature, complementary to move, or is this just for the sample code, to indicate that x will be explicitly copied before passing it into the function? If it's going to be a feature, I guess it would make sense for it to be also spelled as copy x?

xwu · 2022-07-29T12:32:47.472Z

ksluder:

Does this result in a parse ambiguity? What’s the type of { x in move x }?

We already encounter this scenario with @discardableResult functions used with implicit return.

I'd expect { x in move x } to have the same type as { x in foo(x) } where the function is declared @discardableResult func foo<T>(_: T) -> T.

xwu · 2022-07-29T12:45:16.493Z

The revision states:

move x + y // Parses as (move x) + y

I wonder if there could be some more justification of this choice, as it behaves differently from try, etc.

Might it be preferred to have move x + y parse as move (x + y), particularly since there would be issues using explicit parens due to ambiguity with hypothetical functions named move? Users could specify (move x) + y explicitly if that's what they want.

Alternatively, is there room to make move have undefined precedence with standard operators and therefore always require parens?

scanon · 2022-07-29T14:29:53.291Z

(x + y) doesn't have a binding, so move (x + y) wouldn't mean anything. Requiring parens might be reasonable, but parsing as move (x + y) seems nonsensical.

As a side note, I don't really want to get into bikeshedding, but I think I'd prefer to call this operation unbind or unlet. move is something of a term-of-art, but it's a bad name and leads to all sorts of confusion, as this thread has shown.

ksluder · 2022-07-29T15:14:34.999Z

Bikeshed

unlet raises the specter of unvar.

Python uses del, if we want a familiar word that also happens to be 3 letters.

tera · 2022-07-29T18:22:43.338Z

xwu:

move x + y // Parses as (move x) + y

even this:

move x+y

Maybe just prohibit it?

scanon:

As a side note, I don't really want to get into bikeshedding, but I think I'd prefer to call this operation unbind or unlet.

"unlet" is good, just note that in the pitch, this is allowed:

let x = ...
move x
x = ...

while "unlet" word suggests the last "x = ..." is an error, and it should be written as:

let x = ...
unlet x
let x = ...

In other words "unlet" word suggests returning to "stage 0" while the pitch "move" suggests returning to "stage 1":

// stage 0
let x: Int
// stage 1
x = 1
// stage 2

beccadax · 2022-07-29T18:48:51.232Z

This revision is much better—my remaining comments are almost nitpicks.

Joe_Groff:

move x is allowed as a statement on its own, ignoring the return value,
to release the current value of x without forwarding ownership without
explicitly assigning _ = move x.

I’m not sure I like this. When I imagine looking at this code without any foreknowledge of move:

func f(_ x: SomeClassType) {
     move x
     useX(x) // !! Error! Use of x after move
 }

I think I would find move x completely mystifying. Is it moving a file? Is it animating something? Is it transferring an object between concurrency domains? Is it migrating a distributed actor to the current node? Does it do something specific to SomeClassType?

I think requiring the discarding assignment helps clarify move a little because it at least suggests that the usual nature of move is to produce a value, and we are simply ignoring that value in this case. But—since bikeshedding has now come up—a different name for move might also help clarify the issue. For instance, I think any of these would be more understandable than move without the discarding assignment:

    take x
    transferownership x
    handover x
    consume x

    useX(x) // !! Error! Use of x after <whatever>

(To be clear, I’m not trying to argue for these specific names—just demonstrate that some name change might resolve this issue.)

move x.y.z // Syntactically OK (although x.y.z is not currently a movable binding)
move x[0] // Syntactically OK (although x[0] is not currently a movable binding)
move x + y // Parses as (move x) + y

I assume you plan to achieve this by adding a new production to prefix-expression that’s analogous to the one for in-out-expression. Should we amend a description of that into the proposal?

Andrew_Trick · 2022-07-29T21:56:10.859Z

Jumhyn:

I take your point that this seems like a silly optimization in isolation, but are we so sure that in the full complexity of a real-world program, such behavior wouldn’t emerge?

It's important to me that the semantics of the move intrinsic are identical the semantics of "consuming" any move-only value. Whatever behavior we specify won't be specific to move. I'm also saying that this optimization is not only silly but harmful even when we don't have move-only values. So, we'd want to say something about the side effects of potential object deinitialization in general.

I would also like to formally specify away the bad optimization. But I don't think we can invent the formal language that we need in this proposal. We need a separate "deinitialization" semantics proposal. What I really don't want to do is make some broad statement about "side effects" that is specific to the move intrinsic.

At any rate, this is great feedback.

Jumhyn · 2022-07-29T22:41:18.904Z

Andrew_Trick:

It's important to me that the semantics of the move intrinsic are identical the semantics of "consuming" any move-only value. Whatever behavior we specify won't be specific to move.

Okay, this helps crystallize things a bit more for me.

Andrew_Trick:

I would also like to formally specify away the bad optimization. But I don't think we can invent the formal language that we need in this proposal. We need a separate "deinitialization" semantics proposal. What I really don't want to do is make some broad statement about "side effects" that is specific to the move intrinsic.

This also makes sense, though I wonder if it is in conflict with the proposal text. The proposal raises the point several times that move serves not only as a tool useful against modifications of the source, but also against optimizer behavior and future language implementation changes. If we don't have the formal language to guarantee that move protects against the language implementation in this way, perhaps it would be appropriate to specify move for now as a source-level guard, and then further specify the semantics once we flesh out the initialization semantics?

Even as just a source-level guard, though, I think the potential for this 'harmful' optimization speaks to a broader discomfort that I've been feeling but haven't been able to put into words until just now. Again returning to the motivating example for move:

func test() {
  var x: [Int] = getArray()
  x.append(5)
  
  var y = x
  longAlgorithmUsing(&y)
  consumeFinalY(move y)

  x.append(7)
}

the proposal sells move as a solution to the problem of "what if someone tries to use y after x.append(7) and accidentally causes an implicit copy"? But that problem is indexed on two bindings, x and y, and the solution is unilateral: it only affects usage of the y binding. Nothing stops an accidental insertion of an x.append(6) within the 'critical' section between the y = x alias and the final move of y, just like the potential 'optimization' discussed before:

var y = x
longAlgorithmUsing(&y)
x.append(6) // oopsie
consumeFinalY(move y)

It seems like the robust solution to the problem posed starts looking more like a Rust-y lifetime system:

var y = strawmanDontUseTheseAtTheSameTimeKeyword x
longAlgorithmUsing(&y)
x.append(6) // error: 'x' cannot be modified while 'y' is still alive (used below)
consumeFinalY(y) // OR error: 'y' used after lifetime implicitly ended (by use of 'x' above)

x.append(7) // OK, lifetime of 'y' implicitly ended here

I haven't really thought this through, but it does seem like it would be better if we could properly express the relationship we're worried about rather than relying on ending the lifetime of a single binding.

Andrew_Trick · 2022-07-29T22:56:40.602Z

Jumhyn:

var y = strawmanDontUseTheseAtTheSameTimeKeyword x
longAlgorithmUsing(&y)
x.append(6) // error: 'x' cannot be modified while 'y' is still alive (used below)
consumeFinalY(y) // OR error: 'y' used after lifetime implicitly ended (by use of 'x' above)

x.append(7) // OK, lifetime of 'y' implicitly ended here
I haven't really thought this through, but it does seem like it would be better if we could properly express the relationship we're worried about rather than relying on ending the lifetime of a single binding.

Looks like you've just reinvented the roadmap's "borrow variables":

A roadmap for improving Swift performance predictability: ARC improvements and ownership control Pitches

Swift's high-level semantics try to relieve programmers from thinking about memory management in typical application code. In situations where predictable performance and runtime behavior are needed, though, the variability of ARC and Swift's optimizer have proven difficult for performance-oriented programmers to work with. The Swift Performance team at Apple are working on a series of language changes and features that will make the ARC model easier to understand, while also expanding the bread…

Jumhyn · 2022-07-29T23:02:20.631Z

I think it would be slightly different, since in the motivating example for move x and y are semantically independent copies rather than one being a 'borrow' of another, but the relationship being expressed is very similar, yeah.

hassila · 2022-07-30T07:32:55.541Z

One more bikeshed following up which I think would feel natural if approaching swift without obj-c mental state:

release x

Hacksaw · 2022-07-30T08:30:23.880Z

I really like the nesting idea due to it falling neatly into the bracket defined scope usage, something which always made clear sense to me.

Hacksaw · 2022-07-30T09:09:30.075Z

disown x ?

lukasa · 2022-07-30T09:19:38.391Z

beccadax:

I think I would find move x completely mystifying. Is it moving a file? Is it animating something? Is it transferring an object between concurrency domains? Is it migrating a distributed actor to the current node? Does it do something specific to SomeClassType?

I agree with @beccadax (and @ksluder up-thread) that I actually think this has made the thing I was objecting to worse. I thought that _ = move(x) as a synonym for drop was inscrutable, but move x is definitely worse.

The following question is asked in a very tongue-in-cheek way but...are we avoiding adding another word for a reason? I feel as though there has been unstated resistance to using the word drop, or indeed any word other than move, though these revisions: many folks have said they don't love the spellings, but I don't recall seeing an articulation of why we should choose to continue to use the word move here.

Anyway, if we must use the word move then _ = move x is a clearer spelling, but I still think neither of these are better than simply using another word.

3a4oT · 2022-07-30T09:56:31.856Z

+1 to use drop as a keyword instead of move.

asdf · 2022-07-30T15:18:13.901Z

var y = move(x)

is it dropping or moving ?