-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix rascal-lsp incorrect implementation of the LSP Document synchronisation for DSLs #355
Comments
We can also register a (set of) logical URI providers that catch all files that are handled by the LSP server and default to the existing handlers when not handled by us. Open and close can make sure we register and de-register the handlers. By doing so the rascal code that uses the library builtins can remain oblivious to this strange situation. Btw the eclipse IResource mechanism also worked in such a manner; a file system abstraction with in memory caching. |
I like the beauty of that idea, but that would mean we have to do #129 first, to make sure the VS Code itself is not seeing those emulated files. |
Ok let's think this through in a meeting. I think we need to clarify some things first. Probably if we want to have this facade and make the difference when we need to, we have to use something else than the logical resolvers. The reason is that logical resolvers always map to another URI. That's necessary but it's not sufficient. Here we also need to implement different streams. So at least there will be a real URI resolver for files in the LSP space. Like LSP+project:// and a rewriter/logical resolver to forward to that new scheme. And finally we need to be able you selectively rewrite and not always. Perhaps #129 is the answer to that, as you suggested. #129 can only work if we get rid of the lib scheme. That's because the lib scheme URI are not stable between JVM instances. |
the lib schema is already broken over that bridge. since you would get the lib schema is interpreteed by the rascal lsp jvm, not the REPL (which has some chance of working). But agreed, let's have a meeting and hash it out. |
Part of this is solved in #357, but the whole IO operations shouldn't look on disk is still wide open. |
During debugging of #354 I've discovered we implemented LSP incorrectly. For keeping it simple, I'll focus on the Parametric implementation, the Rascal implementation has roughly the same problems, but will require a different solution I suspect.
Specification vs current implementation
I'll be quoting form the LSP specification 3.17 a bit.
Currently our implementation starts to keep track of the document contents after an
open
event. Request that target this current document will use that state, and it's parse tree to provide callbacks to rascal lsp. So for the document in question, we respect the rules, we are not reading from disk.However:
exists
&lastModified
and other functions inIO
will go to disk, and potentially correspond to the wrong "state" of the documents. As a change in LSP does not have to mean a change visible on disk.lastModified
and the unsaved state in the editor is a known example).Currently if VS Code asks us information about a document that is closed (or never opened before) we throw an exception. Even if we've seen the document before. The correct behavior is that is non-opened, we are allowed to read the file from disk, and answer the question that is asked.
We are currently not tracking the version number of a text document. If done well, we can relate diagnostics to certain version of a text document, so VS Code knows when to clean them, or update their location.
Solution proposed
I think we have to:
This is not a small change, but it does solve some of the subtle issues we've been seeing.
Also, for a rascal-core based typechecker, this might become a bit more messy ("disconnecting" it's IO operations), so let's discuss that in a different issue.
The text was updated successfully, but these errors were encountered: