Add yoke crate #705

Manishearth · 2021-05-05T18:25:02Z

This implements the data structure described in #667 (comment)

The Yoke abstraction allows us to "yoke" Cows and similar types (ZeroVecs, and data structures using Cow/ZeroVec) to their backing storage, allowing us to turn the lifetimes into dynamic ones that can be managed by Rcs and caches.

Carried over from #696 because my WIP force pushes made the bot leave a million comments.

Manishearth · 2021-05-05T18:28:41Z

As far as the soundness of the unsafe code is concerned I've asked @mcy and @tmandry to take a look at it.

codecov-commenter · 2021-05-05T18:39:02Z

Codecov Report

Merging #705 (c3d15ef) into main (478c4a9) will increase coverage by 0.50%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #705      +/-   ##
==========================================
+ Coverage   74.18%   74.69%   +0.50%     
==========================================
  Files         171      176       +5     
  Lines       10802    10712      -90     
==========================================
- Hits         8014     8001      -13     
+ Misses       2788     2711      -77

Impacted Files	Coverage Δ
utils/zerovec/src/lib.rs	`100.00% <ø> (ø)`
utils/yoke/src/lib.rs	`100.00% <100.00%> (ø)`
components/datetime/src/error.rs	`9.09% <0.00%> (-10.91%)`	⬇️
components/datetime/src/pattern/error.rs	`25.00% <0.00%> (-10.72%)`	⬇️
components/plurals/src/rules/lexer.rs	`88.17% <0.00%> (-1.94%)`	⬇️
components/datetime/src/fields/symbols.rs	`68.06% <0.00%> (-0.53%)`	⬇️
utils/fixed_decimal/src/lib.rs	`50.00% <0.00%> (ø)`
components/provider/src/error.rs	`0.00% <0.00%> (ø)`
components/capi/src/pluralrules.rs	`0.00% <0.00%> (ø)`
components/plurals/src/operands.rs	`91.95% <0.00%> (ø)`
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 478c4a9...c3d15ef. Read the comment docs.

coveralls · 2021-05-05T18:39:51Z

Pull Request Test Coverage Report for Build bfae49cd313f5f6272658048f463e61c0484060d-PR-705

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 74.118%

Totals
Change from base Build c6582ccddf87d49a44e2d4e54604543365c76bff:	0.002%
Covered Lines:	8127
Relevant Lines:	10965

💛 - Coveralls

mcy

Manish asked me to perform an unsafe code review. I am not confident this is correct yet, so I've asked for some language clarifications to make sure my understanding is sound.

utils/yoke/src/yokeable.rs

mcy · 2021-05-05T21:21:27Z

utils/yoke/src/yokeable.rs

+    /// This enforces that while `F` may mutate `Self<'a>`, it can only mutate it in a way that does
+    /// not insert additional references. For example, `F` may call `to_owned()` on a `Cow` and mutate it,


Are you sure? What if it accesses a thread-local?

I don't believe this is an issue but it's worth explicitly checking this case. (Also, I wonder if there is evil to be done with global mutices...)

You can't borrow from a thread local or a global mutex, you can only do scoped operations on them.

Oh, what about

let empty: &'static mut [u8] = &mut [];

Similarly, Box::leak? I don't think these should be an issue either, because those are truly 'static and that lifetime is covariant. Just trying to nail down every source of borrowed data I can think of.

Yeah, those are truly 'static. And static mut is unsafe to work with anyway.

utils/yoke/src/yokeable.rs

utils/yoke/src/zerovec_impls.rs

utils/yoke/src/cart.rs

utils/yoke/src/yoke.rs

utils/yoke/src/yokeable.rs

…hods

mcy

Ok, I think I'm mostly? confident that this is ok. I haven't managed to think of a way to break out of the "sandbox" so to speak. I'd like to see what Tyler says before formally calling this safe.

mcy · 2021-05-06T14:12:24Z

utils/yoke/src/cart.rs

+/// is never accessed via `&mut` references.
+///
+/// For example, `Rc<Vec<u8>>` and `Rc<[u8]>` are valid [`Cart`]s, however `Rc<RefCell<Vec<u8>>>` is
+/// not, because in the latter type it is possible to use interior mutation to change the data.


So, again to clarify, this sounds very strongly like a "pinned as long as a &T is reachable", but I'm not certain that's what you're saying.

Pinned as long as &T::Inner is reachable, yes.

mcy · 2021-05-06T14:13:43Z

utils/yoke/src/cart.rs

+/// In general, this means "no interior mutability", though interior mutability can be fine
+/// if the interior mutability is to something that does not affect references. For example,
+/// `Rc<[u8]>` does have interior mutability in the refcount, but you cannot get references
+/// to it, so it's fine. On the other hand, `Weak<Box<[u8]>>` cannot be a valid [`Cart`] because
+/// it is possible for the backing buffer to be cleaned up without warning.


Right, and then this means that as a corollary, Rc<Vec<RefCell<u8>>> is ok but the transposition Rc<RefCell<Vec<u8>> is not, because you might cause the Vec's pointee address to change?

Actually thinking about it more both are probably okay: if you have a refcell you can't take longer-lived references to its contents anyway. Neither are useful types since you can't zero copy deserialize (or whatever) from either.

The goal is basically avoiding this pattern:

yoke.attach_to_cart(cart, |data| { do_something_that_borrows_from_data(data) }); yoke.backing_cart().mutate_the_borrowed_thing();

the danger is that I'm not 100% sure there's no way to use interior mutability here where the lifetime escapes, because we are casting the lifetime a bunch of times.

mcy · 2021-05-06T14:20:04Z

utils/yoke/src/yokeable.rs

+    /// This enforces that while `F` may mutate `Self<'a>`, it can only mutate it in a way that does
+    /// not insert additional references. For example, `F` may call `to_owned()` on a `Cow` and mutate it,


Oh, what about

let empty: &'static mut [u8] = &mut [];

Similarly, Box::leak? I don't think these should be an issue either, because those are truly 'static and that lifetime is covariant. Just trying to nail down every source of borrowed data I can think of.

sffc · 2021-05-08T23:54:22Z

utils/yoke/src/yoke.rs

+    }
+}
+
+// clone impls only work for reference counted objects, otherwise you should be


Suggestion: use CloneStableDeref here to make this a bit more general. It works for Rc, Arc, and &'a T.

Hmm, so this works, but then you can no longer have the impl on Option<Rc<T>> (since stable_deref_trait could potentially introduce a impl CloneStableDeref for Option<T> -- it wouldn't make sense but the compiler doesn't know that)

Okay, can you at least add the missing impls? Consider using macro_rules to make it shorter.

utils/yoke/src/yoke.rs

Co-authored-by: Shane F. Carr <[email protected]>

Manishearth · 2021-05-09T15:12:55Z

retriggering docs

Add yoke crate

2337241

Manishearth requested a review from a team as a code owner May 5, 2021 18:25

Manishearth requested review from sffc and zbraniecki May 5, 2021 18:25

Fix lint errors

4035b00

Manishearth mentioned this pull request May 5, 2021

(old PR) WIP: Add yoke crate #696

Closed

Fix lint again

4922733

mcy reviewed May 5, 2021

View reviewed changes

sffc reviewed May 5, 2021

View reviewed changes

Manishearth added 8 commits May 5, 2021 15:27

Clarifications for Miguel

73fd545

Address shane's comments

010a363

Swap Yokeable::new()

c35017d

more fmt

eea0986

mention lifetime

e4edeaa

Add Cartable for references

b05da39

Remove unwrapping Cart impl on Option, move to having more attach met…

2f1d0de

…hods

Reorganize yoke code

a51139a

Manishearth requested a review from sffc May 6, 2021 07:04

mcy reviewed May 6, 2021

View reviewed changes

Manishearth added 2 commits May 6, 2021 07:38

Use ptr::read instead of transmute_copy

387f122

Use StableDeref instead of Cart

c088ff6

Manishearth added the waiting-on-reviewer PRs waiting for action from the reviewer for >7 days label May 7, 2021

Manishearth closed this May 7, 2021

Manishearth reopened this May 7, 2021

sffc reviewed May 8, 2021

View reviewed changes

sffc reviewed May 9, 2021

View reviewed changes

utils/yoke/src/yoke.rs Outdated Show resolved Hide resolved

Update utils/yoke/src/yoke.rs

6f06413

Co-authored-by: Shane F. Carr <[email protected]>

Manishearth requested a review from sffc May 9, 2021 15:11

Manishearth closed this May 9, 2021

Manishearth reopened this May 9, 2021

Manishearth added 2 commits May 10, 2021 16:06

Add more Clone impls

cc429db

Move zerovec yoke impls to zerovec

a2994d9

sffc previously approved these changes May 10, 2021

View reviewed changes

lint

c3d15ef

Manishearth dismissed sffc’s stale review via c3d15ef May 11, 2021 00:00

Manishearth requested a review from sffc May 11, 2021 00:02

sffc approved these changes May 11, 2021

View reviewed changes

Manishearth merged commit 82ace5e into unicode-org:main May 11, 2021

Manishearth deleted the yoke branch June 9, 2021 01:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add yoke crate #705

Add yoke crate #705

Manishearth commented May 5, 2021

Manishearth commented May 5, 2021 •

edited

Loading

codecov-commenter commented May 5, 2021 •

edited

Loading

coveralls commented May 5, 2021 •

edited

Loading

mcy left a comment

mcy May 5, 2021

Manishearth May 5, 2021

mcy May 6, 2021

Manishearth May 6, 2021

mcy left a comment

mcy May 6, 2021

Manishearth May 6, 2021

mcy May 6, 2021

Manishearth May 6, 2021

mcy May 6, 2021

sffc May 8, 2021

Manishearth May 9, 2021

sffc May 10, 2021

Manishearth commented May 9, 2021

		/// This enforces that while `F` may mutate `Self<'a>`, it can only mutate it in a way that does
		/// not insert additional references. For example, `F` may call `to_owned()` on a `Cow` and mutate it,

Add yoke crate #705

Add yoke crate #705

Conversation

Manishearth commented May 5, 2021

Manishearth commented May 5, 2021 • edited Loading

codecov-commenter commented May 5, 2021 • edited Loading

Codecov Report

coveralls commented May 5, 2021 • edited Loading

Pull Request Test Coverage Report for Build bfae49cd313f5f6272658048f463e61c0484060d-PR-705

💛 - Coveralls

mcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented May 9, 2021

Manishearth commented May 5, 2021 •

edited

Loading

codecov-commenter commented May 5, 2021 •

edited

Loading

coveralls commented May 5, 2021 •

edited

Loading