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

Forward Show, Eq, and Hash to RefCell's contents #16714

Closed
wants to merge 1 commit into from

Conversation

shepmaster
Copy link
Member

I'm happy to add some tests, but none of the existing context did so. I assumed that this code is "obviously correct" to visual inspection.

@lilyball
Copy link
Contributor

We already decided in #14883 that Show is not appropriate for RefCell because RefCell borrowing is fallable and Show should never be.

I believe the same argument applies to Eq and Hash. None of these traits should be fallable, and therefore are not suitable for RefCell.

That said, #15411 implemented Encodable/Decodable on RefCell, even though I would think the argument applies there as well. I missed that when it happened or I would have argued against it at the time. /cc @alexcrichton who approved it.

@alexcrichton
Copy link
Member

As I commented on the PR and as the code says, the encoding aspect is meant to not fail, but rather to return an error (because the encodable/decodable context allows it to). It is was a stopgap until we could have custom error strings for any encoder (as we can do now).

As noted, previous PRs to add Show have been declined for now due to the fallibility of RefCell. That said, I am sympathetic to any structure containing a RefCell basically doesn't work with #[deriving], which is quite unfortunate.

@lilyball
Copy link
Contributor

@alexcrichton I believe that the solution to using #[deriving] with RefCell-containing structures is to add the ability to customize #[deriving], as is mentioned in #14268 and #15795. In this case, we'd need the ability to basically substitute an arbitrary expression for the field in the derived traits.

@shepmaster
Copy link
Member Author

I was going to ask about using try_borrow, but I see that was the original proposal.

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.

3 participants