search

chanced / jsonptr

JSON Pointer (RFC 6901) implementation for Rust
Apache License 2.0
44 stars 4 forks source link

Improves CI, fixes `no_std` and other misc issues discovered #51

Closed chanced closed 1 month ago

chanced commented 1 month ago
chanced commented 1 month ago

@asmello i copied over jonhoo/rust-ci-conf and one of the checks that's failing is miri. Could you look at the output when you get a chance please?

chanced commented 1 month ago

There are two failing checks that I do not know how to resolve:

Miri

~/dev/jsonptr  improve-ci ✔                                               5m  ⍉
▶ cargo +nightly miri test
   Compiling jsonptr v0.5.0 (/Users/chance/dev/jsonptr)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.14s
     Running unittests src/lib.rs (target/miri/aarch64-apple-darwin/debug/deps/jsonptr-42b38b1574134774)

running 26 tests
test assign::tests::test_assign_json ... error: Undefined Behavior: trying to retag from <271676> for SharedReadOnly permission at alloc70916[0x4], but that tag does not exist in the borrow stack for this location
   --> /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/slice/raw.rs:140:9
    |
140 |         &*ptr::slice_from_raw_parts(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         trying to retag from <271676> for SharedReadOnly permission at alloc70916[0x4], but that tag does not exist in the borrow stack for this location
    |         this error occurs as part of retag at alloc70916[0x4..0x8]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <271676> was created by a SharedReadOnly retag at offsets [0x5..0x8]
   --> src/pointer.rs:860:15
    |
860 |     let ptr = s.as_ptr().offset(-1);
    |               ^^^^^^^^^^
    = note: BACKTRACE (of the first span) on thread `assign::tests::test_assign_json`:
    = note: inside `std::slice::from_raw_parts::<'_, u8>` at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/slice/raw.rs:140:9: 140:47
note: inside `pointer::extend_one_before`
   --> src/pointer.rs:862:17
    |
862 |     let slice = slice::from_raw_parts(ptr, len);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
   --> src/pointer.rs:189:41
    |
189 |                     let back = unsafe { extend_one_before(back) };
    |                                         ^^^^^^^^^^^^^^^^^^^^^^^
    = note: inside `std::option::Option::<(&str, &str)>::map_or_else::<(token::Token<'_>, &pointer::Pointer), {closure@src/pointer.rs:184:17: 184:19}, {closure@src/pointer.rs:185:17: 185:32}>` at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:1206:24: 1206:28
note: inside `pointer::Pointer::split_front`
   --> src/pointer.rs:181:9
    |
181 | /         self.0[1..]
182 | |             .split_once('/')
183 | |             .map_or_else(
184 | |                 || (Token::from_encoded_unchecked(&self.0[1..]), Self::root()),
...   |
191 | |                 },
192 | |             )
    | |_____________^
note: inside `assign::json::assign_value`
   --> src/assign.rs:251:41
    |
251 |         while let Some((token, tail)) = ptr.split_front() {
    |                                         ^^^^^^^^^^^^^^^^^
note: inside `assign::json::<impl assign::Assign for serde_json::Value>::assign::<serde_json::Value>`
   --> src/assign.rs:240:13
    |
240 |             assign_value(ptr, self, value.into())
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `pointer::Pointer::assign::<serde_json::Value, serde_json::Value>`
   --> src/pointer.rs:397:9
    |
397 |         dest.assign(self, src)
    |         ^^^^^^^^^^^^^^^^^^^^^^
note: inside `assign::tests::Test::<serde_json::Value>::run`
   --> src/assign.rs:584:28
    |
584 |             let replaced = ptr.assign(&mut data, assign.clone());
    |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
   --> src/assign.rs:572:61
    |
572 |             tests.into_iter().enumerate().for_each(|(i, t)| t.run(i));
    |                                                             ^^^^^^^^
    = note: inside closure at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:815:29: 815:36
    = note: inside closure at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/iter/adapters/enumerate.rs:108:27: 108:51
    = note: inside closure at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/array/iter.rs:271:13: 271:77
    = note: inside closure at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/try_trait.rs:392:39: 392:46
    = note: inside `<std::ops::index_range::IndexRange as std::iter::Iterator>::try_fold::<(), {closure@std::ops::try_trait::NeverShortCircuit<()>::wrap_mut_2<(), usize, {closure@<std::array::IntoIter<assign::tests::Test<serde_json::Value>, 19> as std::iter::Iterator>::fold<(), {closure@<std::iter::Enumerate<I> as std::iter::Iterator>::fold::enumerate<assign::tests::Test<serde_json::Value>, (), {closure@std::iter::Iterator::for_each::call<(usize, assign::tests::Test<serde_json::Value>), {closure@src/assign.rs:572:52: 572:60}>::{closure#0}}>::{closure#0}}>::{closure#0}}>::{closure#0}}, std::ops::try_trait::NeverShortCircuit<()>>` at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:2410:21: 2410:32
    = note: inside `<std::iter::ByRefSized<'_, std::ops::index_range::IndexRange> as std::iter::Iterator>::fold::<(), {closure@<std::array::IntoIter<assign::tests::Test<serde_json::Value>, 19> as std::iter::Iterator>::fold<(), {closure@<std::iter::Enumerate<I> as std::iter::Iterator>::fold::enumerate<assign::tests::Test<serde_json::Value>, (), {closure@std::iter::Iterator::for_each::call<(usize, assign::tests::Test<serde_json::Value>), {closure@src/assign.rs:572:52: 572:60}>::{closure#0}}>::{closure#0}}>::{closure#0}}>` at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/iter/adapters/by_ref_sized.rs:45:9: 45:68
    = note: inside `<std::array::IntoIter<assign::tests::Test<serde_json::Value>, 19> as std::iter::Iterator>::fold::<(), {closure@<std::iter::Enumerate<I> as std::iter::Iterator>::fold::enumerate<assign::tests::Test<serde_json::Value>, (), {closure@std::iter::Iterator::for_each::call<(usize, assign::tests::Test<serde_json::Value>), {closure@src/assign.rs:572:52: 572:60}>::{closure#0}}>::{closure#0}}>` at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/array/iter.rs:267:9: 272:11
    = note: inside `<std::iter::Enumerate<std::array::IntoIter<assign::tests::Test<serde_json::Value>, 19>> as std::iter::Iterator>::fold::<(), {closure@std::iter::Iterator::for_each::call<(usize, assign::tests::Test<serde_json::Value>), {closure@src/assign.rs:572:52: 572:60}>::{closure#0}}>` at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/iter/adapters/enumerate.rs:114:9: 114:58
    = note: inside `<std::iter::Enumerate<std::array::IntoIter<assign::tests::Test<serde_json::Value>, 19>> as std::iter::Iterator>::for_each::<{closure@src/assign.rs:572:52: 572:60}>` at /Users/chance/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:818:9: 818:31
note: inside `assign::tests::Test::<serde_json::Value>::all::<[assign::tests::Test<serde_json::Value>; 19]>`
   --> src/assign.rs:572:13
    |
572 |             tests.into_iter().enumerate().for_each(|(i, t)| t.run(i));
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `assign::tests::test_assign_json`
   --> src/assign.rs:604:9
    |
604 | /         Test::all([
605 | |             Test {
606 | |                 ptr: "/foo",
607 | |                 data: json!({}),
...   |
748 | |             },
749 | |         ]);
    | |__________^
note: inside closure
   --> src/assign.rs:601:26
    |
599 |     #[test]
    |     ------- in this procedural macro expansion
600 |     #[cfg(feature = "json")]
601 |     fn test_assign_json() {
    |                          ^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

cargo test

This one's weird. I don't encounter it locally.

cargo test --locked --all-features --all-targets
  shell: /usr/bin/bash -e {0}
  env:
    CARGO_HOME: /home/runner/.cargo
    CARGO_INCREMENTAL: 0
    CARGO_TERM_COLOR: always
 Downloading crates ...
  Downloaded hashbrown v0.14.0
  Downloaded quickcheck_macros v1.0.0
  Downloaded log v0.4.8
  Downloaded equivalent v1.0.0
  Downloaded thread_local v0.[3](https://github.com/chanced/jsonptr/actions/runs/9747282344/job/26899608999?pr=51#step:7:3).6
  Downloaded quote v1.0.0
  Downloaded toml_datetime v0.6.3
  Downloaded unicode-xid v0.2.0
  Downloaded ryu v1.0.0
  Downloaded winnow v0.5.0
  Downloaded regex v1.3.0
  Downloaded regex-syntax v0.6.12
  Downloaded libc v0.2.6[4](https://github.com/chanced/jsonptr/actions/runs/9747282344/job/26899608999?pr=51#step:7:4)
  Downloaded rand v0.8.0
  Downloaded memchr v2.[5](https://github.com/chanced/jsonptr/actions/runs/9747282344/job/26899608999?pr=51#step:7:5).0
  Downloaded indexmap v2.0.0
  Downloaded aho-corasick v0.7.[6](https://github.com/chanced/jsonptr/actions/runs/9747282344/job/26899608999?pr=51#step:7:6)
  Downloaded toml_edit v0.20.0
  Downloaded toml v0.8.0
  Downloaded syn v1.0.0
  Downloaded serde_json v1.0.45
  Downloaded serde v1.0.145
  Downloaded quickcheck v1.0.3
  Downloaded proc-macro2 v1.0.0
  Downloaded lazy_static v1.0.0
  Downloaded itoa v0.4.3
  Downloaded getrandom v0.2.0
  Downloaded env_logger v0.8.2
  Downloaded serde_spanned v0.6.3
  Downloaded rand_core v0.6.2
  Downloaded cfg-if v0.1.2
   Compiling serde v1.0.145
   Compiling memchr v2.5.0
   Compiling libc v0.2.64
   Compiling cfg-if v0.1.2
   Compiling getrandom v0.2.0
   Compiling proc-macro2 v1.0.0
   Compiling log v0.4.8
   Compiling lazy_static v1.0.0
   Compiling unicode-xid v0.2.0
   Compiling thread_local v0.3.6
   Compiling aho-corasick v0.[7](https://github.com/chanced/jsonptr/actions/runs/9747282344/job/26899608999?pr=51#step:7:7).6
   Compiling ryu v1.0.0
   Compiling equivalent v1.0.0
   Compiling syn v1.0.0
   Compiling hashbrown v0.14.0
   Compiling regex-syntax v0.6.12
   Compiling indexmap v2.0.0
   Compiling rand_core v0.6.2
   Compiling quote v1.0.0
   Compiling regex v1.3.0
   Compiling winnow v0.5.0
   Compiling serde_spanned v0.6.3
   Compiling toml_datetime v0.6.3
   Compiling env_logger v0.[8](https://github.com/chanced/jsonptr/actions/runs/9747282344/job/26899608999?pr=51#step:7:9).2
   Compiling toml_edit v0.20.0
   Compiling rand v0.8.0
   Compiling itoa v0.4.3
   Compiling serde_json v1.0.45
   Compiling toml v0.8.0
   Compiling quickcheck v1.0.3
   Compiling quickcheck_macros v1.0.0
error[E0308]: mismatched types
  --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba[15](https://github.com/chanced/jsonptr/actions/runs/9747282344/job/26899608999?pr=51#step:7:16)001f/quickcheck_macros-1.0.0/src/lib.rs:43:31
   |
43 |                     variadic: item_fn.sig.variadic.clone(),
   |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Option<Dot3>`, found `Option<Variadic>`
   |
   = note: expected enum `Option<Dot3>`
              found enum `Option<Variadic>`
asmello commented 1 month ago

Will look into the miri report, could be a legitimate find, could be a limitation of its unsafe lifetime checker.

As for the other one, I think it's because quickcheck has an issue with a transitive dependency, where their locked version doesn't match the one we resolve with the default version resolver. Will also give it a look but we may need to hack the Cargo.toml to workaround until they fix it upstream.

chanced commented 1 month ago

Second one was solved by specifying the most recent versions of serde (1.0.203) and serde_json (1.0.119).

chanced commented 1 month ago

Ah, I forgot I also switched to -Zdirect-minimal-versions

I'm guessing neither change (using most recent versions and not using -Zminimal-versions) are optimal, though.

asmello commented 1 month ago

Yeah the miri thing goes deep into Rust intrinsics. I have a few theories, having to do with how Rust (or miri) tracks borrows for a given memory span. I suspect the borrows are tagged at the start of the span, and by shifting the pointer we invalidate the tag that was tracking its lifetime. Not sure how something like get_unchecked causes a new tag to be pushed onto the stack — if it didn't, it'd run into a similar issue.

Probably the easiest solution is to avoid the pointer arithmetics. I have a few alternatives I want to try that should be just as performant, and lean more heavily on composition over std primitives, which should keep miri happy.

asmello commented 1 month ago

I'm guessing neither change (using most recent versions and not using -Zminimal-versions) are optimal, though.

I think using the most recent version is fine as a workaround, since we already have those as optional dependencies regardless. But for future reference, I believe the way to get around that error otherwise would be to add a section like this to the cargo manifest:

[target.'cfg(any())'.dependencies]
serde = { version = "x.y.z", default-features = false, optional = true }

The trick being that this forces -Zminimal-versions to account for the version we want, but doesn't affect the dependencies we actually build with (due to the target selection being the empty set).

EDIT: BTW we can probably switch back to -Zminimal-versions now.

chanced commented 1 month ago

Hah, I saw a comment by Jon in DOCS.md from his actions repo and assumed it was a typo. That is such a weird syntax.

It must not have been serde. I can't infer from the error output what dependency it may be. I'm not seeing anything jump out at me from cargo tree --frozen --edges all --verbose.

The only crate which would make sense to me at the moment is syn but syn only appears to be used by quickcheck_macros

edit: Also, miri keeps hanging now.

asmello commented 1 month ago

Miri is quite slow, we may need to tune it to only run for a subset of tests or not at all unfortunately...

Let me see if I can find out which dependency quickcheck is failing on (it's not the end of the world to use the direct variant if not).

chanced commented 1 month ago

Nah don’t worry about the dep. I’ll figure it out. You’ve got the unsafe bit which I can’t help with at all right now.

On Mon, Jul 1, 2024 at 5:20 PM André Mello @.***> wrote:

Miri is quite slow, we may need to tune it to only run for a subset of tests or not at all unfortunately...

Let me see if I can find out which dependency quickcheck is failing on (it's not the end of the world to use the direct variant if not).

— Reply to this email directly, view it on GitHub https://github.com/chanced/jsonptr/pull/51#issuecomment-2201092739, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIGFZCLFNGJ2AXAHZWDXLLZKHBY7AVCNFSM6AAAAABKETGMO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBRGA4TENZTHE . You are receiving this because you authored the thread.Message ID: @.***>

asmello commented 1 month ago

The only crate which would make sense to me at the moment is syn but syn only appears to be used by quickcheck_macros

I think that's exactly right, the CI error does point to that crate (sorry if I confused you by saying it was quickcheck, didn't look very closely...).

I think this is what we need:

syn = { version = "1.0.109", optional = true }
asmello commented 1 month ago

Yay, it worked! Also relevant: https://github.com/BurntSushi/quickcheck/pull/317

I guess little chance we can get the fix pushed upstream, but we can try... Should be a trivial patch version bump...

chanced commented 1 month ago

Yay, it worked! Also relevant: https://github.com/BurntSushi/quickcheck/pull/317

Rock on!

I don't understand why we have to be explicit here though. Why is the resolver introducing syn@2.0 when the only dependent is quickcheck_macros and it specifies 1.0:

syn = { version = "1.0", features = ["full"] }

Edit

I didn't mean to seem dubious about the solution. I'm stoked about it!

That was just my general confusion being expressed. Thanks for solving it.

chanced commented 1 month ago

I'm going to go ahead and merge this in.

asmello commented 1 month ago

I don't understand why we have to be explicit here though. Why is the resolver introducing syn@2.0 when the only dependent is quickcheck_macros and it specifies 1.0:

syn = { version = "1.0", features = ["full"] }

It's the other way around! The -Zminimal-dependencies resolver works the opposite of the normal resolver, and will pick the smallest version that satisfies the constraints in the manifest. Since quickcheck-macros has 1.0 as the constraint, the normal resolver will pick 1.0.109, which works just fine, while the minimal resolver will pick 1.0.0, which fails. This reveals a bug both in syn and in quickcheck-macros - the former having done a breaking change in a patch release, and the latter having a dependency on a higher version than what it specifies.

The workaround, on our end, is to specify the smallest version of syn that compiles quickcheck-macros successfully. We could do so by directly listing out the version among our dependencies, but that's kinda unfortunate, since we don't actually use it. So instead we do this trick of Jon's, where we add a dummy target with that version, which constrains the resolver, making it pick the correct version (hence the minimal-dependencies build succeeds), but doesn't actually change our dependencies when compiling for a real target.

I didn't mean to seem dubious about the solution. I'm stoked about it!

That was just my general confusion being expressed. Thanks for solving it.

No worries, man, it didn't read like that. 👍

chanced commented 1 month ago

It's the other way around! The -Zminimal-dependencies resolver works the opposite of the normal resolver, and will pick the smallest version that satisfies the constraints in the manifest. Since quickcheck-macros has 1.0 as the constraint, the normal resolver will pick 1.0.109, which works just fine, while the minimal resolver will pick 1.0.0, which fails. This reveals a bug both in syn and in quickcheck-macros - the former having done a breaking change in a patch release, and the latter having a dependency on a higher version than what it specifies.

Ah! I was under the impression it was picking up 2.0, assuming that 1.0 -> 2.0 introduced the breaking change. I didn't anticipate the breaking change having occurred somewhere between 1.0.0 and 1.0.109. In @BurntSushi's defense, I wouldn't have expected a change in dependency versions to introduce a breaking change for downstream users.

Thank you for the explanation!

Edit: actually, wow, I just realized that this isn't the result of him changing versions. Merely that he needs to specify exactly 1.0.109 (or whichever version introduces the change).

I'm definitely going to be more meticulous about selecting explicit versions from now on.

asmello commented 1 month ago

Edit: actually, wow, I just realized that this isn't the result of him changing versions. Merely that he needs to specify exactly 1.0.109 (or whichever version introduces the change).

This! Yeah it's a really easy pitfall... That's why Jon recommends this check in CI, even though it can be annoying due to transitive problems. It's a sign of brokenness with cargo, but in general I've found that specifying the full version down to patch is the best approach, as dependabot can ensure we still receive patch updates automatically.