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

Wrongly provides Go to definition results to java project #1393

Open
stefanrybacki opened this issue May 10, 2023 · 15 comments
Open

Wrongly provides Go to definition results to java project #1393

stefanrybacki opened this issue May 10, 2023 · 15 comments

Comments

@stefanrybacki
Copy link

stefanrybacki commented May 10, 2023

Describe the bug

The Metals Scala extension is used in "Go to definition" in a java project, which leads to not showing the actual definition used in e.g., a call of a method but all methods that match the name inside the project.

To Reproduce Steps to reproduce the behavior:

  1. Install Java language server extension (redhat)
  2. Install Metals Extension
  3. Create java project
  4. Create java class
class GoToDefinitionTest {
  public String method1(String param) {
    return method1(param, null); // Ctrl+Click on method1 here
  }

  public String method1(String param, String otherParam) {
    return method1(param, otherParam, null);
  }

  public String method1(String param, String otherParam, String yetAnotherParam) {
    return "Hello";
  }
}
  1. Ctrl+click or use Go to definition from the context menu on the method1 call indicated via source comment
  2. see definitions provided by metals rather than java extension (see screenshots below)

Expected behavior

The java language server extension provides the information for "Go to Definition" for the java project.

Screenshots

Here you can see the definition info provided apparently by the metals extension (see next screenshot for metals log)
image

Here you can see metals provides the information for method1 Go to
image

Installation:

  • Operating system: Windows UI, Remote SSH Linux
  • VSCode version: 1.78.1
  • VSCode extension version: v1.23.0
  • Metals version: 0.11.12

Additional context

see reported issue in the java extension for vscode redhat-developer/vscode-java#3095

also if I downgrade to v1.21.0 of the metals extension it seems to work as expected.

Search terms

@circlesmiler
Copy link

Same on MacOS.

@tgodzik
Copy link
Contributor

tgodzik commented May 11, 2023

Thanks for reporting! I think there are two issues here:

  • Metals and Java extension do not work well together. It's best to use Java extension for Java only code and Metals for mixed. That should give the best results, since Java extension doesn't have any idea about Scala and things might easily break. Metals Java support is limited though.
  • there is an error compiling Java files with Metals, which translates to Metals trying to guess the location of the symbol. We need compiled code at least once to offer full support.

Could you post the second issue?

@stefanrybacki
Copy link
Author

@tgodzik I am sorry I am not following. Yes we do have scala projects within a multi module gradle project, however the behaviour occurs in a pure java module/project. The second issue you talk about, I assume is the error in the screenshot showing the metals log? The error is: error: plug-in not found: semanticdb

@tgodzik
Copy link
Contributor

tgodzik commented May 11, 2023

This is not an open source repo by any chance? The semanticdb plugin should be downloaded and added automatically. You can run the doctor and click on the module to see how javac options are defined. Maybe we can see what is going on exactly.

As a workaround you could turn off Metals while working on a pure Java module.

@tgodzik
Copy link
Contributor

tgodzik commented May 11, 2023

You can also generate report via Metals command and send it to us. More details https://scalameta.org/metals/blog/#introducing-error-reports

@stefanrybacki
Copy link
Author

@tgodzik sorry this is not an open source repository. I believe the semanticdb error is not really a problem.
image
What puzzles me is why is metals at all providing "Go to definition" functionality to a java project and also why does it not do this in v1.21.0?

@tgodzik
Copy link
Contributor

tgodzik commented May 12, 2023

What puzzles me is why is metals at all providing "Go to definition" functionality to a java project and also why does it not do this in v1.21.0?

With 1.21.0 we introduced a heuristic to offer definition candidates if we failed to find any. So this is a last chance for providing something to the user. Without compilation we don't know which symbol this actually is, which is why we try to guess by name.

This doesn't work nicely if there is a chance that a Java extension might be used otherwise for the same task.

I believe the semanticdb error is not really a problem.

The compilation fails because we are missing semanticdb compiler plugin in the classpath while also using an option for it. Metals doctor only checks for the existance of the proper options. We should most likely also make sure the classpath contains correct artifacts.

@stefanrybacki
Copy link
Author

@tgodzik again, thank you for your explanation

With 1.21.0 we introduced a heuristic to offer definition candidates if we failed to find any. So this is a last chance for providing something to the user. Without compilation we don't know which symbol this actually is, which is why we try to guess by name.

So is there an option to disable that heuristic for java projects, or disable assist for java projects at all? I do not want to use Metals for java projects, I also do not want to enable/disable metals whenever I switch between files, as I work on java projects and scala projects at the same time.

@tgodzik
Copy link
Contributor

tgodzik commented May 17, 2023

Maybe setting File: Associations would help. Something along the lines of:

Screenshot from 2023-05-17 18-58-48

https://www.youtube.com/watch?v=4wTYV4Zjq5g&t=13s&ab_channel=Code2020

Let me know if that works!

@circlesmiler
Copy link

@tgodzik Your suggestion does not work as you can not provide the plugin id as the associated value. You have to provide a language identifier. But there is no special "redhat.java" language identifier.

I would also suggest to provide an option for disabling java support in the metals plugin.

@tgodzik
Copy link
Contributor

tgodzik commented Jun 1, 2023

@tgodzik Your suggestion does not work as you can not provide the plugin id as the associated value. You have to provide a language identifier. But there is no special "redhat.java" language identifier.

Ach, I hoped that would work. Though, when checking locally I do get definition from both servers. I guess this is only a problem if they point to the same location and the results get joined.

I would also suggest to provide an option for disabling java support in the metals plugin.

I am hesitant to do that with a special settings since this seems very much editor specific and should be the responsibility of the editor 🤔

@tgodzik
Copy link
Contributor

tgodzik commented Jun 1, 2023

Also there might be an issue when Java code points to a Scala one and the Java plugin might not be able to handle that as it only works with Java code. So I think having a separate setting for just turning off would not be a full solution and just a stop gap measure.

Ideally, if Metals would know there are some other extensions that provide support for a file, we could not do the fallback 🤔

I need to get back to it once I have some time.

@tgodzik
Copy link
Contributor

tgodzik commented Jun 1, 2023

@tanishiking suggested that we can us perhaps https://stackoverflow.com/questions/73059687/how-to-obtain-a-list-of-installed-vs-code-extensions-using-code to check if Redhat plugin is active

on the other hand (since not sure if we can check if it's active) @dos65 suggested:

There is vscode.ExtensionContext.globalState.setKeyForSync 
In theory, we can try to exchange data over it between redhat and metals to show warnings that you have to disable one of plugins 

@stefanrybacki
Copy link
Author

do you plan to pursue this further. Just asking no judgement.

@tgodzik
Copy link
Contributor

tgodzik commented Sep 20, 2023

Unfortunately, I don't really have the time to fix this, but I would welcome any contributions or ideas.

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

No branches or pull requests

3 participants