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

jdtls does not support includeDeclaration in textDocument/references? #2148

Closed
zbelial opened this issue Jul 3, 2022 · 5 comments · Fixed by #2253
Closed

jdtls does not support includeDeclaration in textDocument/references? #2148

zbelial opened this issue Jul 3, 2022 · 5 comments · Fixed by #2253

Comments

@zbelial
Copy link

zbelial commented Jul 3, 2022

I tested this in Emacs with lsp-mode, and jtd.ls did not return the declaration. Can any confirm this please?

@zbelial zbelial changed the title jdtls does not support includeDeclaration in textDocument/references jdtls does not support includeDeclaration in textDocument/references? Jul 3, 2022
@rgrunber
Copy link
Contributor

rgrunber commented Jul 21, 2022

I'm not sure that JDT-LS is meant to do anything in addition, though maybe I'm not understanding how the protocol works.

According to https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#referenceContext , includeDeclaration is a parameter passed to the request and JDT-LS does set it :

[Trace - 10:04:21 a.m.] Sending request 'textDocument/references - (172)'.
Params: {
    "textDocument": {
        "uri": "file:///home/rgrunber/git/lemminx/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/utils/XMLPositionUtility.java"
    },
    "position": {
        "line": 419,
        "character": 31
    },
    "context": {
        "includeDeclaration": true
    }
}

But according to the protocol, the response can only be null, or Location[] :

[Trace - 10:04:21 a.m.] Received response 'textDocument/references - (172)' in 95ms.
Result: [
    {
        "uri": "file:///home/rgrunber/git/lemminx/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/participants/ContentModelCodeLensParticipant.java",
        "range": {
            "start": {
                "line": 140,
                "character": 35
            },
            "end": {
                "line": 140,
                "character": 63
            }
        }
    },
    {
        "uri": "file:///home/rgrunber/git/lemminx/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/participants/DTDErrorCode.java",
        "range": {
            "start": {
                "line": 273,
                "character": 29
            },
            "end": {
                "line": 273,
                "character": 57
            }
        }
    },
   ...
   ...
]

I'm not sure the protocol allows us to provide the declaration. Might this be something the client needs to request ?

Update : I guess we could simply include the declaration as the location, but it seems strange to do that particularly when a declaration (eg. a method) has a large block.

@zbelial
Copy link
Author

zbelial commented Jul 24, 2022

Hi, thanks for your reply.

Update : I guess we could simply include the declaration as the location, but it seems strange to do that particularly when a declaration (eg. a method) has a large block.

If I understand the protocol correctly, I think what you said here is a server should do if includeDeclaration is set true in the request.

With regard to declarations with large blocks, I think including the declaration as a location is no problem. IMO, references are often used to help people have an overview of where a symbol is used (and declared) and jump to one of them. For a method, jumping to the beginning of it makes sense.

Some other LSP servers ( such as pyright and rust-analyzer ) will include the declaration as a location if includeDeclaration is set.

@rgrunber rgrunber added bug and removed question labels Aug 2, 2022
@rgrunber
Copy link
Contributor

rgrunber commented Aug 2, 2022

Right. I was interpretting includeDeclaration as the language server returning the symbol under which each reference is discovered (kind of like the parent). What it most likely means is just including the declaration of the reference along with the other results.

So for something like :

package org.example;
public class Example {
	public void test() {
		String message1 = getMessage();
		String message2 = getMessage();
		String message3 = getMessage();
	}
	public String getMessage () {
		return "some message";
	}
}

finding references of getMessage() returns the 3 from within test() but never includes the getMessage() declaration from below. That's probably what is meant by includeDeclaration so this is a bug.

@rgrunber
Copy link
Contributor

rgrunber commented Aug 2, 2022

@JessicaJHee , this would be a nice first issue if interested.

@JessicaJHee
Copy link
Contributor

I'm interested in working on this issue!

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

Successfully merging a pull request may close this issue.

3 participants