Fix LSP panic when importing JSON #1382
Conversation
Nickel terms imported from a JSON file currently don't have any position set, which was making the LSP panic. This commits stores the original input format alongside cached terms, and makes this information available to the public API. The LSP now uses it and abort any usage analysis when the term comes from an external import.
This commit adds a test to the LSP test suite to check that importing an external format doesn't make it crash as it used to.
lsp/nls/src/linearization/mod.rs
Outdated
|
|
||
| // If the import is an external format (such as JSON or YAML), no position will be | ||
| // set. We could in fact set at least the position of the root term, but since this | ||
| // isn't the case currently, an unwrap() below would fail. For now, we simply bail | ||
| // out. This just means we can't "go to definition" on an external import for the | ||
| // time being. | ||
| if lin | ||
| .cache | ||
| .input_format(file) | ||
| .map(|fmt| !matches!(fmt, InputFormat::Nickel)) | ||
| .unwrap_or(false) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // 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); | ||
|
|
||
| // This unwrap fails only when position is a `TermPos::None`, which only happens | ||
| // if the `RichTerm`, has been transformed or evaluated. None of these happen before | ||
| // linearization, so this is safe. | ||
| // unwrap(): this unwrap fails only when position is a `TermPos::None`, which only | ||
| // happens if the `RichTerm` has been transformed or evaluated. None of these | ||
| // happen before linearization. At this point, we also ensured that we are only | ||
| // considering a Nickel source (and not a term deserialized from e.g. JSON), and | ||
| // terms parsed from Nickel must have a position set. So |
There was a problem hiding this comment.
Not that it really matters, but in essence we're getting the term out of the linearization cache twice. First to check that the input format is Nickel. Then we throw away the entry and find it again, this time to just unwrap it. Maybe a let else construct could be better here?
There was a problem hiding this comment.
I think cache.get returns a RichTerm, without the metadata. We probably don't want to change this API, which is used in several places. What I might do is to either add a method that returns the whole CachedTerm, but I'm not sure it's a useful interface. Or one that returns a term together with the input_format. On the other hand, it's just one additional hashmap get by import, which is probably totally negligible.
There was a problem hiding this comment.
I agree, it's probably negligible. Let's not hold up this PR with that 👍
There was a problem hiding this comment.
In the end I added a method get_with_input_format, which looks reasonable.
Add an helper to `cache` to retrieve both the input format and a cached term from the cache with only one hashmap access. This commit also updates some related comments inside the code of the LSP, which were not up-to-date anymore.
Closes #1378
Context
Nickel terms imported from a JSON file currently don't have any position set, which is making the LSP panic. Indeed, imported expressions are registered as an usage site, to enable go to definition on them (which jumps to the corresponding file). The LSP expects that parsed terms have all their positions set, and in particular the one of the root term (corresponding to the whole file).
This commits stores the original input format alongside cached terms, and makes this information available to the public API of
cache. The LSP can thus use it and abort any usage analysis when the term comes from an external import.Improvement
I tried to set the position of terms imported from YAML and JSON, which should enable goto definition on expressions imported from external formats as well. This commit is in fact included in this PR. This does make the original panic go away, but then completion (which works for the first level of nesting) on a value imported from e.g. the YAML file of the github settings of this repo causes a stack overflow on the second level (
value.repository.[.]), which crashes the server with a stack overflow. By fixing this stack overflow, we might remove the special treatment of external formats, restoring goto definition and maybe even completion. Until then, it's probably better to have less features but make the LSP more resilient.