LanguageServer.jl icon indicating copy to clipboard operation
LanguageServer.jl copied to clipboard

Should we handle files outside the workspace at all?

Open davidanthoff opened this issue 4 years ago • 2 comments

My proposal here would be the following:

  • we never add a file to the server._documents collection if that file is outside of the current workspace, unless we receive a didOpen request for such a file. Once such a file outside the workspace is closed again via didClose, we remove it from the server._documents collection.
  • from StaticLint's point of view, the content of server._documents is all there exists in terms of code files. If a file is not in that collection, it simply isn't available to StaticLint.
  • the code that follows include statements would only operate on the files in the server._documents collection. If it finds an include statement that points to files outside the workspace, it would add a diagnostic message to that particular include suggesting that the target folder should be added to the workspace.
  • we generally don't publish diagnostics for any file that isn't part of the workspace (I think we should do that no matter what, actually, even if we don't adopt the full idea in this issue here). In particular, we never publish any diagnostic for a file that was opened via didOpen but is not part of the workspace.
  • the behaviour for files outside the workspace that are opened via didOpen, we would offer a reduced functionality: things like code navigation inside the file could still work, but stuff like goto definition not. I think that is actually quite reasonable in any case, because we currently also don't get things right (for example, we don't really know what env we should use to resolve symbols from symbol server for such files).
  • we hope that at some point we can utilize LSIF for rich code navigation of files that are not in the workspace. I find the available info on LSIF a bit confusing, and it seems to me that there are some missing pieces in general how this will all work together. But my general sense is that this is the design they are moving towards: LS for files in the workspace, LSIF for stuff from packages etc. So the way this might play out is that we have a LSIF file for Julia base (unclear how we aquire that), and I could imagine a world where the new package server actually hosts LSIF files for packages or something like that. Clearly all future talk, because right now it is quite unclear to me how this would then be integrated into VS Code, but I think generally that seems like a promising direction.

I think the main benefits of this are these:

  • Much simplified logic: the LS messages control what we load and when, no "secondary" loading triggers that complicate stuff.
  • A much clearer layering. StaticLint gets a read-only view of the world, that is it. I think that will make a multi-threading story much simpler down the road, the kind of design we have right now where StaticLint can trigger new loads makes a simple multi threading design super, super tricky.
  • Much clearer defined semantics: currently things like "we look three levels up in the file system" are kind of unpredictable
  • I think the relationship between env and files outside of workspace folders is even more complicated than getting things right inside a workspace. This way we just don't even try, which seems better to me.
  • We don't have to come up with our own file watching story. We can't use the client to watch files outside of the workspace, but we need to be notified of updates, so we would have to roll our own... To me the fact that the client doesn't support this is also kind of an indication that the VS Code team generally thinks files outside of the workspace should not be handled by extensions (ok, I admit this is a bit speculative ;) ).

davidanthoff avatar Feb 19 '20 03:02 davidanthoff

Here is another point: if we do follow these kinds of links, we would need to do it at every didChange request, not just in didOpen, as we do today. That seems a very heavy operation for the didChange request, in particular given that then can trigger file IO etc...

davidanthoff avatar Feb 19 '20 16:02 davidanthoff

Hi, initial thoughts (mostly on the benefits, in order):

  1. I'm not quite sure what is being complicated or simplified by either approach - we have a list of files that are available stored by name and an easy way to see if they are part of the same tree structure (shared root). There is currently only one other way for a file to be loaded (i.e. one function call location).
  2. I think it's too early to be making any considerations for support of threading! (But what did you think could be an issue?)
  3. Its a simple approach that I'd guess works in the majority of cases
  4. Yep, I wouldn't dream of trying to handle the env of non-workspace files. I think your comment above about not running diagnostics on non-workspace files is especially valid here (something we should implement right away).
  5. I don't think there's necessarily a need to watch files.

I think we agree on treating out-of-workspace files as second class citizens, just have a slightly different idea of what that would mean at the moment. For example, what I would not like to lose is the ability to debug some issue by following definitions all the way through /base.

ZacLN avatar Feb 20 '20 23:02 ZacLN