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

Add optional support for class files with file scheme #2322

Open
husainaloos opened this issue Nov 13, 2022 · 8 comments
Open

Add optional support for class files with file scheme #2322

husainaloos opened this issue Nov 13, 2022 · 8 comments

Comments

@husainaloos
Copy link

Hello,

I use emacs with eglot. I noticed that sometimes, jdt.ls returns URIs on the form jdt:// when trying to find a definition. Eglot does not handle these URIs.

Is it possible to configure jdt.ls to use file:// protocol instead? I noticed that other packages parse jdt:// and convert them to something useful (I see that emacs's lsp-java and nvim nvim-jdtls do that. I suspect vscode does as well).

Excuse my naivity as I don't now the design and purpose of return jdt:// uris (not sure what that protocol is meant for).

@dannyfreeman
Copy link

I just heard about this issue from someone and wanted to chime in.

Clojure-lsp allows for navigating to decompiled class files without any fancy language extensions. When handling a call to textDocument/definition methods, it decompiles the classes to a temp directory and just returns a normal file URI pointing to the temp .java file. For source files in jars, it returns a JAR scheme URI, which seems kind of standardized and is easy enough to work with. These JDT URIs seem to be non standard and look really hard to parse. Maybe they aren't meant to be parsed, and just thrown back to the jdtls? I'm not sure.

Either way, decompiling classes to a temp directory would be a great thing to support in jdtls in my opinion, as it would remove the need for language extensions and special implementation in clients.

@rgrunber
Copy link
Contributor

rgrunber commented Nov 14, 2022

We don't use file because the code responsible for file would not be able to handle classfile content. There's no "file" on the filesystem we could return because the content itself is often embedded in a jarfile. I'm not sure why we didn't go with jar:// as I would think that could work, but it looks like we embed some additional info in the uri that jar:// probably can't handle.

The jdt scheme (eg. jdt://) is defined by the language server and it's up to clients to support it. Admittedly, the documentation on this is thin so maybe I can explain it here, then add a section about it in our documentation. Here is where we associate a given classfile with the jdt scheme :

https://github.com/eclipse/eclipse.jdt.ls/blob/6c61f23f3b9818d6e54c993095a710ad51600cfc/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java#L802-L816

We call this in quite a few places. The good news is that although, we return this URI to clients, we also provide the methods to handle its content.

Since each client must write some code to deal with this, let me provide an example from vscode-java.

https://github.com/redhat-developer/vscode-java/blob/6d10e1a00c9ec3628f8934e71be92c1a04acc540/src/providerDispatcher.ts#L30

registerTextDocumentContentProvider(..) is basically taking ownership of all document content whose schema is jdt://. From there, the server can send jdt:// uris and this handler will activate for them.

https://github.com/redhat-developer/vscode-java/blob/6d10e1a00c9ec3628f8934e71be92c1a04acc540/src/providerDispatcher.ts#L66

createJDTContentProvider(..) simply defines that we send a request back to the server, calling 'java/classFileContents' (obviously not part of the standard LSP spec) with the corresponding uri passed in. This will return a string representation of the classfile to be used for displaying it.

Altogether, the interaction looks like this :

[Trace - 10:05:10 a.m.] Sending request 'textDocument/definition - (90)'.
Params: {
    "textDocument": {
        "uri": "file:///home/rgrunber/sample-projects/json-example/src/org/example/Item.java"
    },
    "position": {
        "line": 29,
        "character": 16
    }
}

[Info  - 10:05:10 a.m.] Nov. 14, 2022, 10:05:10 a.m. >> document/definition
[Trace - 10:05:10 a.m.] Received response 'textDocument/definition - (90)' in 18ms.
Result: [
    {
        "uri": "jdt://contents/java.base/java.lang/String.class?=json-example_96bcdf0c/%5C/usr%5C/lib%5C/jvm%5C/java-17-openjdk%5C/lib%5C/jrt-fs.jar%60java.base=/javadoc_location=/https:%5C/%5C/docs.oracle.com%5C/en%5C/java%5C/javase%5C/17%5C/docs%5C/api%5C/=/%3Cjava.lang(String.class",
        "range": {
            "start": {
                "line": 139,
                "character": 19
            },
            "end": {
                "line": 139,
                "character": 25
            }
        }
    }
]

[Trace - 10:05:10 a.m.] Sending request 'java/classFileContents - (91)'.
Params: {
    "uri": "jdt://contents/java.base/java.lang/String.class?%3Djson-example_96bcdf0c%2F%5C%2Fusr%5C%2Flib%5C%2Fjvm%5C%2Fjava-17-openjdk%5C%2Flib%5C%2Fjrt-fs.jar%60java.base%3D%2Fjavadoc_location%3D%2Fhttps%3A%5C%2F%5C%2Fdocs.oracle.com%5C%2Fen%5C%2Fjava%5C%2Fjavase%5C%2F17%5C%2Fdocs%5C%2Fapi%5C%2F%3D%2F%3Cjava.lang%28String.class"
}

[Info  - 10:05:10 a.m.] Nov. 14, 2022, 10:05:10 a.m. ClassFile contents request completed
[Trace - 10:05:10 a.m.] Received response 'java/classFileContents - (91)' in 14ms.
Result: ...
...
[String Representation Of java.lang.String]
...

If you want to use your own dissassembler on the classfile content, we support this. We have an extension point for content providers :

https://github.com/eclipse/eclipse.jdt.ls/blob/6c61f23f3b9818d6e54c993095a710ad51600cfc/org.eclipse.jdt.ls.core/schema/org.eclipse.jdt.ls.core.contentProvider.exsd#L50

We use it to contribute a basic dissasembler :

https://github.com/eclipse/eclipse.jdt.ls/blob/6c61f23f3b9818d6e54c993095a710ad51600cfc/org.eclipse.jdt.ls.core/plugin.xml#L43-L51

You'd need to create a separate bundle that gets contributed via. https://github.com/eclipse/eclipse.jdt.ls/wiki/Contribute-an-extension-bundle#use-the-jdtls-extension but would be possible.

@dannyfreeman
Copy link

Thanks for the response, that paints a pretty clear picture of what is going on now.

We don't use file because the code responsible for file would not be able to handle classfile content. There's no "file" on the filesystem we could return because the content itself is often embedded in a jarfile. I'm not sure why we didn't go with jar:// as I would think that could work, but it looks like we embed some additional info in the uri that jar:// probably can't handle.

I understand why file schemes won't work as is for pointing to a zipped classfile. Even if it weren't zipped, I wouldn't want to look at all that byte code :) I'd like to propose an alternative for how it's handled now with the JDT urls and client extensions. Instead of requiring every new client to implement custom code to handle this, JDTLS could decompile the classfiles and save the files in a temp directory on the file system. Then the file scheme would work, right? Clients wouldn't need to do anything different, it's the same action as navigating to another source file in the project. That makes JDTLS more accessible out of the box for more clients. There is precedence for this behavior in clojure-lsp and metals, a scala language server.

Is that something that might be possible?

@rgrunber rgrunber changed the title dealing with jdt:// URIs Add optional support for class files with file scheme Nov 14, 2022
@rgrunber
Copy link
Contributor

rgrunber commented Nov 14, 2022

If we ended up creating a separate file for every .class that could get very complicated. It would have to be stored per-project and get re-generated every time we detect the underlying library changed. I agree we should try to make it easier for clients to implement such functionality, and having more things to implement client side does reduce the likelihood of clients taking full advantage of features. It might be possible to create an extended client capability that informs the server whether the client intends to support jdt:// . The server could then respond with either jdt or file, depending on the client's setting. Still, having to write out each classfile to the filesystem on-demand would likely slow things down.

Is there some uri scheme, we could return from 'textDocument/definition` that most clients will support ? Doesn't look like it.

We use the query string portion (the part of after the ?) in order for JDT to uniquely identify the classfile :

jdt://contents/java.base/java.lang/String.class?=json-example_96bcdf0c/\\/usr\\/lib\\/jvm\\/java-17-openjdk\\/lib\\/jrt-fs.jar`java.base=/javadoc_location=/https:\\/\\/docs.oracle.com\\/en\\/java\\/javase\\/17\\/docs\\/api\\/=/<java.lang(String.class

https://github.com/eclipse/eclipse.jdt.ls/blob/6c61f23f3b9818d6e54c993095a710ad51600cfc/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java#L390-L395

That's definitely custom enough that nothing is going to support this, which is why the client is forced to query the server once again, in order to get the proper content to display to the user.

I've marked this as an enhancement since it could benefit other clients, but the work to support jdt:// is just a matter of extending the client's existing "document type registry", querying JDT-LS once more, and returning the resulting content in the "document type registry provider".

@dannyfreeman
Copy link

dannyfreeman commented Nov 14, 2022

If we ended up creating a separate file for every .class that could get very complicated. It would have to be stored per-project and get re-generated every time we detect the underlying library changed.

I could see it getting complicated. I've never had an underlying library change from underneath a project though. The versions in the filepaths to the jars themselves typically take care of that. Maybe that's more common in java projects than the clojure projects where I normally work.

It might be possible to create an extended client capability that informs the server whether the client intends to support jdt:// . The server could then respond with either jdt or file, depending on the client's setting.

I think that would be a fantastic way to handle this.

Still, having to write out each classfile to the filesystem on-demand would likely slow things down.

I wouldn't be so sure about this degrading performance in a meaningful way. My experience with both clojure-lsp and poking at the scala laguage server has been that this strategy is fast enough for me not to notice a delay between sending a textDocument/definition request, and my editor landing in the middle of a decompiled class file.

Is there some uri scheme, we could return from 'textDocument/definition` that most clients will support ? Doesn't look like it.

Nope :) The closest thing is a zipfile or jar uri, but this doesn't really make sense with class files. They have to decompiled, and the server needs to know how it is being decompiled to be able communicate the resulting location in the response.

To me this issue is a little deeper than finding a URI scheme to support this. The complexity of the JDT URI scheme speaks to that as well, with what looks to be a lot of information encoded in the URI. There are a number of LSP servers I have seen that need some way to tell a client "I know where the location of this type, definition, whatever is, but it isn't a plain file on your filesystem, some extra work needs to be done to retrieve it". Maybe that work is decompiling something, unzipping something, or downloading it from the internet. Either way, this is a gap in the LSP spec that different language servers and clients have filled in different ways. The current Location response isn't enough.

I've marked this as an enhancement since it could benefit other clients, but the work to support jdt:// is just a matter of extending the client's existing "document type registry", querying JDT-LS once more, and returning the resulting content in the "document type registry provider".

Thank you, I hope it gets some serious consideration. The work to support these JDT URIs is more than "just" implementing it. The original issue is about eglot, an Emacs lsp client that is set to be distributed with Emacs next major release. I do not foresee handling jdt URLs being added to that client, as the maintainers are very adamant about not implementing anything outside the LSP spec in Emacs core. There are simply too many small exceptions to the standard like this for a small team to implement and stay on top of. In the meantime, I may try to add support in a separate Emacs package for these JDT URIs.

@rgrunber
Copy link
Contributor

The work to support these JDT URIs is more than "just" implementing it.

Does emacs-lsp/lsp-java#22 help ? Can any of that be re-used ?

@dannyfreeman
Copy link

Not really. Eglot and lsp-mode are different lsp clients with different design philosophies.

But that is beside the point. The code itself wouldn't be that hard to write. Support for this bespoke URL format for this one specific server probably won't make it into Eglot and Emacs core, it will end up in a separate package that someone has to write and maintain. So out of the box, this lsp server won't play very nicely with Emacs and will require the user to install extra packages for it to work correctly. That's not the end of the world. Maybe other lsp-clients require users to download extra packages to support servers like jdtls and users are fine with that. I don't know though.

@theothornhill
Copy link

FWIW, @dannyfreeman , we could support this in emacs using something like:

(defun jdt-file-name-handler (operation &rest args)
  "Support Eclipse jdtls `jdt://' uri scheme."
  (let* ((uri (car args))
         (cache-dir "/tmp/.eglot")
         (source-file
          (expand-file-name
           (file-name-concat
            cache-dir
            (save-match-data
              (when (string-match "jdt://contents/\\(.*?\\)/\\(.*\\)\.class\\?" uri)
                (format "%s.java" (replace-regexp-in-string "/" "." (match-string 2 uri) t t))))))))
    (unless (file-readable-p source-file)
      (let ((content (jsonrpc-request (eglot-current-server) :java/classFileContents (list :uri uri)))
            (metadata-file (format "%s.%s.metadata"
                                   (file-name-directory source-file)
                                   (file-name-base source-file))))
        (unless (file-directory-p cache-dir) (make-directory cache-dir t))
        (with-temp-file source-file (insert content))
        (with-temp-file metadata-file (insert uri))))
    source-file))

(add-to-list 'file-name-handler-alist '("\\`jdt://" . jdt-file-name-handler))

Seeing how eglot finally is in core, I can open a bug report and see if we can include some similar code to this, either in eglot or somewhere a file-handler goes. In the meantime you can steal this for your own config :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants