Show impl for Weak

[forticulous]

Show impl for Weak and tweaked the Show impl for Rc for consistency

[forticulous]

Hmm I see your point. So what should it do if the upgrade returns a None? fail or empty string?

[forticulous]

Or it could just say None I suppose

[sfackler]

It definitely shouldn't fail or print an empty string. Maybe something like <stale weak pointer>?

[forticulous]

What do you think about:

match self.upgrade() {
    Some(rc) => rc.fmt(f),
    _ => "<invalid reference>".fmt(f)
}

[sfackler]

Works for me

[forticulous]

I tested locally and when I used the upgrade() call in fmt it didn't print <invalid reference> so I switched to an if self.strong() == 0 { check

[huonw]

What did it do instead of printing <invalid reference>?

[forticulous]

It printed as if it weren't an invalid reference

[huonw]

That seems... wrong. Did you test with something like:

let x = Rc::new(1);
let y = x.downgrade();

// x is still valid, so should print 1
println!("{}", y);

// remove the strong reference
drop(x);

// no more strong references, should print <invalid reference>
println!("{}", y); 

Also, this needs a test.

[huonw]

(Also, I personally prefer <stale weak pointer> fwiw, since & is called a "reference" in Rust, meaning an "invalid reference" message could be very confusing.)

[forticulous]

I just tried it again locally with the upgrade() and it worked correctly so I must have done something wrong before.

What kind of test should there be?

I'm ok with <stale weak pointer>

[huonw]

You can use my example, except write assert_eq!(format!("{}", y).as_slice(), "1"); and assert_eq!(format!("{}", y).as_slice(), "<stale weak pointer>"); instead of the two println! calls.

[lilyball]

FWIW, while I think this is ok for rc::Weak, I want to caution anyone against trying to implement Show for arc::Weak. It works for Rc because it's task-constrained, and so showing a Weak won't affect the semantics of the program, but arc::Weak should never hide an upgrade().

However despite that, I'm still not entirely sure it's a good idea to be hiding an upgrade() in rc::Weak. It won't affect the semantics of the program, but it still just seems a bit odd. If I want to show a Weak I can just call upgrade() myself and show that.

[huonw]

It works for Rc because it's task-constrained, and so showing a Weak won't affect the semantics of the program

It can. The Show instance for the T in the Weak<T> may interact with the same Rc pointer.

arc::Weak should never hide an upgrade().

Why not?

(That said, I'd also be happy just printing <weak reference> always. shrug)

[lilyball]

The Show instance for the T in the Weak<T> may interact with the same Rc pointer.

Well, I'm referring specifically to whether or not it extends the lifetime of the value. But you're right, theoretically the Show instance for T could interact with the same Rc pointer, and could potentially even drop the Rc (via the use of some internally-mutable container), which would then cause the value itself to drop.

This reason is why the proposed Show instance here must use upgrade() in order to be safe. With the patch as it exists using inner() I can cause a segfault (not that I've tried, but I've proven to myself that I can drop the Rc during the Show, and so the upgrade() is necessary to keep the value alive past the end of Show).

In any case, I'm not sure if this is sufficient reason to say we shouldn't have Show on rc::Weak. After all, the lifetime of the value is changing because of the explicit use of Show, not because of the implicit use of upgrade(). But it is definitely something to consider.

arc::Weak should never hide an upgrade().

Why not?

At least in multi-threaded code, I believe strongly that clients of Arc need to explicitly upgrade their Weak to Arc before using it, to make it clear what's going on. Aside from the fact that this upgrade can change the lifetime of the value, hiding the upgrade also makes it too easy to upgrade the same Weak multiple times in a code block, instead of upgrading it once at the top, e.g.

println!("Twiddling the frobs with {}", foo);
match foo.upgrade() {
    Some(bar) => twiddleFrobsWith(bar),
    None => reportNoFrobsToTwiddle()
}

In the above code, the foo the println!() could print a valid value, and then the following upgrade() could return None, because another thread may have dropped the last strong reference in between those calls.

Incidentally, this is why (I assume; I didn't write it) Weak does not implement Deref and implicitly upgrade(). Implicit upgrades() are just generally a bad idea.

---

The issues with the above code are only a problem in concurrent code, so it doesn't actually apply to rc::Weak. However, that doesn't mean implicit calls to upgrade() are a good idea there. In fact, we should try to maintain API parity between rc::Weak and arc::Weak if we can, which suggests that we really should not be implementing Show on rc::Weak. Similarly we should maintain parity between rc::Rc and arc::Arc, although unfortunately it seems that right now rc::Rc has a few more traits implemented on it (such as Hash) than arc::Arc.

So I guess I'm now firmly on the side of saying we should reject this change.

[forticulous]

Pushed changing the invalid message to <stale weak pointer> and added test

[huonw]

This still needs to be using .upgrade().

[forticulous]

Some people are saying use upgrade(), some say not to. It seems simpler not to use upgrade since that will increase the strong count and then decrease it as the method ends and drop is called

[lilyball]

It is horribly unsafe to not use upgrade(). I have proof-of-concept code on my computer right now that will segfault if you don't use upgrade(), without using unsafe.

[huonw]

@kballard it would be nice to post it as a demonstration. :)

[lilyball]

Ok, here's the code I used to prove that the Show impl could remove the last Rc. This code as-written uses upgrade(), but if this PR as-written lands, I can ditch the upgrade() and start crashing:

use std::cell::RefCell;
use std::rc::Rc;
use std::fmt;

struct Foo {
    inner: RefCell<Option<Rc<Foo>>>,
    s: String
}

impl Foo {
    fn new(s: String) -> Rc<Foo> {
        let f = Rc::new(Foo { inner: RefCell::new(None), s: s });
        let f_ = f.clone();
        *f.inner.borrow_mut() = Some(f_);
        f
    }
}

impl fmt::Show for Foo {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        drop(self.inner.borrow_mut().take()); // kill the Rc
        // At this point, if the weak value did not use upgrade(), then the value will be
        // dropped and &self will be an invalid reference. As a result, the following line
        // will access deallocated memory (from the String).
        write!(f, r#"Foo \{ s: {} \}"#, self.s.as_slice().escape_default())
    }
}

fn main() {
    let f = Foo::new("test Foo".to_string());
    let f_ = f.downgrade();
    drop(f);
    println!("{}", f_.upgrade());
    println!("{}", f_.upgrade());
}

[forticulous]

Thanks for writing it out. I don't quite understand why it wouldn't work without the upgrade() if the current impl does the strong check, but I can take your word for it. I'm admittedly not an expert

[huonw]

@forticulous because the drop happens after the strong check, i.e. the strong check starts at 1, but then goes to 0 inside the fmt call in the else branch.

[lilyball]

@forticulous The current implementation only tests that it's alive before it does any work. But it then goes on to call a method on the value (value.fmt(f)), and that method is capable of dropping the last outstanding strong reference to the value (which is precisely what my code does). When that happens, the value will immediately be dropped, even though it's in the middle of executing a method that has a valid &self pointer. This breaks the guarantee that &-references always refer to valid values.

[forticulous]

Alright I'm sold. Changes coming up

[forticulous]

Changed var names in test

[forticulous]

Show for Rc same as it was, Show for Weak is using upgrade() now

[lilyball]

This looks better. Though as described above, I'm still against making this change at all.

[gereeter]

I think that this change is invalid, as it could cause fmt to go into an infinite loop, as in the following example:

struct Evil {
    val: RefCell<Option<Weak<Evil>>>
}

impl<T: fmt::Show> fmt::Show for Evil {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        self.val.borrow().fmt(f)
    }
}

fn main() {
    let a = Rc::new(Evil { val: RefCell::new(None) });
    let b = a.clone().downgrade();
    *a.val.borrow_mut() = Some(b);
    println!("{}", a);
}

While this code may seem odd, one of the motivations for Weak was the ability to create cycles, and so I think that the standard library should expect them.

[forticulous]

Yeah i'm not so crazy about the change if it has to call upgrade behind the scenes. Show should be a simple object to text conversion without any logic imho. If this has to convert to other objects and do logic maybe its just not worth it. It wouldn't break my heart if it got rejected

[sfackler]

I think Weak should definitely have a Show impl, even if it's just <weak pointer> or whatever.

[lilyball]

@sfackler What use is there for implementing Show to print <weak pointer>? How is that any better than just not trying to Show the weak pointer at all?

The only use-case I can think of is for deriving Show on a struct that contains a weak pointer, but it seems like maybe a struct containing a weak pointer might need to have a custom-written Show anyway, as weak pointers are not a common thing to put in most types. Besides, a generic solution that would be to allow for customization of deriving (either by skipping fields, or providing an override for how the field is handled), which is roughly what #14268 is about.

[forticulous]

As discussed it doesn't seem worth it to add this