archive derive of PartialEq for rkyv - 0.4.x #959

mkatychev · 2023-02-10T17:49:47Z

This PR resolves PartialEq compilation errors for types that derive rkyv::Archive:

error[E0277]: can't compare `chrono::datetime::ArchivedDateTime<Utc>` with `DateTime<Utc>`
   --> common/src/transaction.rs:142:5
    |
142 |     rkyv::Archive,
    |     ^^^^^^^^^^^^^ no implementation for `chrono::datetime::ArchivedDateTime<Utc> == DateTime<Utc>`
    |
    = help: the trait `PartialEq<DateTime<Utc>>` is not implemented for `chrono::datetime::ArchivedDateTime<Utc>`
    = help: see issue #48214
    = note: this error originates in the derive macro `rkyv::Archive` (in Nightly builds, run with -Z macro-backtrace for more info)

This PR does not address #762 since a new archived object is created, for example ArchivedDateTime<Utc> is derived from DateTime<Utc>, but does preserve feature inclusivity.

I don't think this change merits tests as the object created is not directly accessible for comparison as shown in the rkyv basic example but happy to discuss.

esheppa · 2023-02-12T11:07:37Z

Thanks for this @mkatychev - sorry to be a pain but do you mind doing a git rebase -i HEAD~1 or similar so that this gets a new commit hash and the CI runs (I think the change from main to 0.4.x is currently preventing it running)

mkatychev · 2023-02-12T21:45:27Z

@esheppa After testing, I had to introduce a couple of changes to enable safe deserlialization.
Using the README example from the rkyv repository as reference, it seems bytecheck::CheckBytes has to be derived as well if one wants to avoid using unsafe:

// You can use the safe API for fast zero-copy deserialization
let archived = rkyv::check_archived_root::<Test>(&bytes[..]).unwrap();
assert_eq!(archived, &value);

// Or you can use the unsafe API for maximum performance
let archived = unsafe { rkyv::archived_root::<Test>(&bytes[..]) };
assert_eq!(archived, &value);

This seems like a core part of the way rkyv should work so I added a PR upstream to re-export bytecheck: rkyv/rkyv#340.

In the meantime however, it made the most sense to add bytecheck as an optional dependency to chrono while the PR above is still under review.

mkatychev · 2023-02-13T23:40:03Z

chrono/.github/workflows/test.yml

Lines 52 to 56 in d37b0be

    
               toolchain: 1.38.0 
        
           - uses: Swatinem/rust-cache@v1 
        
           # run --lib and --doc to avoid the long running integration tests which are run elsewhere 
        
           - run: cargo test --lib --features unstable-locales,wasmbind,oldtime,clock,rustc-serialize,serde,winapi --color=always -- --color=always 
        
           - run: cargo test --doc --features unstable-locales,wasmbind,oldtime,clock,rustc-serialize,serde,winapi --color=always -- --color=always

The dep:feature is supported in rust 1.60 and upwards. Is there any guidelines for how far behind the msrv should be?
Happy to revert to two separate optional dependencies and use #[cfg_attr(all(feature = "bytecheck", feature = "rkyv"), archive_attr(derive(CheckBytes)))]

djc · 2023-02-14T09:11:12Z

For the 0.4.x branch, we're sticking with 1.38 for now, so the two optional deps would be the way forward for now. Thanks!

Cargo.toml

jtmoon79

I visually reviewed. LGTM. However, I know very little about serde internals so I won't be able to spot serde-specific bugs.

Does this change need new test cases?
For example, I see the rkyv project implements a some test macros like pub fn test_archive<T>(value: &T) https://github.com/rkyv/rkyv/blob/master/rkyv_test/src/util.rs#L4
and test cases using those macros like fn archive_containers() https://github.com/rkyv/rkyv/blob/master/rkyv_test/src/test_alloc.rs#L36

Should the definitions here have test cases that exercise the rkyv features?

mkatychev · 2023-04-05T23:19:50Z

I visually reviewed. LGTM. However, I know very little about serde internals so I won't be able to spot serde-specific bugs.

Does this change need new test cases? For example, I see the rkyv project implements a some test macros like pub fn test_archive<T>(value: &T) https://github.com/rkyv/rkyv/blob/master/rkyv_test/src/util.rs#L4 and test cases using those macros like fn archive_containers() https://github.com/rkyv/rkyv/blob/master/rkyv_test/src/test_alloc.rs#L36

Should the definitions here have test cases that exercise the rkyv features?

I did a bit of upstream work in bytecheck and rkyv that I have to reflect in this PR. As such I've moved this to draft for when I get around to backporting the changes.

mkatychev · 2023-04-13T21:42:05Z

@jtmoon79 I'm going to wait for rkyv to release 0.8 as that allows bytecheck errors to implement Send + Sync before marking this ready for review.

I know very little about serde internals so I won't be able to spot serde-specific bugs

So the derive(Serialize, Deserialize) mentioned above are actually imported as rkyv::Serialize and rkyv::Deserialize respectively rather than serde::Serialize and serde::Deserialize.
There's some info on how that's done here.
rkyv is specifically used to allow a T::to_bytes()? and T::from_bytes(bytes)? implementation rather than as an API library that would allow serialization to various formats (such as serde) which compile time bounds checking.

Without the validation feature flag, the current use of rkyv in chrono is limited to unsafe blocks. By adding #[archive(check_bytes)] to the various chrono structs already using #[derive(rkyv::Archive)] these structs can be used in safe code as well.

Does this change need new test cases?

Other than the codeblock here:

chrono/src/date.rs

Lines 553 to 566 in 5658c49

    
           // This boilerplate is a shortcoming of the `Derive` macro. 
        
           // See: https://github.com/rkyv/rkyv/issues/333 
        
           #[cfg(feature = "rkyv-validation")] 
        
           impl<Tz: TimeZone> fmt::Debug for ArchivedDate<Tz> 
        
           where 
        
               Tz: Archive, 
        
               <Tz as Archive>::Archived: fmt::Debug, 
        
               <<Tz as TimeZone>::Offset as Archive>::Archived: fmt::Debug, 
        
               <Tz as TimeZone>::Offset: fmt::Debug + Archive, 
        
           { 
        
               fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { 
        
                   f.debug_struct("Date").field("date", &self.date).field("offset", &self.offset).finish() 
        
               } 
        
           }

This PR doesn't really introduce any rust code since these derives will fail at compile time. The most you'd have to do is see if cargo check --features rkyv-validation fails (which is how I was testing for regressions during this PR).

This line covers --features rkyv:

chrono/.github/workflows/test.yml

Line 81 in 5658c49

    
           - run: cargo hack check --feature-powerset --optional-deps serde,rkyv --skip default --skip __internal_bench --skip __doctest --skip iana-time-zone --skip pure-rust-locales

I'll be sure to add ,rkyv-validation to make sure this gets tested as well.

Roms1383 · 2023-05-26T18:07:34Z

would you mind adding PartialOrd too ?
inner types support it (tried locally then I realized a PR was already opened).

mkatychev · 2023-06-30T16:23:24Z

@jtmoon79 cargo hack check --feature-powerset should already be covering rkyv-validation feature flag

pitdicker · 2023-07-05T05:28:15Z

@mkatychev I have yet to go and read up about rkyv. But can I ask for your opinion/review of #762?

mkatychev · 2023-07-07T17:35:16Z

@pitdicker I'll post my thoughts in this PR, I'm using this current feature branch in my code, and this section does not impact me:

This new type won't have any traits or methods implemented on it except Deserialize, so it can't be easily used in a zero-copy manner

As long as the rkyv::validation::validators::from_bytes function is used, I don't interact with the intermediate type at all.

rkyv::validation::validators::from_bytes is used with by the derive(rkyv::Archive), archive(check_bytes) combination introduced in this PR so I don't have to worry about le/be.

In essence, the rkyv-validation feature flag bypasses a lot of the motivations of not dealing with an intermediate type.

mkatychev · 2023-09-18T15:24:23Z

@pitdicker what else is needed to move this review forward?

codecov · 2023-09-18T15:28:37Z

Codecov Report

Merging #959 (a5500af) into 0.4.x (0f19d6b) will increase coverage by 0.04%.
The diff coverage is 86.41%.

@@            Coverage Diff             @@
##            0.4.x     #959      +/-   ##
==========================================
+ Coverage   91.55%   91.60%   +0.04%     
==========================================
  Files          38       38              
  Lines       17355    17430      +75     
==========================================
+ Hits        15889    15966      +77     
+ Misses       1466     1464       -2

Files	Coverage Δ
src/duration.rs	`93.22% <100.00%> (+0.29%)`	⬆️
src/format/scan.rs	`99.25% <100.00%> (ø)`
src/month.rs	`75.96% <100.00%> (+0.96%)`	⬆️
src/naive/date.rs	`96.26% <100.00%> (+0.01%)`	⬆️
src/naive/datetime/mod.rs	`97.73% <ø> (+0.08%)`	⬆️
src/naive/datetime/tests.rs	`98.89% <100.00%> (+0.02%)`	⬆️
src/naive/isoweek.rs	`99.15% <100.00%> (+0.98%)`	⬆️
src/naive/time/mod.rs	`96.67% <ø> (+0.11%)`	⬆️
src/naive/time/tests.rs	`98.48% <100.00%> (+0.04%)`	⬆️
src/date.rs	`35.86% <0.00%> (ø)`
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pitdicker · 2023-09-18T15:55:46Z

Thank you for your work!

@pitdicker what else is needed to move this review forward?

Me needing to get a little experience with rkyv, because I don't understand it yet 😄. Or a review from @djc.

pitdicker

What I am missing with the rkyv feature is that we don't have any tests. That is not a fault of this PR, but can you please add some?

I don't think it needs to be more than a check that all types round-trip with rkyv::{to_bytes, from_bytes}.
And maybe include a check that the derived types implement PartialOrd, PartialEq ~~and Debug similar to https://github.com/chronotope/chrono/blob/v0.4.31/src/datetime/tests.rs#L1132.~~ Edit: Better to check (of course) that comparison works against the original type.
That should also make coverage happy.

And can you squash the commits?

Cargo.toml

src/date.rs

src/datetime/mod.rs

Cargo.toml

pitdicker · 2023-09-27T12:55:53Z

I'll incorporate the comments and add tests later this week.

@mkatychev No hurry, just a reminder.

mkatychev · 2023-09-27T22:12:03Z

@pitdicker apologies for delay, I will start work on this tomorrow.

mkatychev · 2023-09-28T23:21:37Z

src/offset/utc.rs

-#[derive(Copy, Clone, PartialEq, Eq, Hash)]
-#[cfg_attr(feature = "rkyv", derive(Archive, Deserialize, Serialize))]
-#[cfg_attr(feature = "rkyv", archive_attr(derive(Clone, Copy, PartialEq, Eq, Debug, Hash)))]
+#[derive(Clone, Copy, Eq, Hash, PartialEq)]


I sorted the derives alphabetically to better compare against the archive_attr equivalents.

mkatychev · 2023-09-29T16:13:22Z

@pitdicker this should be ready for review again

mkatychev · 2023-09-29T17:20:01Z

Tagging related issues for ArchivedDateTime:
rkyv/rkyv#333
rust-lang/rust#26925

pitdicker

Looking good 😄. A few comments, and we have trouble with our MSRV.

Can you squash the commits?

pitdicker · 2023-09-29T18:15:37Z

Cargo.toml

 __internal_bench = []

 [dependencies]
 num-traits = { version = "0.2", default-features = false }
 rustc-serialize = { version = "0.3.20", optional = true }
 serde = { version = "1.0.99", default-features = false, optional = true }
 pure-rust-locales = { version = "0.7", optional = true }
-rkyv = { version = "0.7", optional = true }
+rkyv = { version = "0.7.42", optional = true }


Suggested change

rkyv = { version = "0.7.42", optional = true }

rkyv = { version = "0.7.41", optional = true }

I hope that by setting 0.7.41 as the minimum version and with rkyv/bytecheck#33 that we can get things working with our MSRV.

I'm curious why 0.7.42 was not able to be found 🤔 🤔

src/duration.rs

src/month.rs

src/naive/isoweek.rs

src/date.rs

.github/workflows/test.yml

mkatychev · 2023-09-29T19:38:19Z

Thanks for pushing this through @pitdicker !

djc · 2023-10-02T07:49:49Z

This needs to be rebased and split into clean commits so I can review it. Right now there's too much going on when reviewing all the changes at once (notably reordering of derives while also adding/removing derives) to make this efficient to review.

mkatychev · 2023-10-03T20:41:53Z

@djc I'll rebase the commits later this week, I'll just skip the reordering altogether to make reviewing easier

* specified latest hotfix version of `rkyv` that includes `bytecheck` reexport

…ature flags * added `archive(check_bytes)` derives `rkyv-validation` feature flags

chrono month::tests::test_rkyv_validation chrono naive::date::tests::test_rkyv_validation chrono duration::tests::test_rkyv_validation chrono naive::datetime::tests::test_rkyv_validation chrono naive::isoweek::tests::test_rkyv_validation chrono naive::time::tests::test_rkyv_validation chrono offset::fixed::tests::test_rkyv_validation chrono offset::local::tests::test_rkyv_validation chrono weekday::tests::test_rkyv_validation

mkatychev · 2023-10-09T19:15:20Z

@djc This should be ready for review

djc · 2023-10-10T14:25:15Z

Thanks!

mkatychev changed the base branch from main to 0.4.x February 10, 2023 17:50

mkatychev changed the title ~~Feat/archive peq rkyv 0.4.x~~ archive derive of PartialEq for rkyv - 0.4.x backport Feb 10, 2023

mkatychev mentioned this pull request Feb 12, 2023

archive derive of PartialEq for rkyv #958

Closed

mkatychev force-pushed the feat/archive_peq_rkyv_0.4.x branch from d33f4d0 to 2a985a6 Compare February 12, 2023 21:56

mkatychev changed the title ~~archive derive of PartialEq for rkyv - 0.4.x backport~~ archive derive of PartialEq for rkyv - 0.4.x Feb 13, 2023

mkatychev mentioned this pull request Feb 13, 2023

reexport bytecheck::CheckBytes rkyv/rkyv#340

Merged

mkatychev force-pushed the feat/archive_peq_rkyv_0.4.x branch from 27ca8dd to d37b0be Compare February 13, 2023 20:53

mkatychev force-pushed the feat/archive_peq_rkyv_0.4.x branch from 9bf3815 to 6e46aa3 Compare February 15, 2023 20:33

djc reviewed Feb 16, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

jtmoon79 reviewed Apr 5, 2023

View reviewed changes

mkatychev marked this pull request as draft April 5, 2023 23:19

mkatychev marked this pull request as ready for review June 30, 2023 16:23

mkatychev requested review from jtmoon79 and djc June 30, 2023 16:23

pitdicker added the waiting-on-review label Jul 20, 2023

pitdicker requested changes Sep 18, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

src/date.rs Outdated Show resolved Hide resolved

src/datetime/mod.rs Show resolved Hide resolved

src/datetime/mod.rs Outdated Show resolved Hide resolved

pitdicker reviewed Sep 19, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

mkatychev marked this pull request as draft September 28, 2023 22:02

mkatychev commented Sep 28, 2023

View reviewed changes

mkatychev requested a review from pitdicker September 29, 2023 16:12

mkatychev marked this pull request as ready for review September 29, 2023 16:13

pitdicker mentioned this pull request Sep 29, 2023

Rust 1.57 compatibility rkyv/bytecheck#33

Merged

pitdicker requested changes Sep 29, 2023

View reviewed changes

mkatychev requested a review from pitdicker September 29, 2023 18:52

pitdicker approved these changes Sep 29, 2023

View reviewed changes

mkatychev added 5 commits October 9, 2023 13:44

* added rkyv-validation feature flag

e758ff9

* specified latest hotfix version of `rkyv` that includes `bytecheck` reexport

* added archive(compare(PartialEq/PartialOrd)) derives to rkyv fe…

3faca1b

…ature flags * added `archive(check_bytes)` derives `rkyv-validation` feature flags

hand implemented fmt::Debug for ArchivedDateTime

57c2fbd

removed rkyv from cargo hack check in CI based on feedback

4e1734e

mkatychev force-pushed the feat/archive_peq_rkyv_0.4.x branch from 4adeb52 to 4e1734e Compare October 9, 2023 19:14

clippy fix

a5500af

djc merged commit 02bdd1d into chronotope:0.4.x Oct 10, 2023

This was referenced Jan 24, 2024

Bump chrono from 0.4.31 to 0.4.32 ISibboI/typed-session#27

Closed

Bump chrono from 0.4.31 to 0.4.32 ISibboI/vocabulary-learning-application#115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

archive derive of PartialEq for rkyv - 0.4.x #959

archive derive of PartialEq for rkyv - 0.4.x #959

mkatychev commented Feb 10, 2023 •

edited

Loading

esheppa commented Feb 12, 2023

mkatychev commented Feb 12, 2023

mkatychev commented Feb 13, 2023 •

edited

Loading

djc commented Feb 14, 2023

jtmoon79 left a comment

mkatychev commented Apr 5, 2023

mkatychev commented Apr 13, 2023 •

edited

Loading

Roms1383 commented May 26, 2023

mkatychev commented Jun 30, 2023

pitdicker commented Jul 5, 2023

mkatychev commented Jul 7, 2023 •

edited

Loading

mkatychev commented Sep 18, 2023

codecov bot commented Sep 18, 2023 •

edited

Loading

pitdicker commented Sep 18, 2023

pitdicker left a comment •

edited

Loading

pitdicker commented Sep 27, 2023

mkatychev commented Sep 27, 2023

mkatychev Sep 28, 2023 •

edited

Loading

mkatychev commented Sep 29, 2023

mkatychev commented Sep 29, 2023

pitdicker left a comment

pitdicker Sep 29, 2023

mkatychev Sep 29, 2023

mkatychev commented Sep 29, 2023

djc commented Oct 2, 2023

mkatychev commented Oct 3, 2023

mkatychev commented Oct 9, 2023

djc commented Oct 10, 2023

	rkyv = { version = "0.7.42", optional = true }
	rkyv = { version = "0.7.41", optional = true }

archive derive of PartialEq for rkyv - 0.4.x #959

archive derive of PartialEq for rkyv - 0.4.x #959

Conversation

mkatychev commented Feb 10, 2023 • edited Loading

esheppa commented Feb 12, 2023

mkatychev commented Feb 12, 2023

mkatychev commented Feb 13, 2023 • edited Loading

djc commented Feb 14, 2023

jtmoon79 left a comment

Choose a reason for hiding this comment

mkatychev commented Apr 5, 2023

mkatychev commented Apr 13, 2023 • edited Loading

Roms1383 commented May 26, 2023

mkatychev commented Jun 30, 2023

pitdicker commented Jul 5, 2023

mkatychev commented Jul 7, 2023 • edited Loading

mkatychev commented Sep 18, 2023

codecov bot commented Sep 18, 2023 • edited Loading

Codecov Report

pitdicker commented Sep 18, 2023

pitdicker left a comment • edited Loading

Choose a reason for hiding this comment

pitdicker commented Sep 27, 2023

mkatychev commented Sep 27, 2023

mkatychev Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

mkatychev commented Sep 29, 2023

mkatychev commented Sep 29, 2023

pitdicker left a comment

Choose a reason for hiding this comment

pitdicker Sep 29, 2023

Choose a reason for hiding this comment

mkatychev Sep 29, 2023

Choose a reason for hiding this comment

mkatychev commented Sep 29, 2023

djc commented Oct 2, 2023

mkatychev commented Oct 3, 2023

mkatychev commented Oct 9, 2023

djc commented Oct 10, 2023

mkatychev commented Feb 10, 2023 •

edited

Loading

mkatychev commented Feb 13, 2023 •

edited

Loading

mkatychev commented Apr 13, 2023 •

edited

Loading

mkatychev commented Jul 7, 2023 •

edited

Loading

codecov bot commented Sep 18, 2023 •

edited

Loading

pitdicker left a comment •

edited

Loading

mkatychev Sep 28, 2023 •

edited

Loading