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

Importing json/yaml crashes nls #1378

Closed
simonzkl opened this issue Jun 19, 2023 · 4 comments · Fixed by #1382
Closed

Importing json/yaml crashes nls #1378

simonzkl opened this issue Jun 19, 2023 · 4 comments · Fixed by #1382

Comments

@simonzkl
Copy link

Describe the bug

Importing json/yaml crashes nls. It works fine in nickel.

To Reproduce

let a = import "test.json" in
a
[2023-06-19T08:37:03Z DEBUG nls::linearization] adding term: Import("test.json") @ Original(RawSpan { src_id: FileId(5), start: ByteIndex(8), end: ByteIndex(26) })
[2023-06-19T08:37:03Z DEBUG nls::linearization] Add wildcard item: Import("test.json")
[2023-06-19T08:37:03Z DEBUG nls::linearization] adding term: Var(Ident { label: "a" }) @ Original(RawSpan { src_id: FileId(5), start: ByteIndex(30), end: ByteIndex(31) })
[2023-06-19T08:37:03Z DEBUG nls::linearization] adding usage of variable a followed by chain None
[2023-06-19T08:37:03Z DEBUG nls::linearization] linearizing
[2023-06-19T08:37:03Z DEBUG nls::linearization] unresolved references: []
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', lsp/nls/src/cache.rs:104:97
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: "SendError(..)"', /private/tmp/nix-build-nls-1.0.0.drv-0/nls-1.0.0-vendor.tar.gz/lsp-server/src/stdio.rs:29:37

Expected behavior
Should not crash.

Environment

  • OS name + version: MacOS 13.4
  • Version of the code: nickel-lang-lsp 1.0.0
@simonzkl
Copy link
Author

Anecdotally, I've also had it panic in a different location for the same reason:

// This is safe because the import file is resolved, before we linearize
// the containing file, therefore the cache MUST have the term stored.
let term = lin.cache.get(*file).unwrap();
let position = final_term_pos(&term);

@yannham yannham self-assigned this Jun 19, 2023
@yannham yannham added this to the Next minor (1.1.0) milestone Jun 19, 2023
@yannham
Copy link
Member

yannham commented Jun 19, 2023

@simonzkl I have a fix for my simple test case (and which makes sense with respect to the original location of the panic). But I didn't hit the second location yet. Maybe it's an artifact of the first issue, but just in case, would you mind sharing the JSON file you tried?

@simonzkl
Copy link
Author

simonzkl commented Jun 20, 2023

After narrowing it down, I think just switching to yaml triggers this one. It's a different line than I wrote though. Mentioned it from memory, so it was incorrect.

import "test.yaml"
[2023-06-20T06:58:23Z TRACE nls::files] Parsed, checking types
[2023-06-20T06:58:23Z DEBUG nls::linearization] adding term: Null @ None
[2023-06-20T06:58:23Z DEBUG nls::linearization] linearizing
[2023-06-20T06:58:23Z DEBUG nls::linearization] unresolved references: []
[2023-06-20T06:58:23Z DEBUG nls::linearization] adding term: ResolvedImport(FileId(4)) @ Original(RawSpan { src_id: FileId(3), start: ByteIndex(1), end: ByteIndex(19) })
thread 'main' panicked at 'TermPos::unwrap', lsp/nls/src/linearization/mod.rs:471:38
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[Error - 08:58:23] Connection to server got closed. Server will not be restarted.

@yannham
Copy link
Member

yannham commented Jun 20, 2023

Ok, this is more in line with my own findings. Thanks!

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

Successfully merging a pull request may close this issue.

3 participants