Fix LSP document synchronization #544
Conversation
|
|
||
| @Override | ||
| public boolean isManagingFile(ISourceLocation file) { | ||
| return facts.containsFile(file); |
There was a problem hiding this comment.
why is this not looking at documents? (just like the Parametric one, where both look at the same container?)
| } | ||
|
|
||
| public FallbackResolver() { | ||
| instance = this; |
There was a problem hiding this comment.
I think we should add a small comment on why we're doing this
| public ISourceLocation resolve(ISourceLocation input) throws IOException { | ||
| if (isFileManaged(input)) { | ||
| try { | ||
| return URIUtil.changeScheme(input, "lsp+" + input.getScheme()).top(); |
There was a problem hiding this comment.
why the .top? that would loose information on the URI that we cannot recover?
| throw new UnsupportedOperationException("'authority' not supported by fallback resolver"); | ||
| } | ||
|
|
||
| private Set<IBaseTextDocumentService> textDocumentServices = new HashSet<>(); |
There was a problem hiding this comment.
nit: should be final
notnit: not a threadsafe container, since we're only iterating over it, never clearing any, or using the key of it, an CopyOnWriteArrayList might be the better option.
| return service.getDocumentState(file); | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
is there an alternative to null? should we throw an exception at this point? since isFileManaged is true?
what happens to a uri that was generated while the editor was open, but refenced after it is closed? do we unresolve it to the original URI?
| var fallbackResolver = FallbackResolver.getInstance(); | ||
| uri = stripLspPrefix(uri); | ||
| if (fallbackResolver.isFileManaged(uri)) { | ||
| return new ByteArrayInputStream(fallbackResolver.getDocumentState(uri).getCurrentContent().get().getBytes(StandardCharsets.UTF_16)); |
There was a problem hiding this comment.
There is a risk of a race between isFileManaged and getDocumentState. Maybe it gets better if we merge them? Would also cut down on the map lookups.
|
|
||
| @Override | ||
| public long lastModified(ISourceLocation uri) throws IOException { | ||
| return FallbackResolver.getInstance().getDocumentState(stripLspPrefix(uri)).getLastModified(); |
There was a problem hiding this comment.
can throw a NPE in case getDocumentState returns a null.
rascal-lsp/src/main/java/org/rascalmpl/vscode/lsp/uri/LSPOpenFileResolver.java
Show resolved
Hide resolved
|
|
||
| public Versioned(int version, T object) { | ||
| this.version = version; | ||
| this.object = object; | ||
| this.timestamp = System.currentTimeMillis(); |
There was a problem hiding this comment.
we have no guarantee that the versions are monotomic in their construction. so version 10 might be constructed on thread A at time 20, while version 9 got scheduled on thread B at time 22.
This gets especially messy when tpl files are around where we depend on the "newer" relationship. What I'm wondering is if we could, register the timestamp as close to the time we receive the didChange message? (it might be close to here, I haven't checked)
| @@ -41,6 +41,29 @@ | |||
| import io.usethesource.vallang.IValue; | |||
|
|
|||
| public class Locations { | |||
| public static ISourceLocation toClientLocation(ISourceLocation loc) throws IOException { | |||
There was a problem hiding this comment.
I think we should add some notes on these functions to help future developer pick the right one, as to remain compatible with this redirection feature.
|
The LSP server is responsible for the content of files that are open in the editor. This was not (fully/correctly) implemented in rascal-lsp (#355). During resolution of a source location, the fallback resolver now rewrites source locations of open files, and file operations on such a location are handled by the new
LSPOpenFileResolverclass. This works for both Rascal and DSL files.This depends on usethesource/rascal#2101 to function correctly.