Add custom derive for Yokeable and ZCF #847

Manishearth · 2021-07-02T21:43:17Z

Fixes #761

This doesn't add the DataStruct custom derive, but it comes close.

In the interest of not moving all of our source to pure-zerocopy datastructures, I've included a #[yoke(CloningZCF)] attribute which, when used, implemends ZCF using .clone() instead of the right way. Over time we should be getting rid of these attributes, any instance of this attribute means that the ZCF is not actually zero-copy.

I have left #844 as a followup; right now the derive(Yokeable) derives its safety from getting the compiler to prove covariance. ZeroMap is not designed in a way that lets the compiler prove covariance, and for such types the custom derive will need to be a little different.

cc @shadaj

sffc

Cool!

components/datetime/Cargo.toml

components/datetime/src/provider/gregory.rs

components/decimal/src/provider.rs

provider/core/src/marker/macros.rs

utils/yoke/derive/src/lib.rs

sffc · 2021-07-02T22:47:27Z

utils/yoke/src/macro_impls.rs

+// This is for when we're implementing Yoke on a complex type such that it's not
+// obvious to the compiler that the lifetime is covariant


Suggestion (optional): Is it possible to export this, requiring that it be wrapped in unsafe {} ?

I'd rather not: Everyone else should use the custom derive, we're doing this because we're implementing the trait on types we do not own.

sffc · 2021-07-02T22:47:49Z

utils/yoke/src/macro_impls.rs

+
+// This is for when we're implementing Yoke on a complex type such that it's not
+// obvious to the compiler that the lifetime is covariant
+macro_rules! unsafe_complex_yoke_impl {


Question: Can this impl be used on ZeroMap?

ZeroMap's weird.

utils/yoke/src/zero_copy_from.rs

components/datetime/src/provider/time_zones.rs

sffc · 2021-07-04T06:20:16Z

provider/core/Cargo.toml

@@ -36,7 +36,7 @@ icu_locid = { version = "0.2", path = "../../components/locid" }
 tinystr = "0.4.5"
 writeable = { version = "0.2", path = "../../utils/writeable" }
 thiserror = "1.0"
-yoke = { version = "0.2", path = "../../utils/yoke", features = ["serde"] }
+yoke = { version = "0.2", path = "../../utils/yoke", features = ["serde", "derive"] }


Discussion: should we make the , "derive" be an optional feature, so we don't pull in syn? If every data struct effectively requires the custom derive, though, maybe this is unavoidable.

yeah everything needs the derive anyway. I guess utility crates could avoid it but I don't see much of a benefit

utils/zerovec/src/yoke_impls.rs

sffc · 2021-07-04T06:24:53Z

utils/zerovec/src/yoke_impls.rs

+    <K as ZeroMapKV<'static>>::Container: for<'b> ZeroCopyFrom<<K as ZeroMapKV<'b>>::Container>,
+    <V as ZeroMapKV<'static>>::Container: for<'b> ZeroCopyFrom<<V as ZeroMapKV<'b>>::Container>,
+    <K as ZeroMapKV<'static>>::Container:
+        for<'b> Yokeable<'b, Output = <K as ZeroMapKV<'b>>::Container>,
+    <V as ZeroMapKV<'static>>::Container:
+        for<'b> Yokeable<'b, Output = <V as ZeroMapKV<'b>>::Container>,


Question: Did you check that this impl compiles at the call site and doesn't hit rust-lang/rust#85636?

It does, at least in the ways I'm trying it. That bug is pretty subtle; if you tweak things a little bit it stops occurring (and more tweaks make it recur).

But also I don't think this matters until we try to use this; I'm just fixing up the impl early. There's more work to be done for ZeroMap in (non cloning) ZCF derives

sffc · 2021-07-04T06:27:18Z

docstest isn't happy:

---- src/zoned_datetime.rs - zoned_datetime::ZonedDateTimeFormat::format (line 174) stdout ----
error[E0597]: `zdtf` does not live long enough
  --> src/zoned_datetime.rs:191:22
   |
19 | let formatted_date = zdtf.format(&zoned_datetime);
   |                      ^^^^------------------------
   |                      |
   |                      borrowed value does not live long enough
   |                      argument requires that `zdtf` is borrowed for `'static`
...
22 | } _doctest_main_src_zoned_datetime_rs_174_0() }
   | - `zdtf` dropped here while still borrowed

Manishearth · 2021-07-04T16:15:11Z

docstest isn't happy:

fascinating; I realized it was because my cloning_zcf impl was not quite correct; however I'm not sure how it leaked out that far implicitly. My current assumption is that DataPayload relies on ZCF in complicated ways and my broken impl affected the variance of DataPayload (and thus ZonedDateTimeFormat) somehow

codecov-commenter · 2021-07-04T16:26:45Z

Codecov Report

Merging #847 (672862b) into main (0d428d3) will decrease coverage by 0.25%.
The diff coverage is 72.37%.

@@            Coverage Diff             @@
##             main     #847      +/-   ##
==========================================
- Coverage   74.62%   74.37%   -0.26%     
==========================================
  Files         202      206       +4     
  Lines       12841    13032     +191     
==========================================
+ Hits         9583     9692     +109     
- Misses       3258     3340      +82

Impacted Files	Coverage Δ
provider/core/src/data_provider.rs	`76.56% <ø> (+6.25%)`	⬆️
provider/core/src/erased.rs	`82.88% <ø> (ø)`
provider/core/src/inv.rs	`0.00% <ø> (ø)`
provider/core/src/lib.rs	`100.00% <ø> (ø)`
provider/core/src/serde.rs	`44.44% <ø> (ø)`
provider/core/src/struct_provider.rs	`85.71% <ø> (ø)`
utils/yoke/src/lib.rs	`100.00% <ø> (ø)`
utils/yoke/src/macro_impls.rs	`0.00% <0.00%> (ø)`
utils/zerovec/src/map/mod.rs	`92.50% <ø> (ø)`
utils/yoke/src/zero_copy_from.rs	`77.77% <60.00%> (-10.46%)`	⬇️
... and 45 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 0d428d3...672862b. Read the comment docs.

coveralls · 2021-07-04T16:27:03Z

Pull Request Test Coverage Report for Build f665f1b40009f0ba250aceb427d4d5806476a75f-PR-847

132 of 182 (72.53%) changed or added relevant lines in 15 files are covered.
23 unchanged lines in 5 files lost coverage.
Overall coverage decreased (-0.05%) to 74.458%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
components/datetime/src/provider/gregory.rs	5	6	83.33%
components/datetime/src/provider/time_zones.rs	6	7	85.71%
components/decimal/src/provider.rs	2	3	66.67%
utils/yoke/src/zero_copy_from.rs	6	10	60.0%
provider/macros/src/lib.rs	16	22	72.73%
utils/yoke/src/macro_impls.rs	0	12	0.0%
utils/yoke/derive/src/lib.rs	81	106	76.42%

Files with Coverage Reduction	New Missed Lines	%
components/plurals/src/provider.rs	1	22.22%
experimental/provider_ppucd/src/parse_ppucd.rs	1	93.0%
components/locale_canonicalizer/src/provider.rs	2	8.0%
components/decimal/src/provider.rs	7	45.45%
components/datetime/src/provider/time_zones.rs	12	47.5%

Totals
Change from base Build b9acfc49c983106924f0e201ccccf3453a05b73c:	-0.05%
Covered Lines:	9824
Relevant Lines:	13194

💛 - Coveralls

Manishearth · 2021-07-12T23:12:24Z

I added a #[data_struct] attribute, this now fully fixes #761!

jira-pull-request-webhook · 2021-07-12T23:33:18Z

Notice: the branch changed across the force-push!

components/datetime/Cargo.toml is now changed in the branch
components/locale_canonicalizer/Cargo.toml is now changed in the branch
components/plurals/Cargo.toml is now changed in the branch
components/uniset/Cargo.toml is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

Manishearth · 2021-07-12T23:34:17Z

oops, shouldn't have force-pushed. but it's only on the latest commits

components/plurals/src/provider.rs

sffc

Great work!

Manishearth added 2 commits July 2, 2021 14:29

Add yoke-derive

3c1c16a

Use yoke derive in icu4x

983cb9c

Manishearth requested review from echeran, sffc, zbraniecki and a team as code owners July 2, 2021 21:43

Satisfy clippy

3ef9a0a

sffc reviewed Jul 2, 2021

View reviewed changes

Manishearth added 6 commits July 2, 2021 16:20

Improve ZeroMap yoke impl, add zerovec derive tests

c85a3ea

CloningZCF -> cloning_zcf

e9ff874

unsafe_impl_data_marker_with_lifetime -> impl_data_marker_with_lifetime

d04b17b

Add ZCF impls for ZV

aa50fdd

fix sentence fragment

2d24796

Remove extra yoke deps

570f60f

Manishearth requested a review from sffc July 3, 2021 00:07

sffc reviewed Jul 4, 2021

View reviewed changes

Manishearth added 3 commits July 4, 2021 08:59

fix bounds formatting

d3c5c11

rm yoke::

eebb240

Fix test failure

65c7699

Manishearth requested a review from sffc July 4, 2021 16:18

Manishearth mentioned this pull request Jul 9, 2021

Add .take() function to Yoke #731

Closed

Add #[data_struct] custom derive

101967b

Manishearth added 2 commits July 12, 2021 16:33

Use #[data_struct] everywhere

973eb49

UnicodePropertyMarker -> UnicodePropertyV1Marker

672862b

Manishearth force-pushed the yoke-derive branch from 2fe2648 to 672862b Compare July 12, 2021 23:33

sffc mentioned this pull request Jul 13, 2021

Migrate remaining data structs to fully borrowed #856

Closed

sffc reviewed Jul 13, 2021

View reviewed changes

components/plurals/src/provider.rs Outdated Show resolved Hide resolved

Fix generated docs

3ebb115

Manishearth requested a review from sffc July 13, 2021 17:47

sffc approved these changes Jul 13, 2021

View reviewed changes

Manishearth merged commit 56c71b7 into unicode-org:main Jul 13, 2021

Manishearth deleted the yoke-derive branch July 13, 2021 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom derive for Yokeable and ZCF #847

Add custom derive for Yokeable and ZCF #847

Manishearth commented Jul 2, 2021 •

edited

Loading

sffc left a comment

sffc Jul 2, 2021

Manishearth Jul 2, 2021

sffc Jul 2, 2021

Manishearth Jul 2, 2021

sffc Jul 4, 2021

Manishearth Jul 4, 2021

sffc Jul 4, 2021

Manishearth Jul 4, 2021 •

edited

Loading

sffc commented Jul 4, 2021

Manishearth commented Jul 4, 2021

codecov-commenter commented Jul 4, 2021 •

edited

Loading

coveralls commented Jul 4, 2021 •

edited

Loading

Manishearth commented Jul 12, 2021

jira-pull-request-webhook bot commented Jul 12, 2021

Manishearth commented Jul 12, 2021

sffc left a comment

		// This is for when we're implementing Yoke on a complex type such that it's not
		// obvious to the compiler that the lifetime is covariant

Add custom derive for Yokeable and ZCF #847

Add custom derive for Yokeable and ZCF #847

Conversation

Manishearth commented Jul 2, 2021 • edited Loading

sffc left a comment

Choose a reason for hiding this comment

sffc Jul 2, 2021

Choose a reason for hiding this comment

Manishearth Jul 2, 2021

Choose a reason for hiding this comment

sffc Jul 2, 2021

Choose a reason for hiding this comment

Manishearth Jul 2, 2021

Choose a reason for hiding this comment

sffc Jul 4, 2021

Choose a reason for hiding this comment

Manishearth Jul 4, 2021

Choose a reason for hiding this comment

sffc Jul 4, 2021

Choose a reason for hiding this comment

Manishearth Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

sffc commented Jul 4, 2021

Manishearth commented Jul 4, 2021

codecov-commenter commented Jul 4, 2021 • edited Loading

Codecov Report

coveralls commented Jul 4, 2021 • edited Loading

Pull Request Test Coverage Report for Build f665f1b40009f0ba250aceb427d4d5806476a75f-PR-847

💛 - Coveralls

Manishearth commented Jul 12, 2021

jira-pull-request-webhook bot commented Jul 12, 2021

Manishearth commented Jul 12, 2021

sffc left a comment

Choose a reason for hiding this comment

Manishearth commented Jul 2, 2021 •

edited

Loading

Manishearth Jul 4, 2021 •

edited

Loading

codecov-commenter commented Jul 4, 2021 •

edited

Loading

coveralls commented Jul 4, 2021 •

edited

Loading