Skip to content
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.

Sign up for GitHub

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

ZeroTrie: add Bake, Yoke, and ZeroFrom impls #4021

Merged
merged 4 commits into from
Sep 12, 2023
Merged

ZeroTrie: add Bake, Yoke, and ZeroFrom impls #4021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b21e234
ZeroTrie: add Bake, Yoke, and ZeroFrom impls
sffc Sep 11, 2023
a1c7125
clippy
sffc Sep 11, 2023
1d502bb
Reference Rust issue
sffc Sep 11, 2023
09a7a8a
Add remaining two derive tests
sffc Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions experimental/zerotrie/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,12 @@ all-features = true
denylist = ["bench"]

[dependencies]
databake = { workspace = true, features = ["derive"], optional = true }
displaydoc = { version = "0.2.3", default-features = false }
litemap = { workspace = true, features = ["alloc"], optional = true }
serde = { version = "1.0", optional = true }
yoke = { workspace = true, features = ["derive"], optional = true }
zerofrom = { workspace = true, optional = true }
zerovec = { workspace = true, optional = true }

[dev-dependencies]
Expand All @@ -50,8 +53,11 @@ bench = false # This option is required for Benchmark CI
default = []
bench = ["litemap"]
alloc = []
databake = ["dep:databake", "zerovec?/databake"]
litemap = ["dep:litemap", "alloc"]
serde = ["dep:serde", "dep:litemap", "alloc", "litemap/serde", "zerovec?/serde"]
yoke = ["dep:yoke"]
zerofrom = ["dep:zerofrom"]

[[bench]]
name = "overview"
Expand Down
60 changes: 57 additions & 3 deletions experimental/zerotrie/src/zerotrie.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ use litemap::LiteMap;
/// # Ok::<_, zerotrie::ZeroTrieError>(())
/// ```
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
// Note: The absence of the following derive does not cause any test failures in this crate
robertbastian marked this conversation as resolved.
Show resolved Hide resolved
#[cfg_attr(feature = "yoke", derive(yoke::Yokeable))]
pub struct ZeroTrie<Store>(pub(crate) ZeroTrieFlavor<Store>);

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
Expand Down Expand Up @@ -96,8 +98,10 @@ pub(crate) enum ZeroTrieFlavor<Store> {
/// ```
#[repr(transparent)]
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
#[cfg_attr(feature = "databake", derive(databake::Bake), databake(path = zerotrie))]
pub struct ZeroTrieSimpleAscii<Store: ?Sized> {
pub(crate) store: Store,
#[doc(hidden)] // for databake, but there are no invariants
pub store: Store,
}

/// A data structure that compactly maps from byte strings to integers.
Expand Down Expand Up @@ -126,17 +130,21 @@ pub struct ZeroTrieSimpleAscii<Store: ?Sized> {
/// ```
#[repr(transparent)]
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
#[cfg_attr(feature = "databake", derive(databake::Bake), databake(path = zerotrie))]
pub struct ZeroTriePerfectHash<Store: ?Sized> {
pub(crate) store: Store,
#[doc(hidden)] // for databake, but there are no invariants
pub store: Store,
}

/// A data structure that maps from a large number of byte strings to integers.
///
/// For more information, see [`ZeroTrie`].
#[repr(transparent)]
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
#[cfg_attr(feature = "databake", derive(databake::Bake), databake(path = zerotrie))]
pub struct ZeroTrieExtendedCapacity<Store: ?Sized> {
pub(crate) store: Store,
#[doc(hidden)] // for databake, but there are no invariants
pub store: Store,
}

macro_rules! impl_zerotrie_subtype {
Expand Down Expand Up @@ -474,6 +482,16 @@ macro_rules! impl_zerotrie_subtype {
core::mem::transmute(Store::from_byte_slice_unchecked(bytes))
}
}
#[cfg(feature = "zerofrom")]
impl<'zf, Store1, Store2> zerofrom::ZeroFrom<'zf, $name<Store1>> for $name<Store2>
where
Store2: zerofrom::ZeroFrom<'zf, Store1>,
{
#[inline]
fn zero_from(other: &'zf $name<Store1>) -> Self {
$name::from_store(zerofrom::ZeroFrom::zero_from(&other.store))
}
}
};
}

Expand Down Expand Up @@ -536,6 +554,19 @@ macro_rules! impl_dispatch {
ZeroTrieFlavor::ExtendedCapacity(subtype) => subtype.$inner_fn($arg),
}
};
(&$self:ident, $trait:ident::$inner_fn:ident()) => {
match &$self.0 {
ZeroTrieFlavor::SimpleAscii(subtype) => {
ZeroTrie(ZeroTrieFlavor::SimpleAscii($trait::$inner_fn(subtype)))
}
ZeroTrieFlavor::PerfectHash(subtype) => {
ZeroTrie(ZeroTrieFlavor::PerfectHash($trait::$inner_fn(subtype)))
}
ZeroTrieFlavor::ExtendedCapacity(subtype) => {
ZeroTrie(ZeroTrieFlavor::ExtendedCapacity($trait::$inner_fn(subtype)))
}
}
};
}

impl<Store> ZeroTrie<Store> {
Expand Down Expand Up @@ -625,3 +656,26 @@ where
Self::try_from_tuple_slice(byte_str_slice).unwrap()
}
}

#[cfg(feature = "databake")]
impl<Store> databake::Bake for ZeroTrie<Store>
where
Store: databake::Bake,
{
fn bake(&self, env: &databake::CrateEnv) -> databake::TokenStream {
use databake::*;
let inner = impl_dispatch!(&self, bake(env));
quote! { #inner.into_zerotrie() }
}
}

#[cfg(feature = "zerofrom")]
impl<'zf, Store1, Store2> zerofrom::ZeroFrom<'zf, ZeroTrie<Store1>> for ZeroTrie<Store2>
where
Store2: zerofrom::ZeroFrom<'zf, Store1>,
{
fn zero_from(other: &'zf ZeroTrie<Store1>) -> Self {
use zerofrom::ZeroFrom;
impl_dispatch!(&other, ZeroFrom::zero_from())
}
}
111 changes: 111 additions & 0 deletions experimental/zerotrie/tests/derive_test.rs
View file
Open in desktop
robertbastian marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
// This file is part of ICU4X. For terms of use, please see the file
// called LICENSE at the top level of the ICU4X source tree
// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).

#![allow(non_camel_case_types, non_snake_case)]

use zerotrie::ZeroTrie;
use zerotrie::ZeroTrieExtendedCapacity;
use zerotrie::ZeroTriePerfectHash;
use zerotrie::ZeroTrieSimpleAscii;
use zerovec::ZeroVec;

#[cfg_attr(feature = "yoke", derive(yoke::Yokeable))]
#[cfg_attr(feature = "zerofrom", derive(zerofrom::ZeroFrom))]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
#[cfg_attr(feature = "databake", derive(databake::Bake), databake(path = crate))]
struct DeriveTest_ZeroTrie_ZeroVec<'data> {
#[cfg_attr(feature = "serde", serde(borrow))]
_data: ZeroTrie<ZeroVec<'data, u8>>,
}

#[test]
#[cfg(all(feature = "databake", feature = "alloc"))]
fn bake_ZeroTrie_ZeroVec() {
use databake::*;
extern crate std;
test_bake!(
DeriveTest_ZeroTrie_ZeroVec<'static>,
crate::DeriveTest_ZeroTrie_ZeroVec {
Copy link
Member

Choose a reason for hiding this comment

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

wrapping the zerotrie in a struct doesn't add anything to the test, and it complicates it because it now also requires the struct's bake to be stable. I'd also prefer this as a unit test next to the code under test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please note that these tests are directly based on the ones in utils/zerovec/src/yoke_impls.rs

wrapping the zerotrie in a struct doesn't add anything to the test

It tests that the zerotrie can be used in a struct with derives. Without this test, it's possible that the imps are missing or incomplete. This was a problem in the past which is why we added these tests in the zerovec crate. I think we should err on the side of testing this behavior for any zero-copy data structure.

and it complicates it because it now also requires the struct's bake to be stable

Why? If the bake changes, update the test.

I'd also prefer this as a unit test next to the code under test.

This doesn't catch problems like private fields. Given that this tests external behavior it seems better for this to be in an integration test.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Shane here, this is our standard pattern for testing derive bounds for generic stuff

_data: zerotrie::ZeroTrieSimpleAscii {
store: zerovec::ZeroVec::new(),
}
.into_zerotrie()
},
);
}

#[cfg_attr(feature = "yoke", derive(yoke::Yokeable))]
#[cfg_attr(feature = "zerofrom", derive(zerofrom::ZeroFrom))]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
#[cfg_attr(feature = "databake", derive(databake::Bake), databake(path = crate))]
struct DeriveTest_ZeroTrieSimpleAscii_ZeroVec<'data> {
#[cfg_attr(feature = "serde", serde(borrow))]
_data: ZeroTrieSimpleAscii<ZeroVec<'data, u8>>,
}

#[test]
#[ignore] // https://github.com/rust-lang/rust/issues/98906
#[cfg(all(feature = "databake", feature = "alloc"))]
fn bake_ZeroTrieSimpleAscii_ZeroVec() {
use databake::*;
extern crate std;
test_bake!(
DeriveTest_ZeroTrieSimpleAscii_ZeroVec<'static>,
crate::DeriveTest_ZeroTrieSimpleAscii_ZeroVec {
_data: zerotrie::ZeroTrieSimpleAscii {
store: zerovec::ZeroVec::new(),
}
},
);
}

#[cfg_attr(feature = "yoke", derive(yoke::Yokeable))]
#[cfg_attr(feature = "zerofrom", derive(zerofrom::ZeroFrom))]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
#[cfg_attr(feature = "databake", derive(databake::Bake), databake(path = crate))]
struct DeriveTest_ZeroTriePerfectHash_ZeroVec<'data> {
#[cfg_attr(feature = "serde", serde(borrow))]
_data: ZeroTriePerfectHash<ZeroVec<'data, u8>>,
}

#[test]
#[ignore] // https://github.com/rust-lang/rust/issues/98906
#[cfg(all(feature = "databake", feature = "alloc"))]
fn bake_ZeroTriePerfectHash_ZeroVec() {
use databake::*;
extern crate std;
test_bake!(
DeriveTest_ZeroTriePerfectHash_ZeroVec<'static>,
crate::DeriveTest_ZeroTriePerfectHash_ZeroVec {
_data: zerotrie::ZeroTriePerfectHash {
store: zerovec::ZeroVec::new(),
}
},
);
}

#[cfg_attr(feature = "yoke", derive(yoke::Yokeable))]
#[cfg_attr(feature = "zerofrom", derive(zerofrom::ZeroFrom))]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
#[cfg_attr(feature = "databake", derive(databake::Bake), databake(path = crate))]
struct DeriveTest_ZeroTrieExtendedCapacity_ZeroVec<'data> {
#[cfg_attr(feature = "serde", serde(borrow))]
_data: ZeroTrieExtendedCapacity<ZeroVec<'data, u8>>,
}

#[test]
#[ignore] // https://github.com/rust-lang/rust/issues/98906
#[cfg(all(feature = "databake", feature = "alloc"))]
fn bake_ZeroTrieExtendedCapacity_ZeroVec() {
use databake::*;
extern crate std;
test_bake!(
DeriveTest_ZeroTrieExtendedCapacity_ZeroVec<'static>,
crate::DeriveTest_ZeroTrieExtendedCapacity_ZeroVec {
_data: zerotrie::ZeroTrieExtendedCapacity {
store: zerovec::ZeroVec::new(),
}
},
);
}