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

Show impl for RefCell, Ref & RefMut #14811

Merged
merged 1 commit into from
Jun 12, 2014
Merged

Show impl for RefCell, Ref & RefMut #14811

merged 1 commit into from
Jun 12, 2014

Conversation

forticulous
Copy link

Show impl for RefCell and friends

@alexcrichton
Copy link
Member

This is an interesting direction to take because it means that Show for RefCell is fallible. I don't think that there are many other fallible implementations.

@forticulous
Copy link
Author

I was trying to stick to the idea that the Show impl would borrow a reference to the inner value and call fmt on that. Can you elaborate on the fallible part?

@huonw
Copy link
Member

huonw commented Jun 11, 2014

let x = RefCell::new(1);
let y = x.borrow_mut();

// this will fail, due to the `y` mutable borrow stopping
// any other borrow/borrow_mut.
println!("{}", x);

@forticulous
Copy link
Author

This would also fail for the same reasons:

let mut x = 75u;
let y = &mut x;

println!("{}", x);

@alexcrichton
Copy link
Member

Your example code, @forticulous, does not fail (you can try to run it). @huonw explained why this type is fallible.

@forticulous
Copy link
Author

I did just run it on rust-lang.org

error: cannot borrow `x` as immutable because it is also borrowed as mutable
note: in expansion of format_args!
<std macros>:2:23: 2:77 note: expansion site
<std macros>:1:1: 3:2 note: in expansion of println!
error: aborting due to previous error

@alexcrichton
Copy link
Member

Note that it is a compilation error, not a runtime error. RefCell yields runtime errors.

@huonw
Copy link
Member

huonw commented Jun 11, 2014

The RefCell version fails at runtime. The type is design as a way to get around the hard limits of &mut pointers and aliasability, but this ends up requiring runtime checks (and failure) instead of compile time ones.

@forticulous
Copy link
Author

That's the whole idea of RefCell isn't it? Checking borrow rules at runtime instead of at compilation time.

@forticulous
Copy link
Author

Here's maybe a better example (though this would also be my fault)

let mut x = Rc::new(75u);
let y = x.make_unique();

println!("{}", x);

error: cannot borrow `x` as immutable because it is also borrowed as mutable

@huonw
Copy link
Member

huonw commented Jun 11, 2014

Yep, that's the idea of RefCell, the point is this Show impl differs to all other ones, because it can reasonably fail at runtime, no other implementation of Show will fail like this. (The examples you have given are just invalid code which fail to compile; also, the errors are not related to Show, in particular, replacing the println! with just let z = &x; will display the same errors.)

I would be very happy to merge Ref and RefMut and then consider the RefCell implementation in a separate PR.

@forticulous
Copy link
Author

Yeah, it seems like when you choose to use RefCell you are intentionally giving up some of the compile time safety and relying on runtime checks so you're opening yourself up to possible runtime failures.

I can cut out the RefCell stuff for now and make an update

@forticulous
Copy link
Author

Updated with just Ref and RefMut

bors added a commit that referenced this pull request Jun 12, 2014
@bors bors closed this Jun 12, 2014
@bors bors merged commit a8f581f into rust-lang:master Jun 12, 2014
@forticulous forticulous deleted the refcell-show branch June 13, 2014 03:37
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2023
feat: Render hover actions for closure captures and sig

Also special cases closures for ranged type hover to render the closure hover instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants