#[derive(Clone, Copy, Debug)]
enum Fruit {
Apple,
Banana,
}
impl Fruit {
fn mutate(&mut self) {
match *self {
Self::Apple => {
*self = Self::Banana;
}
_ => {}
}
}
}
struct Container {
fruit: Fruit,
}
impl Container {
pub fn fruit(&self) -> Fruit {
self.fruit
}
}
fn main() {
let mut c = Container {
fruit: Fruit::Apple,
};
c.fruit().mutate();
eprintln!("container's fruit is {:?}", c.fruit); // no mutation
c.fruit.mutate();
eprintln!("container's fruit is {:?}", c.fruit); // mutation
}
The bug, I think, is that because I'd called a getter that returns an enum marked Copy, I created a temporary Fruit, mutated it, and then let it fall out of scope, all while self.fruit remained untouched.
Several things went wrong for this bug to happen. First, if I really needed to call the getter, then I would have had to call c.fruit_mut(). The compiler would have caught that for me, except that (second) I'd derived Copy on this enum, and the rust-analyzer-generated getter took advantage of that.
I now understand what happened, but what can I learn from this? Should I not have derived Copy? I have memorized the "Generally speaking, if your type can implement Copy, it should" guideline, but in this case, it seems to have become a footgun. I know I could change the fruit() getter to return &Fruit, but I'm not sure that's the right answer, either.
In my opinion, if there also exists a fruit_mut function returning &mut Fruit, that would be a possibly reasonable option.
The other approach would be to avoid any &mut self methods on a Copy type. You could create a fn(self) -> Self function instead. That's what the standard library largely does, as far as I'm aware, consistently e. g. for types like i32 or bool, which will have no &mut self methods besides trait implementations, and most of these are from operators such as += which come with their own mechanism of requiring a place-expression as LHS argument to avoid this very problem from occurring.
One thing to point out for your code is that the Copy-ness of the enum was only used in the implementation of the getter. The problematic call-site doesn't even rely on the type being Copy.
By the way, there is some precedent in the standard library for not implementing Copy on certain types whose main API prominently features &mut self methods: Iterator types like Range in std::ops - Rust or Iter in std::slice - Rust don't implement Copy, as Iterator::next is a &mut self method, and it's just too easy to become confused by calling that on a copy of the iterator, accidentally, without advancing the original iterator, which would result in very similar confusing results.
Another possible option is to just not have the getter.
The main time getters are idiomatic in Rust is when you want to keep the field private to maintain an invariant or maximal flexibility, but need to allow other modules/crates to access the field. If this is true for you, getting rid of the getter probably isn't an option.
Even when you have a getter, it's more idiomatic to just access the field when possible, as that is more flexible with regards to the borrow checker. I thought perhaps there would be a lint for this, but I didn't find one.
For what it's worth, I tend to lean in this direction...
If the type might become non-Copy in the future, it could be prudent to omit the Copy implementation now, to avoid a breaking API change.
Start with enums non-Copy then add it if not having it becomes a burden or having it is obviously the correct choice.
A good example are state machine states (which I think is your example). Non-Copy has served me well. In the rare circumstances I've need to copy the value .clone() is not too much typing to be a burden and makes the intent crystal clear.
I'd say Copy is great. If you have a simple enum without data you should make it Copy, because it makes using it much easier, and possibly even a bit more efficient (since a copy of the value is smaller than a pointer).
The footgun here is more from having a in-place-modifying &mut self method on a Copy type. The mutate() could have been #[must_use] fn into_banana(self) -> Self.
Hmm, is there anything specifically because of the enum-ness that causes this? Wouldn't calling a &mut method on a non-enum (like a u32) have exactly the same behaviour?
I guess the Copy here is a red herring. Or at least it's surely not the only one responsible.
Sometimes, when you have a chain of errors leading to a bug, you can't clearly point to one of them and say "now that is the problem!".
I guess the nicest example of this phenomenon is shared mutability. Programmers have been arguing for decades whether it is sharing xor mutability that causes memory safety bugs:
"It's threads!" – shouted JavaScript and Python, and JS remained single-threaded, and Python introduced the GIL.
"It's mutability!" – screamed Haskell and Erlang, and they made (almost) everything immutable.
And then along came Rust, and said: "you are fools! You can have both sharing and mutability in the same language, as long as you isolate them from each other."
Similarly, I'd say a Copy enum is not a problem in itself. If it's a trivial enum, make it Copy. The root cause is calling a mutating method on a temporary, which is partially caused by the getter returning an owned value. (Returning owned values is not a problem in itself, either.)
If I write the equivalent code on the Playground, Clippy doesn't flag it, and I couldn't find a related lint. I'm not sure what the right solution here is, apart from simply not making a getter.
#[must_use] has a problem: “…wait, shouldn't I logically put this on almost every function? How much is too much?”. An analogous attribute for temporary-mutation would be the same way. The things which shouldn't get a warning are cases like std::cell::RefMut::deref_mut (used to mutate the underlying value that outlasts the RefMut) or std::process::Command::spawn (accepts &mut self but the builder is left semantically unchanged and may or may not be used); the majority of &mut self methods are not meaningful to use on a temporary.
Hm, considering those examples, that suggests a pattern: just warn on any call of a function with signature fn(&mut self, ...) -> () on a temporary. All of the examples above are cases where the function returns a non-() value. (It still might be a false positive on things which have a side effect outside of self, like a channel send() function does.)
Types whose values can be duplicated simply by copying bits.
I would interpret this to mean that if I copy a Copy type, the bits of the duplicate must remain the same at all times, so &mut self on a Copy type should be illegal.
I meant &mut that is not &mut self. But, yeah, the point is that there should be no difference, so that's not a useful distinction.
Another thought: if I'm obtaining &mut T, that would typically be so that I can change it and others see the change, right? Because if I don't want to change it, then &T is suffcient, and if I want to change it but no one will see the change, then I can just take ownership.
In that case, could we consider it wrong by the compiler to crate a temporary to enable a &mut T?
Well, &mut self can be implicit, that's probably why people recommend against it. Non-self &mut is explicit, so it's harder to get it wrong and confuse it with a copy.
I don't understand what you mean by this. Can you elaborate?
