Handle Java-only build targets #2520

Arthurm1 · 2021-02-16T22:53:46Z

The idea here is to make Metals aware of Java-only (i.e. non-Scala) build targets, which it currently discards.

This would mean that Java targets would appear in the Metals Tree View and the Doctor, and would be included in the clean compile workspace command.

Bloop currently doesn't support buildTarget/JavacOptions endpoint which is needed for this.
There is a Pull Request here, but I'm not sure what the state of it is.
For now I'm faking buildTarget/JavacOptions in scala.meta.internal.metals.ImportedBuild by translating them from the scalacOptions. This can be dropped and the javacOptions endpoint could be used if/when Bloop supports it.
I see Intellij BSP attempts to ask Bloop for buildTarget/JavacOptions and gets an error.

I was hoping for some feedback on whether this is the right approach to take and what Metals is trying to do in the 10 places I've commented with TODO so that I'll know whether to handle java-only targets there as I'm unfamiliar with the majority of the codebase.

There's not much to the PR. Most of it is splitting BuildTargets#allScala into BuildTargets#allScala, BuildTargets#allJava, BuildTargets#allCommon and the fallout from that.

tgodzik

Thanks for the contribution @Arthurm1 ! Sorry it took me so long to get to it.

We ignore java build targets mostly due to the fact that we can't do anything with them currently. We still haven't figured out how to properly interoperate with a Java Language Server. There is a related issue scalameta/metals-feature-requests#5 that we need to handle in order to properly support java projects.

Although, recently @olafurpg worked on a java compiler plugin that can easily produce semanticdb files:

https://github.com/sourcegraph/lsif-java

This would enable us to support references, renames, implementations and better go to definition. To include that plugin in Metals we would most likely make the changes that you made in this PR, so this is I think a good start. I will need to spend some time working on Bloop next couple of weeks, so I can also take a look at the javacOptions endpoint. With that in place we could add the sourcegraph plugin and at least enable the previously mentioned features.

We will still need to figure out how to provide completions, hovers, signature help etc. We could probably use JDT, but I am not sure how to do it properly yet. Alternatively, we could try to start a java language server alongside metals, but I haven't done any research yet in that area.

metals/src/main/scala/scala/meta/internal/metals/FileSystemSemanticdbs.scala

metals/src/main/scala/scala/meta/internal/metals/FileWatcher.scala

metals/src/main/scala/scala/meta/internal/metals/ImportedBuild.scala

metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala

metals/src/main/scala/scala/meta/internal/metals/debug/BuildTargetClasses.scala

metals/src/main/scala/scala/meta/internal/metals/SemanticdbIndexer.scala

tgodzik · 2021-02-19T18:12:27Z

tests/unit/src/main/scala/tests/MetalsTestEnrichments.scala

@@ -97,6 +97,7 @@ object MetalsTestEnrichments {
        libraries.flatMap(_.classpath.entries).map(_.toURI.toString).asJava,
        ""
      )
+      // TODO test javacOptions?


would probably be useful once we start testing

Arthurm1 · 2021-02-20T16:09:54Z

Thanks for reviewing.

In my mind this was an early stage of a lot more stages for Java support. Those next stages are probably beyond my knowledge. The reason I wanted this is that I had a Bloop PR for some level of Gradle Android support. Without Bloop's support for javacOptions I couldn't see the targets in Intellij. This was one way to see the targets in a UI.

Didn't know there was a SemanticDB plugin coming along for Java - that's great news - go Olafur!

I'd eventually like to see more done with build targets e.g. highlighting of source/resource directories in VSCode file explorer, context menus in the file explorer for running tests, finer control over clean/compile per target, target info pages showing dependencies/classpath, new-scala-file (and new java file) restricted to source dirs. But those are all nice-to-haves.

I'll keep the PR as draft until javacOptions is supported. Feel free to say if you think Java support needs to go in a completely different direction.

Arthurm1 · 2021-03-10T17:43:42Z

@tgodzik

I've converted to using JavacOptions (rather than faking them) and failing gracefully if BSP doesn't support it.

I was going to have a look at Bloop to see if it's easy enough to get JavacOptions supported there unless you've already started?

tgodzik · 2021-03-10T18:07:54Z

@tgodzik

I've converted to using JavacOptions (rather than faking them) and failing gracefully if BSP doesn't support it.

I was going to have a look at Bloop to see if it's easy enough to get JavacOptions supported there unless you've already started?

I haven't looked into it yet. I was mostly focused on zinc migration.

Arthurm1 · 2021-03-10T22:58:21Z

I've taken the existing Bloop JavacOptions PR, deployed locally and tested against this PR. All Java-only projects get recognised as build targets. They appear in the "Doctor", get compiled on "Clean compile workspace" and are navigable in the Metals Tree View.

I've also tested against Intellij+BSP. Intellij was fine before but now it doesn't complain that Bloop doesn't support JavacOptions and it doesn't have any issues now that Bloop does support JavacOptions

The Bloop PR looks complete. I didn't need to change any code - it just needed a rebase which only has a single line conflict. I think it's ready to merge post-rebase. I'm not sure what the protocol is for taking another's PR - should I create my own PR with their commit? I asked the author back in Jan about the status and got no response - but I will try again.

tgodzik · 2021-03-11T07:35:31Z

@Arthurm1 Since it's more important now I will contact the author today and try to get it merged. Maybe release 1.4.9 before the zinc changes.

tgodzik · 2021-03-11T10:39:35Z

The Bloop PR is merged, you can use 1.4.8-15-10dc7269 Bloop version in this PR.

Arthurm1 · 2021-03-26T18:13:05Z

@tgodzik
Is the intention to eventually add a javaSemanticDBVersion entry to bloop.settings.json to allow Metals to specify the lsif-java semanticdb plugin in the same way that it can specify the scala plugin, and then Bloop would auto-add the necessary options to the javac options?

tgodzik · 2021-03-26T21:22:33Z

@tgodzik
Is the intention to eventually add a javaSemanticDBVersion entry to bloop.settings.json to allow Metals to specify the lsif-java semanticdb plugin in the same way that it can specify the scala plugin, and then Bloop would auto-add the necessary options to the javac options?

That is is exactly the plan 😅

Arthurm1 · 2021-03-26T21:33:11Z

Has anyone started work on that part already? I thought I might give it a go.

Arthurm1 · 2021-03-29T10:23:05Z

@tgodzik So I went ahead and implemented the javaSemanticDBVersion in bloop.settings.json and it successfully generates semanticdb files.

The problem is that I'm getting weird highlighting in the Java files when I click on a class/interface name...

Note that I clicked on AutoImportsResult but VSCode has highlighted the chars partially to the left of the class - by exactly the length of interface + a space char.

Is this something I have to handle in Metals or would this be the LSIF plugin supplying wrong position range info - I wasn't sure if I should raise an issue with the LSIF project or whether I need to delve into the Metals code

tgodzik · 2021-03-29T11:39:11Z

Has anyone started work on that part already? I thought I might give it a go.

We planned to include it as part of GSoC, but it's fine to implement it before. There will be some work required to also include JDT, so we can focus on that there.

@tgodzik So I went ahead and implemented the javaSemanticDBVersion in bloop.settings.json and it successfully generates semanticdb files.

The problem is that I'm getting weird highlighting in the Java files when I click on a class/interface name...

Note that I clicked on AutoImportsResult but VSCode has highlighted the chars partially to the left of the class - by exactly the length of interface + a space char.

Is this something I have to handle in Metals or would this be the LSIF plugin supplying wrong position range info - I wasn't sure if I should raise an issue with the LSIF project or whether I need to delve into the Metals code

I think that might be the LSIF plugin. You can check the positions by using the metap tool coursier install metap, which can print out semanticdb file and we should be able to see what's going on there.

Thanks for taking a look at that!

olafurpg · 2021-03-29T13:53:47Z

@Arthurm1 That issue has been fixed in the latest SNAPSHOT of semanticdb-javac. We're planning to cut a new release this week so it should not be a problem soon :)

Don't hesitate to report other similar issues to https://github.com/sourcegraph/lsif-java/ We're actively working on the project.

olafurpg · 2021-03-29T13:55:31Z

BTW, the command lsif-java snapshot DIRECTORY_WITH_SEMANTICDB_FILES/ will generate human-readable files in the generated/ directory that you can use to debug position issues like these. The lsif-java cli will soon be installable via coursier install lsif-java

Arthurm1 · 2021-03-29T16:24:28Z

Thanks @tgodzik

I'll be stopping before JDT :-) I just wanted my java targets to appear in Metals and the existence of LSIF plugin was an added bonus - although I'm not entirely sure what parts of functionality the plugin on its own delivers to Metals - references but not code completion? Anything else?

@olafurpg Thanks for fixing - preemptively :-) I'll report any more issues now I know where the Metals/LSIF line is drawn.

tgodzik · 2021-03-29T16:40:46Z

Thanks @tgodzik

I'll be stopping before JDT :-) I just wanted my java targets to appear in Metals and the existence of LSIF plugin was an added bonus - although I'm not entirely sure what parts of functionality the plugin on its own delivers to Metals - references but not code completion? Anything else?

References, implementation, anything that needs to operate on the entire codebase at once. We use semanticdb in those case for indexes.

Arthurm1 · 2021-07-23T15:47:33Z

Linked to scalacenter/bloop/pull/1534

@tgodzik
There's a couple of bits for me to tidy and tests to add but this gets Bloop to add the LSIF plugin and retrieves Java-only build targets.

I was wondering whether you're happy with the way I've split out ScalaTarget into its mini-hierarchy of CommonTarget, ScalaOnlyTarget JavaTarget etc. before I start looking at tests.

Also - I'm not sure where to go next. I assume there has to be some kind of Java equivalent to ScalaPresentationCompiler or can the Java stuff be put in with the ScalaPresentationCompiler? At what point can Metals delegate stuff to the Eclipse JDT library - would that library implement something akin to JavaPresentationCompiler?

tgodzik · 2021-07-23T16:07:11Z

I was wondering whether you're happy with the way I've split out ScalaTarget into its mini-hierarchy of CommonTarget, ScalaOnlyTarget JavaTarget etc. before I start looking at tests.

I will take a look at that, but most likely next week. From what I remember it looked good, but I last looked at it a while ago.

Also - I'm not sure where to go next. I assume there has to be some kind of Java equivalent to ScalaPresentationCompiler or can the Java stuff be put in with the ScalaPresentationCompiler? At what point can Metals delegate stuff to the Eclipse JDT library - would that library implement something akin to JavaPresentationCompiler?

I actually played a bit with it and it seems to use JDT we need to setup OSGI inside Metals, it should be possible, but I haven't managed to make it work properly. It also seems we need to create the entire project hierarchy, which also doesn't seem too straightforward.

That said, JDT has interfaces for completions, formatting etc. which we could use in Metals to at least provide the basic functionalities, so this is for sure good place to start looking.

I was also thinking if we could actually use the Java language server somehow and redirect a subset of queries to it 🤔 - we would most likely need to generate eclipse files (that's what Java LSP uses) and maybe then start the other server inside Metals? This way we would at least have the same functionalities as the current server has and we wouldn't need to implement everything from scratch. We could even set it up as a separate server and use LSP to communicate with it (forward completions, fallback definition etc.) This might actually be more doable than using JDT explicitely, but I am not sure if it will all play nicely.

We could have separate JavaPresentationCompiler that would just start the Java Language Server underneath and we would run sbt eclipse or the gradle/maven equivalent before starting.

This is all just my ideas, we need to check if any of that makes sense. I am kind of tempted to use Java Language Server, because we would not need to reimplement everything ourselves this way, but it's still a lot of work.

tgodzik

I feel like there is too much classes here, could we reduce it to a main trait Target and two subtraits ScalaTarget and JavaTarget, where scala can contain also javac options? I would then use the main trait wherever possible, otherwise I feel like we are not leveraging inheritance enough.

We could even return scalaVersion for JavaTargets, which would be the fallbackScalaVersion, since any scala file in there will be a standalone file.

metals/src/main/scala/scala/meta/internal/metals/BuildTargets.scala

metals/src/main/scala/scala/meta/internal/metals/ScalaOnlyTarget.scala

metals/src/main/scala/scala/meta/internal/metals/JavaTarget.scala

tgodzik · 2021-07-26T14:27:27Z

Alternatively, I would just have a single CommonTarget class with some options like Option[ScalacOptions] etc. and merge them inside the CommonTarget object.

Arthurm1 · 2021-07-28T20:00:16Z

These classes are really just wrappers round the java BSP4J classes and designed to extract info from them.

I've shifted the extraction to MetalsEnrichments and BuildTargets

CommonTarget, JavaTarget and ScalaTarget are now stand alone classes and only created when multiple bits of info are needed from the build target. I've removed the other classes.

tgodzik · 2021-08-06T18:05:10Z

Looks pretty good, let's wait until the Bloop PR merge and test it out again.

When testing it were you able to navigate java files? I wonder if we need to do something on the client side to make it work, but maybe that's enough.

Arthurm1 · 2021-08-06T22:53:29Z

When testing it were you able to navigate java files? I wonder if we need to do something on the client side to make it work, but maybe that's enough.

Yes - in a Java file in VSCode you can now: Ctrl + mouse hover to see declaration, Go to Definition, Find All References and clicking on a symbol highlights any usage of that symbol throughout the same file.
All that only started working after 1f8c0c1 - so not the initial commit - I can squash these if you prefer.

Code completions don't work but I'm assuming that's because there's no presentation compiler. Nor do formatting and re-factoring but just being able to navigate symbols makes using Java files a lot easier.

Error highlighting doesn't highlight the correct range (just highlights the start of the line) but that should be fixed when scalacenter/bloop#1477 is merged

tgodzik

LGTM! There are a view things we can improve on such as adding code lense and renames, but I would rather not have it as part of this PR.

Awesome work! I think we'll be aiming for a release early next year, so we should have time to test everything out.

tgodzik · 2021-12-22T11:27:20Z

It seems that the eclipse is bringing in jars compiled with JDK 11 :/ We might need to go further back.

And there is an issue with sbt tests, I will take a look at those.

Arthurm1 · 2021-12-22T13:40:49Z

The problem seems to be that all transitive dependencies in eclipse jars are defined using ranges. e.g.

<dependency>
  <groupId>org.eclipse.platform</groupId>
  <artifactId>org.eclipse.text</artifactId>
  <version>[3.6.0,4.0.0)</version>
</dependency>

Version 3.9 was the last one compiled under jdk1.8 and then 3.10 was compiled under jdk11. Version 4.0.0 doesn't even exist yet. But because of this range use, the valid jdk1.8 jar is ignored for a later version. So I can't go back to an earlier eclipse version as the transitive dependency will still be jdk11.

As far as I can see - there's no way round this but having to specify the exact version of every transitive dependency that's declared using ranges.
e.g. dependencyOverrides += "org.eclipse.platform" % "org.eclipse.text" % "3.9.0"

Unless you know a better way?

I can see the same issue in the move from 11 -> 17 sometime in the future

lefou · 2021-12-22T13:47:53Z

An explicit version is like an right open range starting at that version. You can try to specify a rage ending at the last version that still supports Java 8.

Arthurm1 · 2021-12-22T14:06:00Z

I'd still have to specify a version for every transitive dependency wouldn't I?

I've only got 1 dependency in the build: "org.eclipse.jdt" % "org.eclipse.jdt.core" % "3.25.0" (3.25.0 is the last jdk8 version), but its pom has...

<dependencies>
    <dependency>
      <groupId>org.eclipse.platform</groupId>
      <artifactId>org.eclipse.core.resources</artifactId>
      <version>[3.14.0,4.0.0)</version>
    </dependency>
    <dependency>
      <groupId>org.eclipse.platform</groupId>
      <artifactId>org.eclipse.core.runtime</artifactId>
      <version>[3.13.0,4.0.0)</version>
    </dependency>
    <dependency>
      <groupId>org.eclipse.platform</groupId>
      <artifactId>org.eclipse.core.filesystem</artifactId>
      <version>[1.7.0,2.0.0)</version>
    </dependency>
    <dependency>
      <groupId>org.eclipse.platform</groupId>
      <artifactId>org.eclipse.text</artifactId>
      <version>[3.6.0,4.0.0)</version>
    </dependency>
    <dependency>
      <groupId>org.eclipse.platform</groupId>
      <artifactId>org.eclipse.team.core</artifactId>
      <version>[3.1.0,4.0.0)</version>
      <optional>true</optional>
    </dependency>
  </dependencies>

Sometime in those versions there's a switch from compiling using jdk8 to jdk11.
e.g. for org.eclipse.text the last jdk8 was version 3.9.0
for org.eclipse.core.filesystem the last jdk8 was version 1.7.500

So I'd have to work out what the last version compiled for jdk8 was for all of these jars, specify it and then look at all their dependencies and so on until no more ranges. Or am I missing something here? I've not used ranges before.

lefou · 2021-12-22T14:12:34Z

As far a I know, Eclipse 4.16 was the last release which was built for Java 8. The probably easiest way to figure out which versions belong to it, is to download a Eclipse 4.16 release zip and look what jars are into it. But yes, I think there is no other easy way to select compatible versions, as this fact is not a property of the dependency artifacts.

lefou · 2021-12-22T14:21:45Z

I once compiled a list of artifacts which belong to Eclipse 4.16 to use in some other closed projects.Don't know if it is of any help to you: https://gist.github.com/lefou/71b574af75c1b42653b1be8440561a8b#file-build_eclipse_bundles-sc

tgodzik · 2021-12-22T15:42:47Z

build.sbt

@@ -112,6 +112,23 @@ inThisBuild(
    resolvers += Resolver.sonatypeRepo("public"),
    resolvers += Resolver.sonatypeRepo("snapshot"),
    dependencyOverrides += V.guava,
+    dependencyOverrides += "org.eclipse.platform" % "org.eclipse.ant.core" % "3.5.500",


Uff, this is a lot, thanks for compiling the list!

I've had more fun afternoons

If possible can we add a note here as to why we are doing this? I can see later on someone jumping in here and looking at this and wondering what in the world we are doing here with all these overrides.

I was thinking of moving it later to a separate file and commenting. This PR is pretty huge and I don't want to add anything more right now.

Also, I think you did another comment and I can't find it 😅 which is a good indicator that this got way to big. What do you think? Is it ok to do same follow ups later?

:) of course, a follow-up is totally fine.

For the other comment 😆 I deleted it. A couple weeks ago I started reviewing this and only got like half way through and didn't realize I had draft comments saved, so when I added this comment it also posted those. They aren't relevant anymore, so I just removed them. Sorry for the noise!

tgodzik · 2021-12-22T19:22:34Z

metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala

+    val resultOnJavacOptionsUnsupported = new JavacOptionsResult(
+      List.empty[JavacOptionsItem].asJava
+    )
+    if (isSbt) Future.successful(resultOnJavacOptionsUnsupported)


@Arthurm1 I added this check since it seems that sbt shuts down when unexpected request is made.

tgodzik · 2021-12-22T20:36:00Z

@Arthurm1 It's all green! feel free to merge, we can improve things further later on.

Arthurm1 · 2021-12-22T20:39:39Z

What's the standard - should I squash or ...?

ckipp01

Incredible work @Arthurm1 🚀

kpodsiad · 2021-12-23T10:40:16Z

Wow, this is magnificent @Arthurm1!

tgodzik reviewed Feb 19, 2021

View reviewed changes

gravelld mentioned this pull request Apr 13, 2021

"No build target" when importing a Gradle multi module build #2704

Closed

Arthurm1 force-pushed the java_targets branch from 74c7619 to 4bd2bdb Compare July 23, 2021 15:36

Arthurm1 mentioned this pull request Jul 25, 2021

Support Java semanticDB plugin scalacenter/bloop#1534

Merged

tgodzik reviewed Jul 26, 2021

View reviewed changes

Arthurm1 force-pushed the java_targets branch from 97a9486 to 1f8c0c1 Compare July 27, 2021 14:49

tgodzik mentioned this pull request Aug 18, 2021

Interoperability with Java language servers scalameta/metals-feature-requests#5

Open

Arthurm1 mentioned this pull request Aug 30, 2021

Display build target info #3077

Closed

tgodzik mentioned this pull request Aug 30, 2021

Search for symbol references in workspace #2920

Merged

Arthurm1 added 6 commits December 21, 2021 13:47

Initial Java support

4bf5efe

Java handling fixes

4dacf29

Java handling fixes

bdc4dab

Fix test failures

113013f

Fix scalafix import ordering issue

18961e4

Add exports to CI for jdk 17 + semantic plugin

69d9e50

Arthurm1 force-pushed the java_targets branch from 171a7f5 to 69d9e50 Compare December 21, 2021 14:53

Merge branch 'main' into java_targets

cc3acd9

tgodzik approved these changes Dec 21, 2021

View reviewed changes

hardcode eclipse transitive dependency versions

a4a681e

tgodzik reviewed Dec 22, 2021

View reviewed changes

Skipp asking about javacOptions when using sbt via BSP

675816a

tgodzik reviewed Dec 22, 2021

View reviewed changes

ckipp01 approved these changes Dec 23, 2021

View reviewed changes

Arthurm1 merged commit 11c7c27 into scalameta:main Dec 23, 2021

Arthurm1 deleted the java_targets branch December 23, 2021 10:35

ayoub-benali mentioned this pull request Dec 23, 2021

Add new-java-file command scalameta/metals-sublime#77

Closed

This was referenced Dec 23, 2021

Add in new-java-file command scalameta/nvim-metals#307

Closed

Java support status placeholder #3398

Closed

ayoub-benali mentioned this pull request Dec 24, 2021

Metals snapshot not starting on java 8 #3402

Closed

ckipp01 mentioned this pull request Dec 25, 2021

feat: java formatting and new java file command scalameta/nvim-metals#308

Merged

Handle Java-only build targets #2520

Handle Java-only build targets #2520

Conversation

Arthurm1 commented Feb 16, 2021

tgodzik left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arthurm1 commented Feb 20, 2021

Arthurm1 commented Mar 10, 2021

tgodzik commented Mar 10, 2021

Arthurm1 commented Mar 10, 2021

tgodzik commented Mar 11, 2021

tgodzik commented Mar 11, 2021

Arthurm1 commented Mar 26, 2021

tgodzik commented Mar 26, 2021

Arthurm1 commented Mar 26, 2021

Arthurm1 commented Mar 29, 2021

tgodzik commented Mar 29, 2021

olafurpg commented Mar 29, 2021

olafurpg commented Mar 29, 2021

Arthurm1 commented Mar 29, 2021

tgodzik commented Mar 29, 2021

Arthurm1 commented Jul 23, 2021

tgodzik commented Jul 23, 2021

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik commented Jul 26, 2021

Arthurm1 commented Jul 28, 2021

tgodzik commented Aug 6, 2021

Arthurm1 commented Aug 6, 2021

tgodzik left a comment

Choose a reason for hiding this comment

tgodzik commented Dec 22, 2021

Arthurm1 commented Dec 22, 2021

lefou commented Dec 22, 2021 • edited Loading

Arthurm1 commented Dec 22, 2021

lefou commented Dec 22, 2021

lefou commented Dec 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik commented Dec 22, 2021

Arthurm1 commented Dec 22, 2021

ckipp01 left a comment

Choose a reason for hiding this comment

kpodsiad commented Dec 23, 2021

tgodzik left a comment •

edited

Loading

lefou commented Dec 22, 2021 •

edited

Loading