Pre-RFC: Move-or-borrow elision

Motivation

Consider we have functions foo, bar, qux, which all have signature fn(Arc<X>) -> ():

let x = Arc::new(X);
foo(x.clone());
bar(x);

Now we have a new commit that additionally calls qux later on, so we have this diff:

- bar(x);
+ bar(x.clone());
+ qux(x);

This corrupts the git diff. And bar(x.clone()) is sometimes far away from qux(x) and mixed with other parameters, making the diff confusing.

This argument is somewhat similar to that of permitting trailing commas behind the last field of a struct.

Why don't we just clone all the time?

If we instead just clone every time, it would involve an unnecessary clone, which would be costly for some times (such as Box<X>).

It is not possible for the compiler to optimize away the clone in most non-derived Clone implementations. For example, consider the simple case of Rc, which is not even costly:

fn main() {
  let y = Rc::new(Y);
  x(Rc::clone(&y));
}
fn x(y: Rc<Y>) {
  println!("{}", Rc::strong_count(&y));
}

The compiler cannot optimize away the Rc::clone, because the original Rc is not dropped until the end of the program.

(In fact, this is an actual problem because calling drop(y) in x does not free the memory allocated for Y)

Example

It would be cool if we could write the above in the following syntax:

let x = Arc::new(X);
foo(x~.clone());
bar(x~.clone());
qux(x~.clone());

which is equivalent to the following:

let x = Arc::new(X);
foo(x.clone());
bar(x.clone());
qux(x);

Reference-level explanation

(Compare to method call expression a.b())

Syntax:

MoveableMethodCallExpression:

Expression ~. Ident ( CallParams ? )

Suppose expression resolves to a value of type T, and a method T::method(&self, call_params: CallParams) -> T is available in scope. Now compiler sees the expression expression~.method(call_params).

If expression can be immediately moved (i.e. it is the last occurrence of the expression, and it is not used in a loop subblock after expression is last written into), expression~.method(call_params) is compiled into {call_params; expression} (resolved the parameters call_params (?), but does not call .method).

Otherwise, expression~.method(call_params) is simply compiled into expression.method(call_params).

Note that a moveable method call on the same expression (or any intermediates in its path) always uses (be it borrow or move) the expression, so multiple calls to the same variable would only move the last one.

Concerns

Feasibility in compiler

I am not familiar with compiler internals, so I am not sure if it is easy for a compiler to tell whether a value can be moved. It seems intuitive to a human whether a value is "used" again later (simply check the word doesn't appear again later except in an else branch), but I am not sure if there are edge cases not covered.

Limitations

This only works for method call expressions. What if we want to use a similar trick in other expressions?

(Non-method) Call expressions

// how do we annotate that this can be optimized into `x` if `x` is moveable?
Arc::clone(&x)

In particular, what if the function takes multiple parameters? (See "Unresolved questions")

Others

I cannot think of any other use cases. Most operators are not applicable here since they take parameters by value already. Index can probably fit in here, but I don't know any practical case where x[&y] can be elided into y.

Unresolved questions

Resolving call parameters

Suppose we call x~.y(z()). If x is moveable, do we still need to call z()?

If we don't need to resolve the parameters, this greatly increases compiler complexity, because x~.y(x~.y(x2)) would be tricky to decide on.

For simplicity, if we want to get a feature like this quickly implemented, we could just require that x~.y() must not take any parameters since the only practical use case I can think of so far is Clone.

I have multiple issues with this:

  1. Special-casing methods like this with another magic sygil is not at all warranted by the argument that diffs get 1 line longer. (Overall the whole proposal seems severely undermotivated.)
  2. This re-introduces the very problem it's trying to solve, namely, non-locality. How does it improve the status quo if the first and the second use of the expression are so far apart that I'm having trouble keeping track of them? Why is it good that I have to remember yet another piece of state in my head, namely whether ~. moves or clones?
  3. And anyway, if your functions and scopes are so large, then you should consider refactoring your code into smaller, self-contained units instead of trying to "fix" (and actually support) bad coding style with a language feature.
  4. The fact that identical syntax is doing different things depending on what comes after it is a red flag, akin to INTERCAL's COMEFROM statement.

Overall, I'm strongly against this proposal. I think it would be detrimental to general code quality.

21 Likes

In your initial example, if you take the more common case of

let x = Arc::new(X);
foo(x);

I can guarantee you that nobody will write

let x = Arc::new(X);
foo(x~.clone());

just in case there will be a bar(x) call added later. But exactly that would be necessary to avoid diff "corruption" (a very misleading term for a semantic change showing up in the diff).

11 Likes

Well this is what Rust is doing with type inference like .parse, .collect, etc. It's not exactly that bad, particularly when the behaviour described here just means you could elide the call; users should not depend on this behaviour, just like you wouldn't rely on #[inline] having actually inlined or not. I can't think of any footguns if this is only used with cloning.

2 Likes

Well, I myself have come across cases like this at least 3 different times in the past month. I agree I'm a bit OCD to want this feature from the second usage onward though.

Alas, it's not – type inference is local in Rust.

Isn't what I'm discussing in this thread also local? It just needs to search for usages in the block that the variable to move-or-clone is located in. A value that is going to get returned should be considered as moved in the end, so that shouldn't cross function boundaries.

This seems to be very niche to be worth a new sygil. But I could see myself using a move_or_clone!(x) macro. I do not think that macro is definable in current rust. Perhaps we need first to something akin to is_variable_used_after_something!.

Another alternative would be to have either!{x, x.clone()}, for which the compiler could decide/infer to use one or the other. Of course, this would also have many complications.

2 Likes

An aspect you didn’t consider is interaction with closures. Usually it is clearly determined whether some use of a variable uses the variable by reference or by value. Your proposal breaks this determinism. For closures this distinction has a lot of implications.

Take this example:

let x = foo();
let y = Some(0).map(|n| bar(n, x~.clone()));

Does this capture x by reference or by value. Or would you want that to change based on context (i.e. whether there’s another x~.clone() in the following lines)?

2 Likes

It should be possible with a proc-macro. A less powerful subset of it could even be implemented, I think, purely at the syntactic level, without actually performing move analysis.

Procedural macros are expanded at the parsing stage. I don't think rust can infer enough about information to decide this at the parsing stage.

All interactions should assume it just borrows the variable. The mechanism is intended to optimize, so every other place can just treat the variable as borrowed. It's probably fine to replace borrow clones into moves at a very late stage since it has no side effects on other parts of the local function (in terms of borrowck logic)

While I have not been involved with Rust language design, I have learnt to cherish many of its choices over the past year. The picture that formed is that Rust makes important choices explicit and safe, this is the underlying design philosophy as far as I could discern. With this in mind, I’d like to respond to the motivation given in the opening message of this thread:

The diff shows exactly the changes introduced by the programmer. Cloning a value is an important part of what the processor will be doing when it executes this piece of code. Therefore, the diff is entirely correct. Saying that it is “corrupted” is therefore incorrect — I’d go as far as saying that the choice of words is not objective because corruption implies ill will.

Since by the above reasoning the motivation given for this feature request is invalid, I’d hope that we can all agree that further discussion is pointless unless a different motivation is given or my reasoning is proven incorrect.

6 Likes

While I think that @SOF3 wording could have been better, it is correctly pointing a relatively common issue. I certainly have codes in which I want to use x or x.clone(), where the move x is used the last. And those expressions can be far away from each other. That can make that when just adding/removing an instance you also have to check/change the surrounding ones, a superfluous time-consuming task. And sometimes I just give up and put x.clone() everywhere. I think it would be great if that labor would be done by the compiler in a proper way. I do not believe, however, the original post to be the proper way.

1 Like

I don't think this applies to optimizations. The point of this new syntax is to indicate that the compiler can be allowed to perform optimizations that elide the call. The compiler already performs a wide range of optimizations that elide calls, for example, not calling the Iterator of some collections if the loop body is a noop. This RFC merely provides a way to indicate that the optimization can be performed without caring about side effects such as not calling the clone impl, and I don't see how it's different from existing optimizations. In particular, users should not depend on this optimization for anything semantically distinguishable.

Why is everyone happy about #[inline] sometimes inlining and sometimes not, but not happy about sometimes automatically avoiding a useless clone?

IIRC there is a clippy warning for when you .clone() a variable where you could just move it instead. And also the approach of just always trying a move when you’re not sure will result in the compiler pointing directly at the relevant point of code in its error message. Error-driven development is pretty powerful in strongly typed languages.


As I said, in the context of closures the capture mode has a lot of (important) implications. One of the most important being whether only FnOnce is implemented or also FnMut or Fn.


For the record, the clippy lint seems to ignore “redundant” .clone()s in FnOnce closures, so it’s not perfect in that regard either. But it’s just a lint so (I guess) it doesn’t have to be perfect and support everything.

5 Likes

Nice, I forgot about the clippy warming. In that case that suits my purpose that I'm not only worried about adding a new clone in the future, but also removing it.

I agree that convenience is a concern here, I’ve experience this as well. My mode is to add clones where the borrow checker reminds me and remove them where the linter points this out — these are both mechanical changes, so some tedium could be removed by the tooling. But I remain convinced that this is no reason to change the language itself. Hiding a .clone() change in a code diff makes it invisible to reviewers, taking away sometimes crucial information.

I didn’t react to the “why not clone always?” rhetoric before: .clone() can be expensive, that is its intention, so it should be obvious that always cloning is not a useful proposal — that would be equivalent to saying that the difference between Copy and Clone is not relevant and should thus be removed.

3 Likes

While it may feel like an optimisation in the narrow case you care about, it is not an optimisation as presented in the spec change: there is a huge semantic difference between cloning and moving! It can be crucial for correctness (as in object identity) as well as resource usage (memory, CPU) to know whether the “optional” method is called.

In general: Rust is successful as a systems language because it does not employ magic in the Perl “do what I mean” sense (it certainly employs magic in figuring out whether what you’re doing is safe, granted). Let’s please leave it like that.

3 Likes

Relevant link:

at the "Conclusion" section they suggest an Autoclone trait, for types which should get Copy-like ("linear" semantics) even though they are not Copy - i.e., get clone-d whenever they are moved. This is meant for cheap-to-clone types, like Arc.
1 Like