From 44e94378d03b30af3351f27ffc08732eba8fe0e6 Mon Sep 17 00:00:00 2001 From: Liam Zdenek Date: Thu, 2 Jun 2016 15:04:54 +0000 Subject: [PATCH 01/12] Adding 'clone to satisfy the borrow checker' anti-pattern --- README.md | 1 - anti_patterns/borrow_clone.md | 56 +++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 anti_patterns/borrow_clone.md diff --git a/README.md b/README.md index c35e3670..5dfc2912 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,6 @@ language that you can read [here](https://rust-unofficial.github.io/patterns/). ### Anti-patterns * TODO thread + catch_panic for exceptions -* TODO Clone to satisfy the borrow checker * TODO Matching all fields of a struct (back compat) * TODO wildcard matches * TODO taking an enum rather than having multiple functions diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md new file mode 100644 index 00000000..51fb0a7b --- /dev/null +++ b/anti_patterns/borrow_clone.md @@ -0,0 +1,56 @@ +# Clone to satisfy the borrow checker + +## Description + +The borrow checker prevents rust users from developing otherwise unsafe code by +ensuring that either: only one mutable reference exists, or potentially many but +all immutable references exist. If the code written does not hold true to these +conditions, this anti-pattern arises when the developer resolves the compiler +error by cloning the variable. + + +## Example + +```rust fn main() { + // define any variable + let mut x = 5; + + // Borrow `x` -- but clone it first + let y = &mut (x.clone()); + + // perform some action on the borrow to prevent rust from optimizing this + //out of existence + *y += 1; + + // without the x.clone() two lines prior, this line would fail on compile as + // x has been borrowed + // thanks to x.clone(), x was never borrowed, and this line will run. + println!("{}", x); +} +``` + + +## Motivation + +It is tempting, particularly for beginners, to use this pattern to resolve +confusing issues with the borrow checker. However, there are serious +consequences. Using .clone() causes a copy of the data to be made. Any changes +between the two are not synchronized -- as if two completely separate variables +exist. + +There are special cases -- `Rc` is designed to handle clones intelligently. +It internally manages exactly one copy of the data, and cloning it will only +clone the reference. + +In general, clones should be deliberate, with full understanding of the +consequences. If a clone is used to make a borrow checker error disappear, +that's a good indication this anti-pattern may be in use. + +If an unnecessary clone is suspected, The Rust Book's chapter on Ownership +should be understood fully before assessing whether the clone is required or not. + + +## See also + +[The Rust Book: Ownership, which describes how the borrow checker behaves](https://doc.rust-lang.org/book/ownership.html) +[Rc documentation, which handles .clone() intelligently](http://doc.rust-lang.org/std/rc/) From 7b8f56dd4b47c42dc8c9ec93af77f5b3c5a38f81 Mon Sep 17 00:00:00 2001 From: Liam Zdenek Date: Mon, 25 Jul 2016 16:16:06 +0000 Subject: [PATCH 02/12] Small changes per feedback on the PR --- anti_patterns/borrow_clone.md | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md index 51fb0a7b..8736c66e 100644 --- a/anti_patterns/borrow_clone.md +++ b/anti_patterns/borrow_clone.md @@ -2,7 +2,7 @@ ## Description -The borrow checker prevents rust users from developing otherwise unsafe code by +The borrow checker prevents Rust users from developing otherwise unsafe code by ensuring that either: only one mutable reference exists, or potentially many but all immutable references exist. If the code written does not hold true to these conditions, this anti-pattern arises when the developer resolves the compiler @@ -11,22 +11,21 @@ error by cloning the variable. ## Example -```rust fn main() { - // define any variable - let mut x = 5; - - // Borrow `x` -- but clone it first - let y = &mut (x.clone()); - - // perform some action on the borrow to prevent rust from optimizing this - //out of existence - *y += 1; - - // without the x.clone() two lines prior, this line would fail on compile as - // x has been borrowed - // thanks to x.clone(), x was never borrowed, and this line will run. - println!("{}", x); -} +```rust +// define any variable +let mut x = 5; + +// Borrow `x` -- but clone it first +let y = &mut (x.clone()); + +// perform some action on the borrow to prevent rust from optimizing this +//out of existence +*y += 1; + +// without the x.clone() two lines prior, this line would fail on compile as +// x has been borrowed +// thanks to x.clone(), x was never borrowed, and this line will run. +println!("{}", x); ``` From 39c95553a66a06f67a1757645a3c96a99ab60557 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 1 Jan 2021 13:54:38 +0100 Subject: [PATCH 03/12] Add borrow_clone.md to SUMMARY.md --- SUMMARY.md | 1 + 1 file changed, 1 insertion(+) diff --git a/SUMMARY.md b/SUMMARY.md index 6b78d2be..967b4530 100644 --- a/SUMMARY.md +++ b/SUMMARY.md @@ -36,6 +36,7 @@ - [Visitor](./patterns/visitor.md) - [Anti-patterns](./anti_patterns/index.md) + - [Clone to satisfy the borrow checker](./anti_patterns/borrow_clone.md) - [`#[deny(warnings)]`](./anti_patterns/deny-warnings.md) - [Deref Polymorphism](./anti_patterns/deref.md) From eaa9e0a6daa9ec202db2f11345c3dbec0a0c39ab Mon Sep 17 00:00:00 2001 From: Simon Date: Wed, 6 Jan 2021 10:59:35 +0100 Subject: [PATCH 04/12] markdownlint --- anti_patterns/borrow_clone.md | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md index 8736c66e..3bc6af83 100644 --- a/anti_patterns/borrow_clone.md +++ b/anti_patterns/borrow_clone.md @@ -4,11 +4,10 @@ The borrow checker prevents Rust users from developing otherwise unsafe code by ensuring that either: only one mutable reference exists, or potentially many but -all immutable references exist. If the code written does not hold true to these +all immutable references exist. If the code written does not hold true to these conditions, this anti-pattern arises when the developer resolves the compiler error by cloning the variable. - ## Example ```rust @@ -28,7 +27,6 @@ let y = &mut (x.clone()); println!("{}", x); ``` - ## Motivation It is tempting, particularly for beginners, to use this pattern to resolve @@ -48,8 +46,7 @@ that's a good indication this anti-pattern may be in use. If an unnecessary clone is suspected, The Rust Book's chapter on Ownership should be understood fully before assessing whether the clone is required or not. - ## See also [The Rust Book: Ownership, which describes how the borrow checker behaves](https://doc.rust-lang.org/book/ownership.html) -[Rc documentation, which handles .clone() intelligently](http://doc.rust-lang.org/std/rc/) +[`Rc` documentation, which handles .clone() intelligently](http://doc.rust-lang.org/std/rc/) From e2d348e376d4d11775173b290276c3dd055e71a7 Mon Sep 17 00:00:00 2001 From: Simon Date: Wed, 6 Jan 2021 11:08:53 +0100 Subject: [PATCH 05/12] remove trailing whitespace --- anti_patterns/borrow_clone.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md index 3bc6af83..4d332513 100644 --- a/anti_patterns/borrow_clone.md +++ b/anti_patterns/borrow_clone.md @@ -15,7 +15,7 @@ error by cloning the variable. let mut x = 5; // Borrow `x` -- but clone it first -let y = &mut (x.clone()); +let y = &mut (x.clone()); // perform some action on the borrow to prevent rust from optimizing this //out of existence From f7071fdeb32277d7857470ff3f5855b90ef8c692 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Thu, 21 Jan 2021 00:39:31 +0100 Subject: [PATCH 06/12] Update anti_patterns/borrow_clone.md Co-authored-by: Marco Ieni <11428655+MarcoIeni@users.noreply.github.com> --- anti_patterns/borrow_clone.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md index 4d332513..df655847 100644 --- a/anti_patterns/borrow_clone.md +++ b/anti_patterns/borrow_clone.md @@ -31,7 +31,7 @@ println!("{}", x); It is tempting, particularly for beginners, to use this pattern to resolve confusing issues with the borrow checker. However, there are serious -consequences. Using .clone() causes a copy of the data to be made. Any changes +consequences. Using `.clone()` causes a copy of the data to be made. Any changes between the two are not synchronized -- as if two completely separate variables exist. From 7838d3d25725583587e3764afd4fbb84046784b7 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Thu, 21 Jan 2021 00:40:05 +0100 Subject: [PATCH 07/12] Update anti_patterns/borrow_clone.md Co-authored-by: Marco Ieni <11428655+MarcoIeni@users.noreply.github.com> --- anti_patterns/borrow_clone.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md index df655847..26f8641e 100644 --- a/anti_patterns/borrow_clone.md +++ b/anti_patterns/borrow_clone.md @@ -46,6 +46,8 @@ that's a good indication this anti-pattern may be in use. If an unnecessary clone is suspected, The Rust Book's chapter on Ownership should be understood fully before assessing whether the clone is required or not. +Also be sure to always run `cargo clippy` in your project, which will detect some cases in which `.clone()` is not necessary, like [1](https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone), [2](https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy), [3](https://rust-lang.github.io/rust-clippy/master/index.html#map_clone) or [4](https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref). + ## See also [The Rust Book: Ownership, which describes how the borrow checker behaves](https://doc.rust-lang.org/book/ownership.html) From 0264c2d8f3a6c12c627f98955e8caf8c0574a61a Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 29 Mar 2021 20:37:24 +0200 Subject: [PATCH 08/12] Adding Arc to special cases --- anti_patterns/borrow_clone.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md index 26f8641e..64e2bc88 100644 --- a/anti_patterns/borrow_clone.md +++ b/anti_patterns/borrow_clone.md @@ -39,6 +39,11 @@ There are special cases -- `Rc` is designed to handle clones intelligently. It internally manages exactly one copy of the data, and cloning it will only clone the reference. +There is also `Arc` which provides shared ownership of a value of type T +that is allocated in the heap. Invoking `.clone()` on `Arc` produces a new `Arc` +instance, which points to the same allocation on the heap as the source `Arc`, +while increasing a reference count. + In general, clones should be deliberate, with full understanding of the consequences. If a clone is used to make a borrow checker error disappear, that's a good indication this anti-pattern may be in use. @@ -46,7 +51,10 @@ that's a good indication this anti-pattern may be in use. If an unnecessary clone is suspected, The Rust Book's chapter on Ownership should be understood fully before assessing whether the clone is required or not. -Also be sure to always run `cargo clippy` in your project, which will detect some cases in which `.clone()` is not necessary, like [1](https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone), [2](https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy), [3](https://rust-lang.github.io/rust-clippy/master/index.html#map_clone) or [4](https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref). +Also be sure to always run `cargo clippy` in your project, which will detect some +cases in which `.clone()` is not necessary, like [1](https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone), +[2](https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy), +[3](https://rust-lang.github.io/rust-clippy/master/index.html#map_clone) or [4](https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref). ## See also From 70e744f7aeb33232f1908627cfb8168c2733b9cb Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 29 Mar 2021 20:42:18 +0200 Subject: [PATCH 09/12] Adding review comments --- anti_patterns/borrow_clone.md | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md index 64e2bc88..a35a4e1c 100644 --- a/anti_patterns/borrow_clone.md +++ b/anti_patterns/borrow_clone.md @@ -48,7 +48,16 @@ In general, clones should be deliberate, with full understanding of the consequences. If a clone is used to make a borrow checker error disappear, that's a good indication this anti-pattern may be in use. -If an unnecessary clone is suspected, The Rust Book's chapter on Ownership +Even though `.clone()` is an indication of a bad pattern, sometimes +**it is fine to write inefficient code**, in cases such as when: + +- the developer is still new to ownership +- the code doesn't have great speed or memory constraints + (like hackathon projects or prototypes) +- satisfying the borrow checker is really complicated and you prefer to + optimize readability over performance + +If an unnecessary clone is suspected, The [Rust Book's chapter on Ownership](https://doc.rust-lang.org/book/ownership.html) should be understood fully before assessing whether the clone is required or not. Also be sure to always run `cargo clippy` in your project, which will detect some @@ -58,5 +67,5 @@ cases in which `.clone()` is not necessary, like [1](https://rust-lang.github.io ## See also -[The Rust Book: Ownership, which describes how the borrow checker behaves](https://doc.rust-lang.org/book/ownership.html) -[`Rc` documentation, which handles .clone() intelligently](http://doc.rust-lang.org/std/rc/) +- [`Rc` documentation, which handles .clone() intelligently](http://doc.rust-lang.org/std/rc/) +- [`Arc` documentation, a thread-safe reference-counting pointer](https://doc.rust-lang.org/std/sync/struct.Arc.html/) From 56ed417b76f4ccb27301848d29d743baae2fc8f4 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 29 Mar 2021 20:44:23 +0200 Subject: [PATCH 10/12] Adding ownership tricks link --- anti_patterns/borrow_clone.md | 1 + 1 file changed, 1 insertion(+) diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md index a35a4e1c..b6f5fc84 100644 --- a/anti_patterns/borrow_clone.md +++ b/anti_patterns/borrow_clone.md @@ -69,3 +69,4 @@ cases in which `.clone()` is not necessary, like [1](https://rust-lang.github.io - [`Rc` documentation, which handles .clone() intelligently](http://doc.rust-lang.org/std/rc/) - [`Arc` documentation, a thread-safe reference-counting pointer](https://doc.rust-lang.org/std/sync/struct.Arc.html/) +- [Tricks with ownership in Rust](https://web.archive.org/web/20210120233744/https://xion.io/post/code/rust-borrowchk-tricks.html) From fc43e19c10dedf15d07b6df310ad369fcf335c67 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 29 Mar 2021 20:50:33 +0200 Subject: [PATCH 11/12] Adding crosslinks between mem::take/replace and borrow_clone --- anti_patterns/borrow_clone.md | 1 + idioms/mem-replace.md | 6 ++---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md index b6f5fc84..91c06c7b 100644 --- a/anti_patterns/borrow_clone.md +++ b/anti_patterns/borrow_clone.md @@ -67,6 +67,7 @@ cases in which `.clone()` is not necessary, like [1](https://rust-lang.github.io ## See also +- [`mem::{take(_), replace(_)}` to keep owned values in changed enums](../idioms/mem-replace.md) - [`Rc` documentation, which handles .clone() intelligently](http://doc.rust-lang.org/std/rc/) - [`Arc` documentation, a thread-safe reference-counting pointer](https://doc.rust-lang.org/std/sync/struct.Arc.html/) - [Tricks with ownership in Rust](https://web.archive.org/web/20210120233744/https://xion.io/post/code/rust-borrowchk-tricks.html) diff --git a/idioms/mem-replace.md b/idioms/mem-replace.md index 49d6891b..3649b6d4 100644 --- a/idioms/mem-replace.md +++ b/idioms/mem-replace.md @@ -116,7 +116,5 @@ like Indiana Jones, replacing the artifact with a bag of sand. ## See also -This gets rid of the [Clone to satisfy the borrow checker] antipattern in a -specific case. - -[Clone to satisfy the borrow checker](TODO: Hinges on PR #23) +This gets rid of the [Clone to satisfy the borrow checker](../anti_patterns/borrow_clone.md) +antipattern in a specific case. From e16d2083ba484ecb6f26cca3868e6f53967b7c91 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 29 Mar 2021 20:54:54 +0200 Subject: [PATCH 12/12] Fix typo in link --- anti_patterns/borrow_clone.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/anti_patterns/borrow_clone.md b/anti_patterns/borrow_clone.md index 91c06c7b..4ee96789 100644 --- a/anti_patterns/borrow_clone.md +++ b/anti_patterns/borrow_clone.md @@ -69,5 +69,5 @@ cases in which `.clone()` is not necessary, like [1](https://rust-lang.github.io - [`mem::{take(_), replace(_)}` to keep owned values in changed enums](../idioms/mem-replace.md) - [`Rc` documentation, which handles .clone() intelligently](http://doc.rust-lang.org/std/rc/) -- [`Arc` documentation, a thread-safe reference-counting pointer](https://doc.rust-lang.org/std/sync/struct.Arc.html/) +- [`Arc` documentation, a thread-safe reference-counting pointer](https://doc.rust-lang.org/std/sync/struct.Arc.html) - [Tricks with ownership in Rust](https://web.archive.org/web/20210120233744/https://xion.io/post/code/rust-borrowchk-tricks.html)