Skip to content

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented May 28, 2025

RUST-1887

This turned out to be a bit more of an adventure than I was expecting. Because we (still 😞) don't have specialization, I couldn't directly express "accept anything that's impl Into<RawBson> or impl Into<RawBsonRef>", so this PR implements the next best thing. It's a bit ugly but I lean towards it being worth it for the better user experience (append is the core of the rawdoc macro so this will make that transparently accept a wider range of types).

/// Types that can be consumed to produce raw bson references valid for a limited lifetime.
/// Conceptually a union between `T: Into<RawBson>` and `T: Into<RawBsonRef>`; if your type
/// implements either of those you should consider adding an impl for this as well.
pub trait BindRawBsonRef {
Copy link
Contributor Author

@abr-egn abr-egn May 28, 2025

Choose a reason for hiding this comment

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

Because this is consuming a value to produce a reference, it couldn't just be a straightforward Into-style converter; for owned types that would invalidate the backing storage for the reference we're trying to get. Instead I ended up with this mildly loopy way to consume a value to pass a reference to a closure, which bounds the lifetime.

I named it bind because in a past life I was a Haskell programmer and this is giving me flashbacks to the infamous monadic bind operator.

};
}

raw_bson_ref_from_impls! {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pair of lists are writing out all of the From impls for RawBsonRef and RawBson, with preference given to RawBsonRef when there's overlap; merging these lists is what the new trait enables.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the overlap prevent blanket impls for T where T: Into<RawBson> and T where T: Into<RawBsonRef>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't do both but I realized we could do one, so I've revised this to have a blanket impl for T: Into<RawBsonRef> and a manual list jsut for RawBson.

@abr-egn abr-egn marked this pull request as ready for review May 28, 2025 19:09
@abr-egn abr-egn requested a review from a team as a code owner May 28, 2025 19:09
@abr-egn abr-egn requested a review from isabelatkinson May 28, 2025 19:09
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

I have a question and suggestion, but overall this seems like an improvement to me!

@@ -188,7 +188,6 @@ impl RawDocumentBuf {
/// result in errors when communicating with MongoDB.
///
/// If the provided key contains an interior null byte, this method will panic.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some language here about the acceptable input types and show a borrowed value being appended in the code example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

};
}

raw_bson_ref_from_impls! {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the overlap prevent blanket impls for T where T: Into<RawBson> and T where T: Into<RawBsonRef>?

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

lgtm!

@abr-egn abr-egn merged commit c038fb5 into mongodb:main Jun 2, 2025
9 of 11 checks passed
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.

2 participants