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

Reusing cache for LSP #408

Open
ysndr opened this issue Sep 24, 2021 · 1 comment
Open

Reusing cache for LSP #408

ysndr opened this issue Sep 24, 2021 · 1 comment

Comments

@ysndr
Copy link
Contributor

ysndr commented Sep 24, 2021

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 #405 (comment)

@ysndr
Copy link
Contributor Author

ysndr commented Sep 24, 2021

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

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

No branches or pull requests

1 participant