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

impl Debug for Readers #390

Closed
wants to merge 2 commits into from
Closed

impl Debug for Readers #390

wants to merge 2 commits into from

Conversation

as-com
Copy link
Contributor

@as-com as-com commented Apr 16, 2023

This generates implementations of Debug for Readers for easier debugging. (I did not implement it for Builders because of some messy lifetimes issues.)

To make this work, I changed the getters on Readers to borrow instead of consume (see: #241 (comment)). This should be a safe and backwards-compatible change but I am not completely sure. I had to do this because reborrow throws away lifetime information that is necessary for the Debug implementations to pass the borrow checker.

@dwrensha
Copy link
Member

Thanks! I agree that there should be an easy way to print out a textual representation of a reader.
To achieve that, what I really want to do is to port capnproto-c++'s DynamicValue to Rust. That would let us write a Debug impl that require much less generated code -- all readers would just delegate to the same stringification code. I've made some initial small steps in #392 and I intend to continue exploring that.

@as-com
Copy link
Contributor Author

as-com commented Apr 17, 2023

Something that I considered was implementing LabelledGeneric where Readers' implementations produce an HList of something like Field<(f,i,e,l,d,__,n,a,m,e), PointerField<type::Reader<'_>, Text>> for a sort of type-safe reflection. This could result in lots of messy code requiring specialization though.

Should I separate out changing the getters' self parameter into a separate PR?

@@ -130,6 +131,13 @@ impl<'a> RustNodeInfo for node::Reader<'a> {
.collect::<Vec<String>>()
.join(", ")
+ " ");
let where_clause_with_debug = "where ".to_string()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of doing this we could add the Debug bound in the definition of the Owned trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a Debug bound to the Owned trait required adding more constraints to a lot of other Readers. I've put this in a separate commit - which way do you think is cleaner?

@dwrensha
Copy link
Member

Should I separate out changing the getters' self parameter into a separate PR?

How did changing the self parameter help you? My understanding is that you should be able to insert reborrow() calls to make things work, but also in the Reader case you shouldn't need the reborrow() calls. Does your issue have to do with some interaction with generics?

@as-com
Copy link
Contributor Author

as-com commented Apr 17, 2023

How did changing the self parameter help you?

The Debug trait's fmt method uses &self, which means if the getter calls consume self, a reborrow is necessary. The return value of a struct getter after a reborrow has some anonymous lifetime that does not outlive the function, but the returned struct must be passed into the provided Formatter<'_> and must outlive the function call. The reborrow erases the longer lifetime of &self, causing the borrow checker to fail.

@as-com
Copy link
Contributor Author

as-com commented Apr 17, 2023

That being said, it appears that changing the generated reborrow function to have a signature reborrow(&self) -> Reader<'a, ...> (instead of -> Reader<'_, ...>) allows the reborrows to work as expected (not completely sure of the safety though). This is still less ergonomic than not having to reborrow in the first place though.

@dwrensha
Copy link
Member

Ah, the problem is that generic readers were not Copy. I believe that after #393 your self -> &self change is no longer needed.

@as-com
Copy link
Contributor Author

as-com commented Apr 18, 2023

Ah, the problem is that generic readers were not Copy. I believe that after #393 your self -> &self change is no longer needed.

Yep, that was the issue. I removed the &self change.

@dwrensha
Copy link
Member

I've made significant progress on #392, and I'm now convinced that approach is going to work. It should generalize to Builders too.

My plan is to get it into a releasable state and to ship it (including Debug impls for readers) within a small number of weeks from now.

@dwrensha
Copy link
Member

dwrensha commented May 5, 2023

@as-com: #392 is almost ready. I would welcome any feedback on it. I intend to write a blog post about it and release capnproto-rust v0.17.0 soon.

@dwrensha
Copy link
Member

dwrensha commented May 8, 2023

Closing in favor of #392.
Thanks for the PR!

@dwrensha dwrensha closed this May 8, 2023
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.

None yet

2 participants