search

tweag / nickel

Better configuration for less
https://nickel-lang.org/
MIT License
2.36k stars 89 forks source link

Reusing cache for LSP #408

Open ysndr opened 3 years ago

ysndr commented 3 years ago

I don't really like the idea of exposing publicly the underlying hashmap. That's ok for now if it's a blocker for you, because it seems the cache is not handling file changes correctly, but let us maybe open an issue to fix the actual problem and remember to remove this once this is not needed anymore.

_Originally posted by @yannham in https://github.com/tweag/nickel/pull/405#discussion_r715787873_

ysndr commented 3 years ago

The cache module is currently reused as it in the implementation for the language server where it provides access to adding source roots, parsing, and type checking.

In the use case of a language server, source files are repeatedly updated, which before #405 was not possible. It is possible in that the content associated with a FileId can be updated (through cache.files_mut().update(_). Yet, parsing the same FileId is not possible as it is short cut in the implementation if there is an entry in terms (which there is after the first parse).

Hence, i added access to the terms hash map in order to invalidate the file, i.e. removing the root term for that file.

Clearly, this gives way more access to that structure than necessary. There are alternatives however:

  1. we could have an invalidate_root(file_id) method but if we start doing this and want to use the cache terms structure in future analysis we will be adding multiple methods just for the lsp

  2. looking forward to a more fine grained update mechanism, we might do more changes on this structure altogether, to a point where a different structure would be preferable over the current HashTree approach in that case implementing a proper cache module solely for the LS might be the preferable way forward. Note that this might lead to a great degree of code duplication

  3. somewhat hybrid and just off of my head at this point is to abstract out the terms implementation, providing variably potent wrappers around a HashMap (or in case of the LS possibly different DS) I think this would also allow to implement proper extension traits for a cache used in a specific context This could be a hybrid approach between reusing the cache as it, retaining most of its functionality, not needing to duplicate everything and not introducing insecure access to consumers of the nickel RT lib