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.

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

Unicode issues with inline snapshots #137

Closed
c-spencer opened this issue Nov 3, 2020 · 8 comments
Closed

Unicode issues with inline snapshots #137

c-spencer opened this issue Nov 3, 2020 · 8 comments

Comments

@c-spencer
Copy link

c-spencer commented Nov 3, 2020

Seems to be a re-occurrence of #42

This is with cargo-insta and insta 1.1.0

Some snapshots are updating incorrectly as per the original issue (adding additional content past the end of the snapshot string), have also run into some panics as below where it's ending up trying to splice in the middle of a char.

Source code of the test causing segfault:

        adb!(
            ty("forall N:N. forall A:*. (A -> 1 + 1) -> Vec N A -> some K:N. Vec K A"),
            @"(∀ν : ℕ. (∀α : ⋆. ((α → (1 + 1)) → (Vec ν α → (∃κ : ℕ. Vec κ α)))))"
        );

Snapshot file and log/backtrace when interacting with updating:

{"run_id":"1604428650-748676057","line":166,"new":{"module_name":"hale__sac__parser__tests","snapshot_name":"type_parsing-5","metadata":{"source":"src/sac/parser.rs","expression":"ty(\"forall N:N. forall A:*. forall B:*.(A -> B) -> Vec N A -> Vec N B\")"},"snapshot":"(∀ν : ℕ. (∀α : 🟊. (∀β : 🟊. ((α → β) → (Vec ν α → Vec ν β)))))"},"old":{"module_name":"hale__sac__parser__tests","metadata":{},"snapshot":"(∀ν : ℕ. (∀α : ⋆. (∀β : ⋆. ((α → β) → (Vec ν α → Vec ν β)))))"}}
{"run_id":"1604428650-748676057","line":170,"new":{"module_name":"hale__sac__parser__tests","snapshot_name":"type_parsing-6","metadata":{"source":"src/sac/parser.rs","expression":"ty(\"forall N:N. forall A:*. (A -> 1 + 1) -> Vec N A -> some K:N. Vec K A\")"},"snapshot":"(∀ν : ℕ. (∀α : 🟊. ((α → (1 + 1)) → (Vec ν α → (∃κ : ℕ. Vec κ α)))))"},"old":{"module_name":"hale__sac__parser__tests","metadata":{},"snapshot":"(∀ν : ℕ. (∀α : ⋆. ((α → (1 + 1)) → (Vec ν α → (∃κ : ℕ. Vec κ α)))))"}}
-old snapshot
+new results
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
ty("forall N:N. forall A:*. (A -> 1 + 1) -> Vec N A -> some K:N. Vec K A")
────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────
    1       │-(∀ν : ℕ. (∀α : ⋆. ((α → (1 + 1)) → (Vec ν α → (∃κ : ℕ. Vec κ α)))))
          1 │+(∀ν : ℕ. (∀α : 🟊. ((α → (1 + 1)) → (Vec ν α → (∃κ : ℕ. Vec κ α)))))
────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────

  a accept   keep the new snapshot
  r reject   keep the old snapshot
  s skip     keep both for now
thread 'main' panicked at 'byte index 82 is not a char boundary; it is inside '∃' (bytes 80..83) of `            @"(∀ν : ℕ. (∀α : ⋆. ((α → (1 + 1)) → (Vec ν α → (∃κ : ℕ. Vec κ α)))))"`', /home/chris/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/str/mod.rs:2052:47
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/libunwind.rs:86
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:78
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:59
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1076
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1537
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:62
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:198
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:218
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:490
  11: rust_begin_unwind
             at src/libstd/panicking.rs:388
  12: core::panicking::panic_fmt
             at src/libcore/panicking.rs:101
  13: core::str::slice_error_fail
             at src/libcore/str/mod.rs:0
  14: core::str::traits::<impl core::slice::SliceIndex<str> for core::ops::range::RangeFrom<usize>>::index::{{closure}}
  15: cargo_insta::inline::FilePatcher::set_new_content
  16: cargo_insta::cargo::SnapshotContainer::commit
  17: cargo_insta::cli::process_snapshots
  18: cargo_insta::cli::run
  19: cargo_insta::main
  20: std::rt::lang_start::{{closure}}
  21: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:52
  22: std::panicking::try::do_call
             at src/libstd/panicking.rs:297
  23: std::panicking::try
             at src/libstd/panicking.rs:274
  24: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  25: std::rt::lang_start_internal
             at src/libstd/rt.rs:51
  26: main
  27: __libc_start_main
  28: _start
@max-sixty
Copy link
Sponsor Contributor

I can have a look at this. Do you have a minimal repro, with only insta?

@c-spencer
Copy link
Author

c-spencer commented Nov 3, 2020

Just checked in a fresh crate, this test is enough to reproduce a panic for me on its own:

#[test]
fn unicode() {
    insta::assert_debug_snapshot!(
        "(∀ν : ℕ. (∀α : 🟊. ((α → (1 + 1)) → (Vec ν α → (∃κ : ℕ. Vec κ α)))))",
        @"(∀ν : ℕ. (∀α : ⋆. ((α → (1 + 1)) → (Vec ν α → (∃κ : ℕ. Vec κ α)))))"
    );
}

Placing that in a project, and then using cargo insta test + cargo insta review results in:

thread 'main' panicked at 'byte index 78 is not a char boundary; it is inside '∃' (bytes 76..79) of `        @"(∀ν : ℕ. (∀α : ⋆. ((α → (1 + 1)) → (Vec ν α → (∃κ : ℕ. Vec κ α)))))"`...

@max-sixty
Copy link
Sponsor Contributor

I can't repro, unfortunately. Please could you try copying & pasting the snippet from your GH comment to confirm we're running the same code?

Alternatively you could publish the repo to GH; or I'm open to some other way of reproducing.

image

@c-spencer
Copy link
Author

c-spencer commented Nov 4, 2020

Perhaps a missing reproduction step - happens when trying to accept the snapshot within cargo insta review?

The files involved are just the above (as lib.rs) and a normal Cargo.toml:

[package]
name = "insta-repro"
version = "0.1.0"
edition = "2018"

[dev-dependencies]
insta = "1.1.0"

Reproduced on both a MacOS machine and arch linux. Rust 1.47.0. I reproduced on MacOS (separate machine from initial occurence) just by copying from this issue.

@max-sixty
Copy link
Sponsor Contributor

happens when trying to accept the snapshot within cargo insta review?

I think that's the screenshot above, no?

I tried copying the lib.rs, but I still can't repro.

Could you confirm that the copying the text back from GH still reproduces? I see one character that may be an unknown code point.

Otherwise I think the best alternatives are to push a failing test to insta, or push a reproducing repo to GH.

@c-spencer
Copy link
Author

c-spencer commented Nov 5, 2020

Reproduction repository: https://github.com/c-spencer/insta-unicode-repro
Action result: https://github.com/c-spencer/insta-unicode-repro/runs/1358803519

Apologies, I'd like to dig into this myself but haven't time currently to orient myself on insta codebase. Extra content (unicode_len) in the lib.rs was just some sanity checking I tried, has no effect on the outcome.

@max-sixty
Copy link
Sponsor Contributor

That works! I'll have a look in the next few days

@max-sixty
Copy link
Sponsor Contributor

This is confusing. I misspoke above — I still can't reproduce. Your repro is very compelling. The only thing I can think of is Linux vs Mac.

Or I'm missing something — let me know if anything stands out to you. FYI I matched rust version & also ran cargo insta test --accept

Screen Cast 2020-11-06 at 9 14 32 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants