-
Notifications
You must be signed in to change notification settings - Fork 179
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
Add projection utilities to Yoke #833
Conversation
The ICE is due to rust-lang/rust#86703 . |
utils/yoke/src/yoke.rs
Outdated
/// - `P` ends up borrowing data from `Y` (or elsewhere) that did _not_ come from the cart, | ||
/// for example `P` could borrow owned data from a `Cow`. | ||
/// - Borrowed data from `Y` escapes with the wrong lifetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this list complete? I'm struggling to convince myself it is. Here's a proof sketch that feels right but which I'm not willing to bet on:
If Yoke<Y>
is destroyed, it must not invalidate Yoke<P>
. The yoked data will still be alive, because of CloneableCart
, so it remains to check that we don't keep any of the underlying bookkeeping data that Y would be entitled to destroy. We can't hold onto any data that lives only for 'y
(the lifetime of the Y
object) so data owned by Y
is out. So we can only copy data from Y
, so it remains to check that copying non-'static
data is ok, which reduces to checking if copying references is ok.
If the reference has the special for<'a>
yoke lifetime, that means it's attached to the cart, so it can be copied, since it's the same yoke lifetime on both sides. If the reference has some other lifetime, like if we were converting Y<'static, 'b> -> P<'static>
, then we can only copy it if P
can express 'b
separately from the yoke lifetime, since the borrowed content is not yoked.
Now, the part I'm not sure on is that I've referred to "the" yoke lifetime, but one could conceive of multiple yoke lifetimes being active simultaneously? Like can you wind up in a situation where you have something silly like
struct NestedYokeable<'y> {
inner: Box<Yoke<NestedYokeable<'y>, Cart>>,
ref: &'y i32,
}
Could you sneak yoked content intended for the outer yoke into the inner yoke, thus breaking the guarantee we otherwise get from CloneableCart? Can a value of this type even be constructed safely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you sneak in other lifetimes they're like adding more existential parameters, essentially.
Note that the Yokeable
used in a Yoke
must be 'static
, you can only sneak in other lifetimes via the cart. Yokes only have one "the" yoke lifetime because of Yokeable, all other lifetimes are in the cart and would constrain both sides in an acceptable way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could do
y.project(|y, _| y.inner.as_ref().clone())
but that's fine, since the lifetimes match up anyways (since you've cloned).
I think I'm pretty convinced this is correct. Huzzah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some bikeshedding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! (Although we will need to fix the coverage CI before this is mergeable)
Yeah the only fix is to ignore that doctest, which I did |
Codecov Report
@@ Coverage Diff @@
## main #833 +/- ##
==========================================
- Coverage 74.80% 74.71% -0.09%
==========================================
Files 198 198
Lines 12762 12777 +15
==========================================
Hits 9546 9546
- Misses 3216 3231 +15
Continue to review full report at Codecov.
|
Pull Request Test Coverage Report for Build 0660b94ac44c19e2bb9274e1eef7c053647cd2f4-PR-833
💛 - Coveralls |
Fixes #829
Discovered another compiler bug in the process 🙃 (rust-lang/rust#86702)
I also figured out a better workaround for rust-lang/rust#84937 and used it here: there's a separate version of this function with explicit capturing that can be used until we can use closures here.
Fallible
try_
versions of this can be added if necessary.