metals
metals copied to clipboard
Add LSP jar file system
This is yet another way of implementing the FileSystemAPI.
I found with https://github.com/scalameta/metals/pull/3750 that it was getting too complicated and there were code paths in the MetalsURLConnection that I was unsure of. There were also performance issues and I think it would have taken a lot of investigation to get the custom URLConnection and custom FileSystem on par with the built in Java ones.
So instead this PR simply maps jar: <-> metalsfs: uris whenever Metals communicates via LSP or via Debug Adaptor. Bugs in this area should be easy to track down/fix as it's probably down to missing mapper code in one of the messages.
This PR shouldn't change any client that doesn't support jar: scheme in virtual documents. As far as I know - that's only VSCode. If there are others then they will need to change from handling jar: to handling metalsfs: scheme. If they want they can also implement the LSP FileSystemAPI but they don't have to as virtual docs is separate from FileSystemAPI support.
By default - on VSCode - the user won't switch to a multi-workspace and display Metals libraries in the File Explorer but they have a command Show libraries folder in file explorer to do that.
I've tested this PR with navigation, debugging, searching jars, Metals Tree View and all seems ok.
This PR also auto-decompiles class files when they are missing source jars and allows navigation within that decompiled class file. It also allows decoding of class and tasty files from within jars. Currently it only does this from the file explorer. You can't run "goto definition" on a symbol that has no source jar. Ideally this would jump to an auto-decompiled class file of the workspace jar. I'm just not sure where this should be done. It doesn't find the non-source jar because SymbolIndexBucket#query0 only calls loadFromSourceJars - it doesn't try a loadFromWorkspaceJars equivalent. Is that where the code should be changed? Or should it be in MetalsLanguageServer#definitionOrReferences I don't want to cause issues (or performance issues) in other codepaths.
I've changed the MetalsDebugAdapter interfaces to use URI (as a String) instead of AbsolutePath. Is this an issue - is it a public interface? I see that there are 2 versions of the interface - do I need to create a 3rd?
The issue is that metalsfs: is not a valid path (which AbsolutePath wraps) and passing it as a URI fixes this.
I wonder if the uri mapping will also help VSCode notebook cells which I think have the scheme vscode-notebook-cell:
I've tried to test this without virtual docs enabled to mimic the behaviour of non-VSCode clients but it would be handy if someone double checked I've not caused any issues for those clients.
I've done nothing with handling CompletionRequest in DebugProxy line:120->146. I'm not sure if this should handle jar files - what does it actually do?
(I haven't looked at this PR yet so this comment may be absurd)
I was experimenting with Java & vscode and I found that they show dependencies in a separate tree view. It's similar to packages in Metals view). I didn't check how they handle with missing sources but this might a a good lead.
Screenshot
@tgodzik This is ready for review whenever you have time.
I think the FileWatcherLspSuite test failure on the Mac is spurious as it happens on virtual docs and the readonly. But I can't test locally as I don't have mac.
@tgodzik This is ready for review whenever you have time.
I think the
FileWatcherLspSuitetest failure on the Mac is spurious as it happens on virtual docs and the readonly. But I can't test locally as I don't have mac.
Awesome! I will take a look after the weekend.
I've done nothing with handling CompletionRequest in DebugProxy line:120->146. I'm not sure if this should handle jar files - what does it actually do?
It handles completions in the debug console, so most likely it doesn't need to be handled here. Taking a look at the PR!
@tgodzik There is possibly a third way of implementing this.
The LSP filesystem could present the exact file system info the jar has.
e.g. jar:file:///home/me/coursier/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.16/scala-library-2.12.16.jar
This would mean no special metalfs: uri scheme or java file system needing to be created. Or any mapping of URIs within message handlers. Or any mapping for the debug adapter.
The downside is that the file explorer would not be flat so the jars would be in deep dirs. Breadcrumbs would work within a jar - which is likely where they are most used.
To solve the file explorer issue - we could add a additional nodes to the Packages Tree view (under Projects and Libraries) which would allow the user to browse the jdk, source jars and workspace jars.
I think this would add a chunk of functionality to the user (breadcrumb navigation and jar browsing) that could be built on by Metals later. Then we could either go down the java file system root or down the mapping root but that could be decided at a later date.
Ideally something like https://github.com/microsoft/vscode/pull/124903 would be implemented so that a flat structure could be represented by the LSPFileSystemAPI but the original jar: URIs would be used for file management
There is possibly a third way of implementing this.
The LSP filesystem could present the exact file system info the jar has. e.g. jar:file:///home/me/coursier/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.16/scala-library-2.12.16.jar
Just remembered we can't do that as VSCode doesn't like "jar:file:" uris under the FileSystemAPI, only under virtual documents.
There is possibly a third way of implementing this. The LSP filesystem could present the exact file system info the jar has. e.g. jar:file:///home/me/coursier/https/repo1.maven.org/maven2/org/scala-lang/scala-library/2.12.16/scala-library-2.12.16.jar
Just remembered we can't do that as VSCode doesn't like "jar:file:" uris under the FileSystemAPI, only under virtual documents.
Ach ok, I am wondering though if we could do a proxy filesystem that would exactly the same mapping we do here and would work automatically with the new URIs. Would that be possible? I haven't yet looked into it, but I plan to see if we can improve this PR a bit in terms of maintainability
@tgodzik
I am wondering though if we could do a proxy filesystem that would exactly the same mapping we do here and would work automatically with the new URIs. Would that be possible? I haven't yet looked into it, but I plan to see if we can improve this PR a bit in terms of maintainability
#3750 is pretty much a proxy filesystem. It fakes the initial path section: metalsfs:/root/source/somejar.jar and then delegates any further path to the jar: filesystem. I can look into why it's slow if you're not keen on this mapping PR.
@tgodzik
I am wondering though if we could do a proxy filesystem that would exactly the same mapping we do here and would work automatically with the new URIs. Would that be possible? I haven't yet looked into it, but I plan to see if we can improve this PR a bit in terms of maintainability
#3750 is pretty much a proxy filesystem. It fakes the initial path section:
metalsfs:/root/source/somejar.jarand then delegates any further path to thejar:filesystem. I can look into why it's slow if you're not keen on this mapping PR.
Sorry for talking so long to look into this! I am just still trying to figure out which is the best way here. I would probably prefer the filesystem as the mapper adds the requirement that we always need to remember to map the URI, which is a bit less maintainable I think. On the other hand if using the file system is slow, I can try and look into that. In which cases was it slow?
I haven't forgotten about your PR and would love to have it merged, just want to minimize the amount of potential issues.
Going to mark this as a draft for now. If you dive back into this, please do mark it back as ready to review!
Going to mark this as a draft for now. If you dive back into this, please do mark it back as ready to review!
It's really stuck on me, since I was trying to think on how to make it a bit less complex and have so far failed to figure it out :/