Use ZeroVec in UnicodeSet #922

echeran · 2021-07-30T04:44:17Z

Closes #855.

echeran · 2021-08-02T03:45:37Z

components/uniset/src/uniset.rs

    // TODO: need advice - how should we remove Hash and Eq from UnicodeSet unless we need it?

    // If we wanted to use an array to keep the memory on the stack, there is an unsafe nightly feature
    // https://doc.rust-lang.org/nightly/core/array/trait.FixedSizeArray.html
    // Allows for traits of fixed size arrays

    // Implements an [inversion list.](https://en.wikipedia.org/wiki/Inversion_list)
-    inv_list: Vec<u32>,
+    inv_list: ZeroVec<'d, u32>,


How do I solve the following compiler error(s)?:

error[E0277]: the trait bound `ZeroVec<'_, u32>: Hash` is not satisfied --> components/uniset/src/uniset.rs:36:5 | 36 | inv_list: ZeroVec<'d, u32>, | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Hash` is not implemented for `ZeroVec<'_, u32>` | ::: /home/elango/.rustup/toolchains/1.51.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/hash/mod.rs:168:13 | 168 | fn hash<H: Hasher>(&self, state: &mut H); | - required by this bound in `core::hash::Hash::hash` | = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info) error[E0277]: the trait bound `ZeroVec<'d, u32>: Eq` is not satisfied --> components/uniset/src/uniset.rs:36:5 | 36 | inv_list: ZeroVec<'d, u32>, | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Eq` is not implemented for `ZeroVec<'d, u32>` | ::: /home/elango/.rustup/toolchains/1.51.0-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/cmp.rs:297:31

Hash:

Easiest would be to stop implementing Hash on UnicodeSet. I remember that was only used by some test code or something. It never really made sense to me.

Otherwise, you could look into adding a Hash impl on ZeroVec. It's probably not too hard.

Eq:

Seems like we need an Eq impl on ZeroVec. You could make a separate PR to add it.

echeran · 2021-08-02T03:56:44Z

components/uniset/src/uniset.rs

@@ -89,6 +90,7 @@ impl UnicodeSet {
    pub fn from_inversion_list(inv_list: Vec<u32>) -> Result<Self, UnicodeSetError> {
        if is_valid(&inv_list) {
            let size: usize = inv_list.chunks(2).map(|end_points| end_points[1] - end_points[0]).sum::<u32>() as usize;
+            // TODO: how to convert a `Vec<u32>` into ZeroVec?


This function from_inversion_list takes an inversion list of Vec<u32> and needs to turn that into a ZeroVec inversion list when constructing the UnicodeSet.

But my understanding is that ZeroVec is designed for taking a byte array and converting it via allocation to an aligned array/vector of other types/widths only at the time it is needed. When the input is a non-byte slice, then an allocation is required to convert it into the backing byte array.

So is there a way to convert the Vec<u32> into the &[u8] without allocations?

You should change the function to take ZeroVec instead of Vec.

If desired, you could keep these as two separate functions.

echeran · 2021-08-02T04:09:00Z

I could also use feedback on all things related to lifetime parameters here: whether I added lifetime parameters on the correct structs, in the right places, got the syntax right, etc.

components/uniset/src/builder.rs

components/uniset/src/conversions.rs

components/uniset/src/props.rs

components/uniset/src/provider.rs

components/uniset/src/uniset.rs

sffc · 2021-08-02T17:55:53Z

components/uniset/src/uniset.rs

    // TODO: need advice - how should we remove Hash and Eq from UnicodeSet unless we need it?

    // If we wanted to use an array to keep the memory on the stack, there is an unsafe nightly feature
    // https://doc.rust-lang.org/nightly/core/array/trait.FixedSizeArray.html
    // Allows for traits of fixed size arrays

    // Implements an [inversion list.](https://en.wikipedia.org/wiki/Inversion_list)
-    inv_list: Vec<u32>,
+    inv_list: ZeroVec<'d, u32>,


Hash:

Easiest would be to stop implementing Hash on UnicodeSet. I remember that was only used by some test code or something. It never really made sense to me.

Otherwise, you could look into adding a Hash impl on ZeroVec. It's probably not too hard.

Eq:

Seems like we need an Eq impl on ZeroVec. You could make a separate PR to add it.

components/uniset/src/uniset.rs

sffc · 2021-08-02T17:58:19Z

components/uniset/src/uniset.rs

@@ -89,6 +90,7 @@ impl UnicodeSet {
    pub fn from_inversion_list(inv_list: Vec<u32>) -> Result<Self, UnicodeSetError> {
        if is_valid(&inv_list) {
            let size: usize = inv_list.chunks(2).map(|end_points| end_points[1] - end_points[0]).sum::<u32>() as usize;
+            // TODO: how to convert a `Vec<u32>` into ZeroVec?


You should change the function to take ZeroVec instead of Vec.

If desired, you could keep these as two separate functions.

components/uniset/src/uniset.rs

sffc

(setting review bit)

Manishearth · 2021-08-10T20:48:09Z

components/uniset/src/builder.rs

@@ -16,15 +16,15 @@ pub struct UnicodeSetBuilder {
    intervals: Vec<u32>,
 }

-impl UnicodeSetBuilder {
+impl<'d> UnicodeSetBuilder {


is this 'd doing anything?

Done. No, it's not doing anything -- now removed.

Manishearth · 2021-08-11T07:14:34Z

This commit manages to fix the crash, fwiw

commit 6cfb09f329bbe0b5bc597e7cb0379ea62ae3227f
Author: Manish Goregaokar <[email protected]>
Date:   Wed Aug 11 00:12:40 2021 -0700

    work around rust bug

diff --git a/components/uniset/src/lib.rs b/components/uniset/src/lib.rs
index 92b24de7..52dc0579 100644
--- a/components/uniset/src/lib.rs
+++ b/components/uniset/src/lib.rs
@@ -51,6 +51,10 @@
 
 #![cfg_attr(not(any(test, feature = "std")), no_std)]
 
+// Workaround for https://github.com/rust-lang/rust/issues/87932
+#[cfg(feature = "serde")]
+extern crate serde;
+
 extern crate alloc;
 
 #[macro_use]
diff --git a/components/uniset/src/utils.rs b/components/uniset/src/utils.rs
index 80ef039c..aea83a3e 100644
--- a/components/uniset/src/utils.rs
+++ b/components/uniset/src/utils.rs
@@ -7,7 +7,7 @@ use core::{
     ops::{Bound::*, RangeBounds},
 };
 use zerovec::ZeroVec;
-// use zerovec::ule::AsULE;
+use zerovec::ule::AsULE;
 
 /// Returns whether the vector is sorted ascending non inclusive, of even length,
 /// and within the bounds of `0x0 -> 0x10FFFF` inclusive.

echeran · 2021-09-14T17:41:04Z

I also am not sure why the #[displaydoc("...")] annotations on the UnicodeSetError enum variants are triggering a missing docs clippy error. I can always duplicate the doc string argument as the Rust doc string. I see #[displaydoc()] elsewhere in the codebase that doesn't do that, though. How should best address the error?

sffc

Not done reviewing, but here are some comments to get started

Cargo.toml

components/uniset/examples/unicode_bmp_blocks_selector.rs

components/uniset/src/lib.rs

components/uniset/src/provider.rs

components/uniset/src/uniset.rs

iainireland

Picking a few nits, but generally this looks pretty good to me.

iainireland · 2021-09-14T16:44:13Z

Cargo.toml

@@ -16,8 +16,8 @@ members = [
    "components/plurals",
    "components/uniset",
    "experimental/bies",
+    # "experimental/calendar",


Nit: did you mean to add this line?

Removed, thanks. I didn't realize that it was an artifact of merged from latest on main.

iainireland · 2021-09-14T16:48:29Z

components/uniset/examples/unicode_bmp_blocks_selector.rs

@@ -18,13 +18,13 @@ icu_benchmark_macros::static_setup!();

 use icu_uniset::{UnicodeSet, UnicodeSetBuilder};

-fn get_basic_latin_block() -> UnicodeSet {
+fn get_basic_latin_block<'data>() -> UnicodeSet<'data> {


Nit: should this be fn get_basic_latin_block() -> UnicodeSet<'static>?

iainireland · 2021-09-14T16:55:42Z

components/uniset/src/lib.rs


 /// Custom Errors for [`UnicodeSet`].
 #[derive(Display, Debug)]
-#[allow(missing_docs)] // TODO(#1030) - Add missing docs.


Did you intend to remove this attribute?

Unremoved, thanks. That answers the question I had in the PR-level comment.

iainireland · 2021-09-14T16:56:47Z

components/uniset/src/props.rs


 // helper fn
-fn get_prop<'data, D>(ppucd_provider: &D, resc_key: ResourceKey) -> UnisetResult
+fn get_prop<'data, D>(provider: &'data D, resc_key: ResourceKey) -> UnisetResult<'data>


Good catch on changing the name here.

iainireland · 2021-09-14T17:06:25Z

components/uniset/src/provider.rs

-#[cfg_attr(
-    feature = "provider_serde",
-    derive(serde::Serialize, serde::Deserialize)
-)]
 pub struct UnicodePropertyV1<'data> {
    pub name: Cow<'data, str>,


Is there still a purpose for the name field here? My understanding is that it was only used in the experimental ppucd provider.

You're right. Based on @sffc 's comment above, I'm not sure yet about unlinking ppucd_provider, and maybe it provides an alternate way of exercising the provider logic as a temporary measure?

I suggest we get rid of the field in a separate PR.

iainireland · 2021-09-14T17:24:13Z

components/uniset/src/utils.rs

+    // let v_iter = v.iter();
+    // let v_len = v.len();
+    // let v_last = v.last();
+    // is_iter_valid(v_iter, v_len, v_last)


Nit: should this be removed, or uncommented?

iainireland · 2021-09-14T17:47:09Z

components/uniset/src/uniset.rs

    }
 }

-impl UnicodeSet {
+impl<'data> Eq for UnicodeSet<'data> {}


For my own education: is there a reason we can't derive Eq here?

It looks like Eq is not implemented on ZeroVec. When I remove this line and put Eq into the derive for UnicodeSet, I got/get:

error[E0277]: the trait bound `usize: icu_provider::yoke::ZeroCopyFrom<_>` is not satisfied --> components/uniset/src/uniset.rs:22:45 | 22 | #[derive(Debug, PartialEq, Clone, Yokeable, ZeroCopyFrom)] | ^^^^^^^^^^^^ the trait `icu_provider::yoke::ZeroCopyFrom<_>` is not implemented for `usize` | = note: required by `zero_copy_from` = note: this error originates in the derive macro `ZeroCopyFrom` (in Nightly builds, run with -Z macro-backtrace for more info)

Can you implement Eq on ZeroVec (where T is Eq) then? Something like:

impl<T> Eq for ZeroVec<T> where T: Eq {} // or maybe just add #[derive(Eq)] on ZeroVec

iainireland · 2021-09-14T17:53:13Z

components/uniset/src/uniset.rs

+    /// Note: this function currently causes an allocation due to the requirements from
+    /// the constructors of the underlying [`ZeroVec`].
+    ///


Should we instead / also have a constructor along the lines of pub fn from_inversion_list<'a>(inv_list: &'a [u32]) -> Result<UnicodeSet<'a>, UnicodeSetError>?

It's a little awkward that ZeroVec doesn't give us direct access to a slice of u32, or else we could replace the previous constructor with a more general form.

Yeah, I think we should basically have 3 constructors:

ZeroVec<'data, u32>

&'data [u32]

&[u32] (requires cloning the data)

Note that technically option 2 is a subset of option 1, but I think it's common enough to warrant its own constructor.

iainireland · 2021-09-14T17:57:20Z

components/uniset/src/uniset.rs

-        let result: Vec<u32> = self
-            .as_inversion_list() // Only crate public, to not leak impl
-            .to_vec();
+        let result: Vec<u32> = self.as_inversion_list(); // Only crate public, to not leak impl


Verifying my understanding: we're keeping this private because ZeroVec can't expose a u32 slice, and ZeroVec can't expose a u32 slice because of endianness concerns?

and alignment concerns. yes.

iainireland · 2021-09-14T17:59:55Z

components/uniset/src/uniset.rs

+                if let Some(x) = self.inv_list.get(pos + 1) {
+                    (till) <= x
+                } else {
+                    // This code should be unreachable because:
+                    // Inversion list query should not return out of bounds index
+                    false
+                }
+            }


Nit: What do you think of till <= self.inv_list.get(pos + 1).expect("Inversion list query should be in bounds")?

I originally put in this change to avoid the possibility of a panic. @sffc -- should that be a concern here?

OTOH, the comment asserts that it should be unreachable (based on the binary search result from the inversion list query not giving an out-of-bounds index). There is some precedent -- we used an .unwrap() in the CPT code because we couldn't avoid it and there weren't doubts about the code (which came from ICU4C).

I do prefer what Elango has written over having unwraps in our code. However, I would also be okay with an unreachable!().

components/uniset/src/builder.rs

components/uniset/src/conversions.rs

utils/zerovec/src/zerovec/mod.rs

components/uniset/src/uniset.rs

components/uniset/src/builder.rs

components/uniset/src/uniset.rs

components/uniset/src/props.rs

…ned()

…erator in pairs

… ZeroVec

…ds alloc

… UnicodePropertyV1 Serde requires specifying the lifetime bounds on the struct that is borrowed, so that UnicodePropertyV1 provider struct can use the default serde derive when borrowing UnicodeSet as a field

…provider_serde feature enabled

components/uniset/src/uniset.rs

echeran added 6 commits July 29, 2021 21:42

Convert iterations on inversion list for ZeroVec

47e3f7b

Update lifetime parameters for conversions.rs

3c47293

Add some lifetime parameters for provider.rs

c74cb1a

Add lifetime parameters in props.rs

484f42a

Add lifetime parameter signature on impl block declarations

d63113f

Add question for review

93d67aa

echeran commented Aug 2, 2021

View reviewed changes

echeran requested a review from sffc August 2, 2021 04:09

sffc reviewed Aug 2, 2021

View reviewed changes

sffc requested changes Aug 2, 2021

View reviewed changes

echeran added 3 commits August 4, 2021 22:37

Apply some feedback changes

c8681b5

Apply more feedback changes

1c1c2f3

Apply more feedback changes for UnicodeSet API refactoring

70fbe51

Manishearth reviewed Aug 11, 2021

View reviewed changes

echeran added 7 commits August 26, 2021 18:43

Apply Manish's workaround for rust-lang/rust #87932

31f7340

Resolve compiler failures

bc77908

Fix types, configs after merge latest from main

ab7f442

More lifetime parameter adjustmnets

bd8caee

Make tests pass again

d848d65

Apply clippy and formatter changes, fix tests/benches

50376d6

Apply review feedback

a1a321c

echeran marked this pull request as ready for review September 14, 2021 16:26

echeran requested review from iainireland and a team as code owners September 14, 2021 16:26

echeran requested a review from sffc September 14, 2021 16:26

sffc requested changes Sep 14, 2021

View reviewed changes

iainireland reviewed Sep 14, 2021

View reviewed changes

echeran added 4 commits September 20, 2021 13:46

Return a Unicode<'static> for singleton sets based on const data

ef6d16b

Remove is_valid()

37447a7

Merge branch 'main' into uniset-zerovec

9db3dee

Do not allocate in contains_set()

3099d25

sffc requested changes Sep 20, 2021

View reviewed changes

echeran added 14 commits September 22, 2021 07:55

Use clone_from_slice() to be more readable than .from_slice().into_ow…

39934df

…ned()

Touch up ZeroVec impls of Eq and PartialEq

c1c7cc1

Change doc string example to better exemplify constructor usage

f39c812

Update doc string wording for UnicodeSet constructor methods

c1b45fb

Revert unreachable!() to debug_assert!()

b377a78

Avoid redundant clone in UnicodeSet::all() that returns static data

49de1ee

Move unit tests to examples for UnicodeSet::all() and ::bmp()

871025d

Use .as_slice().chunks(2) on ZeroVec instead of manually consuming it…

d85486e

…erator in pairs

Update more builder mutation method to iterate using .chunks(2)

e091d45

Also add the ?Sized bound for the other PartialEq impl type param for…

713caa7

… ZeroVec

Refactor complement_list to take iterator so that complement_set avoi…

d986579

…ds alloc

Minor code change for clarity

47c3060

Specify UnicodeSet lifetimes for serde to use default serde derive on…

74214be

… UnicodePropertyV1 Serde requires specifying the lifetime bounds on the struct that is borrowed, so that UnicodePropertyV1 provider struct can use the default serde derive when borrowing UnicodeSet as a field

Use cfg_attr to make the serde(borrow) attribute apply only with the …

4ccf292

…provider_serde feature enabled

Manishearth mentioned this pull request Sep 23, 2021

Move zerovec to no_std #1094

Merged

Merge branch 'main' into uniset-zerovec

3cd6e1b

sffc reviewed Sep 23, 2021

View reviewed changes

components/uniset/src/uniset.rs Show resolved Hide resolved

Fix doc string test markdown syntax

17d6559

sffc approved these changes Sep 23, 2021

View reviewed changes

echeran requested review from Manishearth and iainireland September 23, 2021 22:50

Manishearth approved these changes Sep 24, 2021

View reviewed changes

echeran merged commit 19e4a15 into unicode-org:main Sep 27, 2021

echeran deleted the uniset-zerovec branch September 27, 2021 14:40

echeran mentioned this pull request Sep 28, 2021

Move CodePointTrie to components/utils #1114

Merged

3 tasks

Use ZeroVec in UnicodeSet #922

Use ZeroVec in UnicodeSet #922

Uh oh!

Conversation

echeran commented Jul 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echeran commented Aug 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Aug 11, 2021

Uh oh!

echeran commented Sep 14, 2021

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iainireland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment