From a1176466456e15e7e34426f64f30f9e60a3110cc Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Fri, 17 Mar 2017 23:34:11 +0100 Subject: [PATCH 1/9] initial-commit --- text/0000-rc-newref-clone.md | 62 ++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 text/0000-rc-newref-clone.md diff --git a/text/0000-rc-newref-clone.md b/text/0000-rc-newref-clone.md new file mode 100644 index 00000000000..1595c2a8d3a --- /dev/null +++ b/text/0000-rc-newref-clone.md @@ -0,0 +1,62 @@ +- Feature Name: refcount_clone_new_ref +- Start Date: 2017-03-17 +- RFC PR: +- Rust Issue: + +# Summary +[summary]: #summary + +This RFC proposes the simple addition of a new_ref method to reference-counted pointer types (Rc and Arc) in the standard library synonymous to clone, with a more explicit meaning. + +# Motivation +[motivation]: #motivation + +Clone is one of the most loosely defined concept in rust. Clone implementations, as the standard libray puts it "may or may not be expensive". The meaning of Clone can be shallow or deep clones alike. + +It is therefore required, when reading code that calls clone to know the exact type of the calling object, because the cloning a ```Vec``` and cloning an ```Arc>``` have very different semantic and performance implications. + +This leads to recurrent misunderstandings when reading or reviewing code [1]. +In many cases the code could be a lot easier to understand at a glance if reference counted pointers had a method with a more explicit name to create new reference. + +This issue is not specific to clone. The author of this RFC believes that it is generally best to avoid using a high level and vaguely defined concept as the only way to name an operation. ```Clone::clone``` is a prime example because it is at the same time one of the most vague concept, can imply either a very cheap or a very costly operation, and can go from one to the other with simple changes that don't necessarily affect the overal semantic of the progam. + +# Detailed design +[design]: #detailed-design + +A method ```fn new_ref(&self) -> Self``` is added to ```Rc``` and ```Arc```. +The ```Clone::clone``` implementations for ```Rc``` and ```Arc``` are ust calls to the respective new_ref methods. + + + +This is the bulk of the RFC. Explain the design in enough detail for somebody familiar +with the language to understand, and for somebody familiar with the compiler to implement. +This should get into specifics and corner-cases, and include examples of how the feature is used. + +# How We Teach This +[how-we-teach-this]: #how-we-teach-this + +Note that this RFC proposes a more descriptive way to do express an operation, but does not prevent from continuing to use the more generic way (clone still works as expected and remains the abstract way. + +It would make sense for ```new_ref``` to be the preferred way to create new reference-counted pointers out of existing one, over the still existing ```clone``` method, since it is more descriptive. + +This is not a big change in the standard library, however, common code examples in the official documentation and tutorials should be changed if ```new_ref``` becomes the preferred way over ```clone```. + +# Drawbacks +[drawbacks]: #drawbacks + +Very short names are quite popular in the rust community for common operations. This RFC proposes a slightly longer name than the existing one. + +# Alternatives +[alternatives]: #alternatives + +Finer grained traits could probably be added to express shallow and deep copies, another trait could also be added to express adding a reference to a reference-counted object. It could also be added later + +The impact of not accepting this proposal would be that a paper-cut of the language remains. + +# Unresolved questions +[unresolved]: #unresolved-questions + +Is there a better name than ```new_ref```? + +# Footnotes +[1] TODO: WebRender example. From 10411622b48c033632394e238fd2f5aa9ae71f86 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Sat, 18 Mar 2017 00:11:17 +0100 Subject: [PATCH 2/9] footnote --- text/0000-rc-newref-clone.md | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/text/0000-rc-newref-clone.md b/text/0000-rc-newref-clone.md index 1595c2a8d3a..ad72d2caf78 100644 --- a/text/0000-rc-newref-clone.md +++ b/text/0000-rc-newref-clone.md @@ -11,7 +11,7 @@ This RFC proposes the simple addition of a new_ref method to reference-counted p # Motivation [motivation]: #motivation -Clone is one of the most loosely defined concept in rust. Clone implementations, as the standard libray puts it "may or may not be expensive". The meaning of Clone can be shallow or deep clones alike. +Clone is one of the most loosely defined concept in rust. Clone implementations can be shallow or deep clones which, as the [standard libray documentation](https://doc.rust-lang.org/std/clone/trait.Clone.html) puts it "may or may not be expensive". It is therefore required, when reading code that calls clone to know the exact type of the calling object, because the cloning a ```Vec``` and cloning an ```Arc>``` have very different semantic and performance implications. @@ -23,14 +23,11 @@ This issue is not specific to clone. The author of this RFC believes that it is # Detailed design [design]: #detailed-design -A method ```fn new_ref(&self) -> Self``` is added to ```Rc``` and ```Arc```. -The ```Clone::clone``` implementations for ```Rc``` and ```Arc``` are ust calls to the respective new_ref methods. - +This RFC does not involve any compiler change (only a minor addition to alloc/rc.rs and alloc/arc.rs. +A method ```fn new_ref(&self) -> Self``` is added to ```Rc``` and ```Arc```. +The ```Clone::clone``` implementations for ```Rc``` and ```Arc``` simply call their respective ```new_ref``` methods. -This is the bulk of the RFC. Explain the design in enough detail for somebody familiar -with the language to understand, and for somebody familiar with the compiler to implement. -This should get into specifics and corner-cases, and include examples of how the feature is used. # How We Teach This [how-we-teach-this]: #how-we-teach-this @@ -58,5 +55,7 @@ The impact of not accepting this proposal would be that a paper-cut of the langu Is there a better name than ```new_ref```? -# Footnotes -[1] TODO: WebRender example. +# Notes +[1] Here is an example of code where the similarity between ```Vec::clone()``` and ```Arc>::clone()``` is a recurrent source of confusion: + +[WebRender](https://github.com/servo/webrender)'s [resource_cache.rs](https://github.com/servo/webrender/blob/e1ba6ff8146a0ba7a33bb9af6390b34f6b313b78/webrender/src/resource_cache.rs#L381) now stores CPU image data as an ```Arc>```. These were originally simple ```Vec``` which were cloned each time they were be sent to a separate thread in charge of communicating with the GPU. Images being potentially very large, these clones were quite expensive and we decided to use Arcs to avoid the copy. Since cloning an entire vector and creating a reference-counted reference are both exposed through ```Clone::clone()```, the places where the expensive clones happened read exactly the same now that they only increment an atomic reference count. Long after this change reading ```image.data.clone()``` still rings an alarm when reading or reviewing this code because it _reads_ like an expensive operation (copy the entire image) even though it is a simple atomic increment. People still occasionally talk about fixing these already-fixed copies and reviewers themselves get it wrong. If it was possible to write ```image.data.new_ref()```, this simple code would be a lot less misleading. From 2d13f27d781e51a9183d11a607e78f752f53fdfb Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Sat, 18 Mar 2017 16:17:11 +0100 Subject: [PATCH 3/9] Update 0000-rc-newref-clone.md --- text/0000-rc-newref-clone.md | 39 ++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/text/0000-rc-newref-clone.md b/text/0000-rc-newref-clone.md index ad72d2caf78..4d3dd7ca5d7 100644 --- a/text/0000-rc-newref-clone.md +++ b/text/0000-rc-newref-clone.md @@ -15,7 +15,7 @@ Clone is one of the most loosely defined concept in rust. Clone implementations It is therefore required, when reading code that calls clone to know the exact type of the calling object, because the cloning a ```Vec``` and cloning an ```Arc>``` have very different semantic and performance implications. -This leads to recurrent misunderstandings when reading or reviewing code [1]. +This leads to recurrent misunderstandings when reading or reviewing code (See the example section at the end of this document). In many cases the code could be a lot easier to understand at a glance if reference counted pointers had a method with a more explicit name to create new reference. This issue is not specific to clone. The author of this RFC believes that it is generally best to avoid using a high level and vaguely defined concept as the only way to name an operation. ```Clone::clone``` is a prime example because it is at the same time one of the most vague concept, can imply either a very cheap or a very costly operation, and can go from one to the other with simple changes that don't necessarily affect the overal semantic of the progam. @@ -23,16 +23,26 @@ This issue is not specific to clone. The author of this RFC believes that it is # Detailed design [design]: #detailed-design -This RFC does not involve any compiler change (only a minor addition to alloc/rc.rs and alloc/arc.rs. +This RFC does not involve any compiler change (only minor additions to alloc/rc.rs and alloc/arc.rs). -A method ```fn new_ref(&self) -> Self``` is added to ```Rc``` and ```Arc```. -The ```Clone::clone``` implementations for ```Rc``` and ```Arc``` simply call their respective ```new_ref``` methods. +The following steps apply to ```Rc```, ```Arc```, ```rc::Weal```, ```arc::Weak```. +A method ```fn new_ref(&self) -> Self``` is added to the pointer type, into which the code of the ```Clone::clone``` implementation is moved. +The ```Clone::clone``` implementations for the pointer type simply becomes a ```new_ref``` call. + +The proposed change is simple enough that it may be even simpler to see in code directly than in english. +It is therefore implemented on the author's [new_ref branch](https://github.com/nical/rust/tree/new_ref): + - [Addition of Arc::new_ref](https://github.com/nical/rust/commit/392e105b0dd3ffb44beb8cbf853f75493a5167b5). + - [Addition of Rc::new_ref](https://github.com/nical/rust/commit/5903ed4aa3ddb825f8b9b3412b3240f07193b711). + - [Addition of arc::Weak::new_ref](https://github.com/nical/rust/commit/6f72fe1e208d96917c806bb4895b7014c1bfe164). + - [Addition of rc::Weak::new_ref](https://github.com/nical/rust/commit/ddbd2b5e7e42d6be11194abef4f5d12ec11aa41e). + +Note that these commits are missing the proper stable/unstable annotations. # How We Teach This [how-we-teach-this]: #how-we-teach-this -Note that this RFC proposes a more descriptive way to do express an operation, but does not prevent from continuing to use the more generic way (clone still works as expected and remains the abstract way. +Note that this RFC proposes a more descriptive way to do express an operation, but does not prevent from continuing to use the more generic way (clone still works as expected and remains the generic way to . It would make sense for ```new_ref``` to be the preferred way to create new reference-counted pointers out of existing one, over the still existing ```clone``` method, since it is more descriptive. @@ -46,16 +56,23 @@ Very short names are quite popular in the rust community for common operations. # Alternatives [alternatives]: #alternatives -Finer grained traits could probably be added to express shallow and deep copies, another trait could also be added to express adding a reference to a reference-counted object. It could also be added later +This RFC tries to remain as straightforard and simple as possible. The tention between deep and shallow clones could maybe also be avoided through the addition of finer grained ShallowClone and DeepClone traits. It is not clear tothe author, however, if the added genericity would be useful in practice. -The impact of not accepting this proposal would be that a paper-cut of the language remains. +The impact of not accepting this proposal would be that a "paper-cut" annoyance in the standard library remains. # Unresolved questions [unresolved]: #unresolved-questions -Is there a better name than ```new_ref```? +Is there a better name than ```new_ref```? The documentation uses the term _pointer_ in many places, although _reference_-counting is also present. ```new_ptr``` would work as well, although _ptr_ seems to be most used in referrence to raw pointers. +Longer names such as ```new_reference``` or ```new_pointer``` are just as informative, although the extra length goes against the general convention of using short names for common constructs. +Othe names such as ```new_rc```, ```new_arc```, etc. could be considered. + +# Generalization + +The author believes that as in general, types (structures and enums) should provide methods using adequately descriptive names and, _in addition_, implement (usually more abstract) traits using these methods. This way, no compromise is made on the names of the functionality exposed by the type. -# Notes -[1] Here is an example of code where the similarity between ```Vec::clone()``` and ```Arc>::clone()``` is a recurrent source of confusion: +# Example +Here is an example of code where the similarity between ```Vec::clone()``` and ```Arc>::clone()``` is a recurrent source of confusion: -[WebRender](https://github.com/servo/webrender)'s [resource_cache.rs](https://github.com/servo/webrender/blob/e1ba6ff8146a0ba7a33bb9af6390b34f6b313b78/webrender/src/resource_cache.rs#L381) now stores CPU image data as an ```Arc>```. These were originally simple ```Vec``` which were cloned each time they were be sent to a separate thread in charge of communicating with the GPU. Images being potentially very large, these clones were quite expensive and we decided to use Arcs to avoid the copy. Since cloning an entire vector and creating a reference-counted reference are both exposed through ```Clone::clone()```, the places where the expensive clones happened read exactly the same now that they only increment an atomic reference count. Long after this change reading ```image.data.clone()``` still rings an alarm when reading or reviewing this code because it _reads_ like an expensive operation (copy the entire image) even though it is a simple atomic increment. People still occasionally talk about fixing these already-fixed copies and reviewers themselves get it wrong. If it was possible to write ```image.data.new_ref()```, this simple code would be a lot less misleading. +[WebRender](https://github.com/servo/webrender)'s [resource_cache.rs](https://github.com/servo/webrender/blob/e1ba6ff8146a0ba7a33bb9af6390b34f6b313b78/webrender/src/resource_cache.rs#L381) now stores CPU image data as an ```Arc>```. These were originally simple ```Vec```s which were cloned each time they were be sent to a separate thread in charge of communicating with the GPU. Images being potentially very large, these clones were quite expensive and we decided to use Arcs to avoid the copy. Since cloning an entire vector and creating a reference-counted reference are both exposed through ```Clone::clone()``` (and _only_ through clone), the places where the expensive clones happened read exactly the same now that they only increment an atomic reference count. Long after this change, reading ```image.data.clone()``` still rings an alarm when going through or reviewing this code because it _reads_ like an expensive operation (copy the entire image) even though it is a simple atomic increment. People still occasionally talk about fixing these already-fixed clones and reviewers themselves have gotten it wrong at times. If it was possible to write ```image.data.new_ref()```, this simple code would be a lot less misleading. +This example is simple and came up recently, but isn't specific to the type of problem WebRender is solving. There are many other places where clone could be replaced by a more descriptive wording for improved clarity. From 14f29266574cbee38fd7f0c6382f3bf71de1824c Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Sat, 18 Mar 2017 16:29:16 +0100 Subject: [PATCH 4/9] Update 0000-rc-newref-clone.md --- text/0000-rc-newref-clone.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/text/0000-rc-newref-clone.md b/text/0000-rc-newref-clone.md index 4d3dd7ca5d7..012997956c2 100644 --- a/text/0000-rc-newref-clone.md +++ b/text/0000-rc-newref-clone.md @@ -6,12 +6,12 @@ # Summary [summary]: #summary -This RFC proposes the simple addition of a new_ref method to reference-counted pointer types (Rc and Arc) in the standard library synonymous to clone, with a more explicit meaning. +This RFC proposes the simple addition of a new_ref method to reference-counted pointer types (Rc, Arc and repsective weak pointer types) in the standard library synonymous to clone, with a more explicit terminology. # Motivation [motivation]: #motivation -Clone is one of the most loosely defined concept in rust. Clone implementations can be shallow or deep clones which, as the [standard libray documentation](https://doc.rust-lang.org/std/clone/trait.Clone.html) puts it "may or may not be expensive". +Clone is one of the most vaguely defined concept in rust. Clone implementations can be shallow or deep clones which, as the [standard libray documentation](https://doc.rust-lang.org/std/clone/trait.Clone.html) puts it "may or may not be expensive". It is therefore required, when reading code that calls clone to know the exact type of the calling object, because the cloning a ```Vec``` and cloning an ```Arc>``` have very different semantic and performance implications. @@ -67,9 +67,11 @@ Is there a better name than ```new_ref```? The documentation uses the term _poin Longer names such as ```new_reference``` or ```new_pointer``` are just as informative, although the extra length goes against the general convention of using short names for common constructs. Othe names such as ```new_rc```, ```new_arc```, etc. could be considered. -# Generalization +# Generalization (open discussion) -The author believes that as in general, types (structures and enums) should provide methods using adequately descriptive names and, _in addition_, implement (usually more abstract) traits using these methods. This way, no compromise is made on the names of the functionality exposed by the type. +The vagueness of Clone is not criticised, here. Clone, as defined and with its abstraction level, serves its purpose well as a trait for generic operations. The problem comes from _more-specific_ operations and types, which would benefit from being expressed with descriptive terminology, being _only_ usable through high level or abstract terms. + +The author of this RFC believes that as in general, types (structures and enums) should provide methods using adequately descriptive names and, _in addition_ to these methods, implement the more abstract traits using these methods. This way, no compromise is made on the names of the functionality exposed by the type. This should of course be taken with a grain of salt. It really depends on how far apart the type and a trait are in terms of the genericity of the concept they embody or describe. # Example Here is an example of code where the similarity between ```Vec::clone()``` and ```Arc>::clone()``` is a recurrent source of confusion: From 3228649825b5c7ee367cba78041ad67cf17509eb Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Sat, 18 Mar 2017 16:32:44 +0100 Subject: [PATCH 5/9] Update 0000-rc-newref-clone.md --- text/0000-rc-newref-clone.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-rc-newref-clone.md b/text/0000-rc-newref-clone.md index 012997956c2..0c130d83152 100644 --- a/text/0000-rc-newref-clone.md +++ b/text/0000-rc-newref-clone.md @@ -13,12 +13,12 @@ This RFC proposes the simple addition of a new_ref method to reference-counted p Clone is one of the most vaguely defined concept in rust. Clone implementations can be shallow or deep clones which, as the [standard libray documentation](https://doc.rust-lang.org/std/clone/trait.Clone.html) puts it "may or may not be expensive". -It is therefore required, when reading code that calls clone to know the exact type of the calling object, because the cloning a ```Vec``` and cloning an ```Arc>``` have very different semantic and performance implications. +It is therefore required, when reading code that calls clone to know the exact type of the calling object, because the cloning a ```Vec``` and cloning an ```Arc>``` have very different logic and performance implications, while their semantic is really mostly driven by the type of ```T```. This leads to recurrent misunderstandings when reading or reviewing code (See the example section at the end of this document). In many cases the code could be a lot easier to understand at a glance if reference counted pointers had a method with a more explicit name to create new reference. -This issue is not specific to clone. The author of this RFC believes that it is generally best to avoid using a high level and vaguely defined concept as the only way to name an operation. ```Clone::clone``` is a prime example because it is at the same time one of the most vague concept, can imply either a very cheap or a very costly operation, and can go from one to the other with simple changes that don't necessarily affect the overal semantic of the progam. +This issue is not specific to clone. The author of this RFC believes that it is generally best to avoid using a high level and vaguely defined concept as the only way to name an operation. ```Clone::clone``` is a prime example because it is at the same time an intentionally vague concept, can imply either a very cheap or a very costly operation, and can go from one to the other with simple changes to the code that don't necessarily affect the overal semantic of the progam. This RFC therefore concentrates on the case of Clone and reference counted types in the standard library. # Detailed design [design]: #detailed-design From 6e1371c01fd5f6739fc550fb4c9e15fee2291979 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Sat, 18 Mar 2017 16:35:23 +0100 Subject: [PATCH 6/9] Update 0000-rc-newref-clone.md --- text/0000-rc-newref-clone.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-rc-newref-clone.md b/text/0000-rc-newref-clone.md index 0c130d83152..66461dbda84 100644 --- a/text/0000-rc-newref-clone.md +++ b/text/0000-rc-newref-clone.md @@ -77,4 +77,4 @@ The author of this RFC believes that as in general, types (structures and enums) Here is an example of code where the similarity between ```Vec::clone()``` and ```Arc>::clone()``` is a recurrent source of confusion: [WebRender](https://github.com/servo/webrender)'s [resource_cache.rs](https://github.com/servo/webrender/blob/e1ba6ff8146a0ba7a33bb9af6390b34f6b313b78/webrender/src/resource_cache.rs#L381) now stores CPU image data as an ```Arc>```. These were originally simple ```Vec```s which were cloned each time they were be sent to a separate thread in charge of communicating with the GPU. Images being potentially very large, these clones were quite expensive and we decided to use Arcs to avoid the copy. Since cloning an entire vector and creating a reference-counted reference are both exposed through ```Clone::clone()``` (and _only_ through clone), the places where the expensive clones happened read exactly the same now that they only increment an atomic reference count. Long after this change, reading ```image.data.clone()``` still rings an alarm when going through or reviewing this code because it _reads_ like an expensive operation (copy the entire image) even though it is a simple atomic increment. People still occasionally talk about fixing these already-fixed clones and reviewers themselves have gotten it wrong at times. If it was possible to write ```image.data.new_ref()```, this simple code would be a lot less misleading. -This example is simple and came up recently, but isn't specific to the type of problem WebRender is solving. There are many other places where clone could be replaced by a more descriptive wording for improved clarity. +This is only a simple example which isn't specific to the type of problem WebRender is solving. There are many other pieces of code out there where ```.clone()``` could be replaced by a more descriptive verb for improved clarity. From 8cfd3b14071864c6240dff7257b6d892b9953c79 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Sun, 19 Mar 2017 18:26:48 +0100 Subject: [PATCH 7/9] Address some of the feedback. --- text/0000-rc-newref-clone.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/text/0000-rc-newref-clone.md b/text/0000-rc-newref-clone.md index 66461dbda84..32b9d977247 100644 --- a/text/0000-rc-newref-clone.md +++ b/text/0000-rc-newref-clone.md @@ -25,7 +25,7 @@ This issue is not specific to clone. The author of this RFC believes that it is This RFC does not involve any compiler change (only minor additions to alloc/rc.rs and alloc/arc.rs). -The following steps apply to ```Rc```, ```Arc```, ```rc::Weal```, ```arc::Weak```. +The following steps apply to ```Rc```, ```Arc```, ```rc::Weak```, ```arc::Weak```. A method ```fn new_ref(&self) -> Self``` is added to the pointer type, into which the code of the ```Clone::clone``` implementation is moved. The ```Clone::clone``` implementations for the pointer type simply becomes a ```new_ref``` call. @@ -51,6 +51,8 @@ This is not a big change in the standard library, however, common code examples # Drawbacks [drawbacks]: #drawbacks +Adding methods (like ```new_ref``` to pointer types such as ```Rc``` can create conflicts with the methods of the pointee type T. This can be worked around by defining new_ref as ```fn new_ref(this: &Self) -> Self;``` and calling it as follows: ```let thing_bis = Rc::new_ref(&thing);```. + Very short names are quite popular in the rust community for common operations. This RFC proposes a slightly longer name than the existing one. # Alternatives @@ -58,12 +60,15 @@ Very short names are quite popular in the rust community for common operations. This RFC tries to remain as straightforard and simple as possible. The tention between deep and shallow clones could maybe also be avoided through the addition of finer grained ShallowClone and DeepClone traits. It is not clear tothe author, however, if the added genericity would be useful in practice. +As described in the drawbacks section, ```new_ref``` is a new method on the pointer type which can conflict with the method of the pointee type. Defining the method as ```fn new_ref(this: &Self) -> Self;``` solves the issue. The drawback of using this pattern is that new_ref becomes a lot less simple to call than clone (for example ```let thing_bis = Rc::new_ref(&thing);``` versus ```let thing_bis = thing.clone();```), and the risk is that clone remains the popular way of copying reference counted pointers due to being easier to use. +If a trait (let's call it ```NewRef```) is added for this purpose, a function working on NewRef types could be added so that the call sites could look like ```let thing_bis = new_ref(&thing);``` which is is lighter, although arguably not as convelinet as clone still, and would require importing the function with a ```use``` statement. + The impact of not accepting this proposal would be that a "paper-cut" annoyance in the standard library remains. # Unresolved questions [unresolved]: #unresolved-questions -Is there a better name than ```new_ref```? The documentation uses the term _pointer_ in many places, although _reference_-counting is also present. ```new_ptr``` would work as well, although _ptr_ seems to be most used in referrence to raw pointers. +Is there a better name than ```new_ref```? The documentation uses the term _pointer_ in many places, although _reference_-counting is also present. ```new_ptr``` would work as well, although _ptr_ seems to be most used in referrence to raw pointers. ```clone_ref``` seems like another sufficitently descriptive name. Longer names such as ```new_reference``` or ```new_pointer``` are just as informative, although the extra length goes against the general convention of using short names for common constructs. Othe names such as ```new_rc```, ```new_arc```, etc. could be considered. From bd4e3a02be00c1256618f3ecf7a62f3e99daf0eb Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Tue, 21 Mar 2017 08:31:50 +0100 Subject: [PATCH 8/9] Add a comment about linter warnings. --- text/0000-rc-newref-clone.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/text/0000-rc-newref-clone.md b/text/0000-rc-newref-clone.md index 32b9d977247..ba8f30d1a0a 100644 --- a/text/0000-rc-newref-clone.md +++ b/text/0000-rc-newref-clone.md @@ -42,11 +42,12 @@ Note that these commits are missing the proper stable/unstable annotations. # How We Teach This [how-we-teach-this]: #how-we-teach-this -Note that this RFC proposes a more descriptive way to do express an operation, but does not prevent from continuing to use the more generic way (clone still works as expected and remains the generic way to . +Note that this RFC proposes a more descriptive way to do express an operation, but does not prevent from continuing to use the more generic way (clone still works as expected and remains the generic way to achieve the same result). -It would make sense for ```new_ref``` to be the preferred way to create new reference-counted pointers out of existing one, over the still existing ```clone``` method, since it is more descriptive. +It would make sense for ```new_ref``` to be the preferred way to create new reference-counted pointers out of existing one, over the still existing ```clone``` method. In order to teach this we should: -This is not a big change in the standard library, however, common code examples in the official documentation and tutorials should be changed if ```new_ref``` becomes the preferred way over ```clone```. + - Adapt common code examples in the official documentation and tutorials to use new_ref instead of clone. + - Add a linter warning about the usage of clone with reference counted pointers outside of generic code, which would recommand using new_ref instead. # Drawbacks [drawbacks]: #drawbacks From c8bddbdebdbcde572a6913809907b1b4a2f95d32 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Wed, 29 Mar 2017 00:53:25 +0200 Subject: [PATCH 9/9] Update the alternatives section. --- text/0000-rc-newref-clone.md | 41 ++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-) diff --git a/text/0000-rc-newref-clone.md b/text/0000-rc-newref-clone.md index ba8f30d1a0a..7637726fbea 100644 --- a/text/0000-rc-newref-clone.md +++ b/text/0000-rc-newref-clone.md @@ -59,25 +59,54 @@ Very short names are quite popular in the rust community for common operations. # Alternatives [alternatives]: #alternatives -This RFC tries to remain as straightforard and simple as possible. The tention between deep and shallow clones could maybe also be avoided through the addition of finer grained ShallowClone and DeepClone traits. It is not clear tothe author, however, if the added genericity would be useful in practice. +For alternative naming schemes, see the unresolved questions section. -As described in the drawbacks section, ```new_ref``` is a new method on the pointer type which can conflict with the method of the pointee type. Defining the method as ```fn new_ref(this: &Self) -> Self;``` solves the issue. The drawback of using this pattern is that new_ref becomes a lot less simple to call than clone (for example ```let thing_bis = Rc::new_ref(&thing);``` versus ```let thing_bis = thing.clone();```), and the risk is that clone remains the popular way of copying reference counted pointers due to being easier to use. -If a trait (let's call it ```NewRef```) is added for this purpose, a function working on NewRef types could be added so that the call sites could look like ```let thing_bis = new_ref(&thing);``` which is is lighter, although arguably not as convelinet as clone still, and would require importing the function with a ```use``` statement. +## Not change the standard library -The impact of not accepting this proposal would be that a "paper-cut" annoyance in the standard library remains. +A case can be made that writing ```let image2 = Rc::clone(&image1);``` is sufficient to avoid the confusion between deep and shallow clones. The advantage of this approach is that it does not require any addition to the standard library. The dowside is that it is know to very few, and strictly less convenient than writing ```let image2 = image1.clone();```. It also requires importing ```Rc``` into the module with a ```use``` directive. Unless the standard tool chain aggressively warns against calling clone with the method syntax, it is unlikely that the more explicit function syntax will be used in practice. + +## new_ref as an external function instread of a method on the pointer types + +As described in the drawbacks section, ```new_ref``` is a new method on the pointer type which can conflict with the method of the pointee type. Defining the method as ```fn new_ref(this: &Self) -> Self;``` solves the issue. The drawbacks of using this pattern are the same as using the function syntax for clone, with the added benefit that the function name is less vague than "clone". new_ref becomes is still a less simple to call than clone (for example ```let image2 = Rc::new_ref(&image1);``` versus ```let image2 = image1.clone();```), and the risk is that clone remains the popular way of copying reference counted pointers due to being easier to use. + +## Adding a trait + +A trait (let's call it ```NewRef```) could be added for this purpose. + +The trait definition could resemble the code below: +``` +trait NewRef { + fn new_ref(&self) -> Self; +} + +// In their respective modules the implementations could be (equivalent to): +impl NewRef for std::rc::Rc { self.clone() } +impl NewRef for std::rc::Weak { self.clone() } +impl NewRef for std::sync::Arc { self.clone() } +impl NewRef for std::sync::Weak { self.clone() } +``` + +One downside of exposing this method as a trait is that this trait needs to be explicitly imported with a ```use``` directive, which adds some friction compared to using clone, and makes new_ref less discoverable, although the method would not conflict with potentially existing new_ref methods on the pointee type. Perhaps this could be mitigated by adding the trait to the prelude, with the caveat that the method would then potentially conflict. + +A function working on NewRef types could be added so that the call sites could look like ```let thing_bis = new_ref(&thing);``` which is is lighter, although arguably not as convenient as clone still, and would also require importing the function with a ```use``` directive. + + +## Impact of not accepting this proposal or + +The impact of not accepting this proposal or another proposal aiming at solving the same problem would be that a "paper-cut" annoyance in the standard library remains. This annoyance does not affect the soundness of the language, although it tends to make common code patterns easy to misinterpret. # Unresolved questions [unresolved]: #unresolved-questions Is there a better name than ```new_ref```? The documentation uses the term _pointer_ in many places, although _reference_-counting is also present. ```new_ptr``` would work as well, although _ptr_ seems to be most used in referrence to raw pointers. ```clone_ref``` seems like another sufficitently descriptive name. Longer names such as ```new_reference``` or ```new_pointer``` are just as informative, although the extra length goes against the general convention of using short names for common constructs. -Othe names such as ```new_rc```, ```new_arc```, etc. could be considered. +Other names such as ```new_rc```, ```new_arc```, etc. could be considered. # Generalization (open discussion) The vagueness of Clone is not criticised, here. Clone, as defined and with its abstraction level, serves its purpose well as a trait for generic operations. The problem comes from _more-specific_ operations and types, which would benefit from being expressed with descriptive terminology, being _only_ usable through high level or abstract terms. -The author of this RFC believes that as in general, types (structures and enums) should provide methods using adequately descriptive names and, _in addition_ to these methods, implement the more abstract traits using these methods. This way, no compromise is made on the names of the functionality exposed by the type. This should of course be taken with a grain of salt. It really depends on how far apart the type and a trait are in terms of the genericity of the concept they embody or describe. +The author of this RFC believes that, in general, types (structures and enums) should provide methods using adequately descriptive names and, _in addition_ to these methods, implement the more abstract traits using these methods. This way, no compromise is made on the names of the functionality exposed by the type. This should of course be taken with a grain of salt. It really depends on how far apart the type and a trait are in terms of the genericity of the concept they embody or describe. # Example Here is an example of code where the similarity between ```Vec::clone()``` and ```Arc>::clone()``` is a recurrent source of confusion: