Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rc::make_mut doesn't clone if there are weak refs #60961

Closed
avl opened this issue May 19, 2019 · 3 comments · Fixed by #61135
Closed

Rc::make_mut doesn't clone if there are weak refs #60961

avl opened this issue May 19, 2019 · 3 comments · Fixed by #61135
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@avl
Copy link

avl commented May 19, 2019

The docs for Rc::make_mut say:

If there are other Rc or Weak pointers to the same value, then make_mut will invoke clone on the inner value to ensure unique ownership. This is also referred to as clone-on-write.

(See https://doc.rust-lang.org/std/rc/struct.Rc.html#method.make_mut )

As I read the above paragraph, the last line of the following program should not print 'None'. In fact, it does print None, at least on stable rust 1.34.2.

use std::rc::Rc;

#[derive(Clone,Debug)]
struct Example {
    value: String
}

pub fn main() {
    let mut strong1 = Rc::new(Example{value:"First".to_string()});
    let mut strong2 = strong1.clone();
    let weak2 = Rc::downgrade(&strong2);
    strong2 = Rc::new(Example{value:"Second".to_string()}); //Make sure there is one strong and one weak
    println!("Strong count: {}",Rc::strong_count(&strong1));
    println!("Weak count: {}",Rc::weak_count(&strong1));
    let strong1mut = Rc::make_mut(&mut strong1);  //should clone Example, at least according to docs
    strong1mut.value = "Mutated".to_string();
    let weak2ref = weak2.upgrade();
    println!("Weak: {:?}",weak2ref); //Should not print None, at least docs seem to imply this
    
}

I'm not being bit by this, possibly no-one is. Just thought I should report it.

Edit: The following paragraph didn't make much sense. This is just a doc error. After make_mut has finished, there are no strong refs so the weak refs should just all die anyway. The docs as written don't make sense, the implementation is the only reasonable one.

(If I haven't missed anything, and there actually is a discrepancy between the documentation and the code, what would be the correct solution? Changing the code or the docs?)

@jonas-schievink
Copy link
Contributor

Looking at the implementation, the behavior looks correct, but the docs are incorrect in claiming that Clone will be called when there are only Weak pointers left (in that case the implementation will just "steal" the value by moving it into a new Rc and leaving the old one destroyed so the Weaks can't be upgraded).

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 19, 2019
@avl
Copy link
Author

avl commented May 19, 2019

I can see cases where you as a programmer would want either the documented behavior or the implemented behavior. Either way docs and implementation should agree 😃 .
Edit: Above does not make sense.

@avl
Copy link
Author

avl commented May 19, 2019

Now I get it. The documented behavior doesn't make sense, does it? After make_mut has run, there are no strong refs, so by the definition of a weak ref all the weak refs should die. Disregard my comment above. This is just a minor documentation issue.

@jonas-schievink jonas-schievink added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label May 19, 2019
Centril added a commit to Centril/rust that referenced this issue Jun 4, 2019
…rk-Simulacrum

Fix documentation of `Rc::make_mut` regarding `rc::Weak`.

Closes rust-lang#60961
Centril added a commit to Centril/rust that referenced this issue Jun 4, 2019
…rk-Simulacrum

Fix documentation of `Rc::make_mut` regarding `rc::Weak`.

Closes rust-lang#60961
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants