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

Access Javac API from a standalone javac jar instead of the default JDK runtime #924

Draft
wants to merge 715 commits into
base: dom-with-javac
Choose a base branch
from

Conversation

testforstephen
Copy link

@testforstephen testforstephen commented Nov 5, 2024

This PR tries to introduce a standalone Javac jar from the repository https://github.com/JaroslavTulach/nb-javac for Javac API access within our project. The benefit is that it allows the language server to support latest language features without requiring user to run the language server with the latest JDK.

This PR is an experiment PR, not full ready yet. It faces some issues with new approach, and I open the PR first for suggestions and feedback.

  • The project can compile fine with the embedded nb-javac, but get the module conflict issue during runtime. It reports module conflicts between jdk.compiler module and unnamed module from the embedded nb-javac jar. When running it in Eclipse, it throws exception below.
java.lang.ClassCastException: class com.sun.tools.javac.api.JavacTool cannot be cast to class com.sun.tools.javac.api.JavacTool (com.sun.tools.javac.api.JavacTool is in module jdk.compiler of loader 'app'; com.sun.tools.javac.api.JavacTool is in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @d014059)
   at org.eclipse.jdt.core.dom.JavacCompilationUnitResolver.parse(JavacCompilationUnitResolver.java:690)
   at org.eclipse.jdt.core.dom.JavacCompilationUnitResolver.toCompilationUnit(JavacCompilationUnitResolver.java:541)
   at org.eclipse.jdt.core.dom.ASTParser.internalCreateASTCached(ASTParser.java:1299)
   at org.eclipse.jdt.core.dom.ASTParser.lambda$1(ASTParser.java:1178)
   at org.eclipse.jdt.internal.core.JavaModelManager.cacheZipFiles(JavaModelManager.java:5692)
   at org.eclipse.jdt.core.dom.ASTParser.internalCreateAST(ASTParser.java:1178)
   at org.eclipse.jdt.core.dom.ASTParser.createAST(ASTParser.java:918)
   at org.eclipse.jdt.internal.core.CompilationUnit.buildStructure(CompilationUnit.java:184)
   at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:246)
   at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:569)
   at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:292)
   at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:278)
   at org.eclipse.jdt.internal.core.CompilationUnit.getSourceRange(CompilationUnit.java:1087)
  • The packaged nb-javac only includes the classes of jdk.compiler, no jdk.javadoc classes. To bypass the compilation error, I temporarily comment out the jdk.javadoc classes reference from JavadocConverter.java.
  • License review. nb-javac is under GPL-2.0 license, require legal review.

@mickaelistria
Copy link

Does the launch configuration or the java command still set the --add-exports ... flag? If so, please make sure they're removed from most places. The META-INF/p2.inf file could probably be removed too.

@testforstephen
Copy link
Author

I removed the --add-exports ... flags from the launch configuration. However, Eclipse automatically adds --add-modules=ALL_SYSTEM to the launch command, which includes the java.compiler module in the runtime by default. This causes the call to ToolProvider.getSystemJavaCompiler() to use the default java.compiler api and return the default JDK compiler instance and its associated jdk.compiler module. Since nb-javac already contains all the classes from both java.compiler and jdk.compiler, we need to find a way to ensure that the nb-javac JAR is prioritized over the JDK module.

@rgrunber
Copy link

rgrunber commented Nov 7, 2024

Found this on ClearlyDefined with a score that makes it usable, https://clearlydefined.io/definitions/sourcearchive/mavencentral/com.dukescript.nbjavac/nb-javac/jdk-19+33 (not useable, License score is what counts, and it's 25, must be at least 60) . I have filed https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/17140 for the particular version here.

Update: It is Approved ✔️

@testforstephen
Copy link
Author

In recent commits, I addressed two issues:

  • The first issue is java.lang.ClassCastException: class com.sun.tools.javac.api.JavacTool cannot be cast to class com.sun.tools.javac.api.JavacTool (com.sun.tools.javac.api.JavacTool is in module jdk.compiler of loader 'app'; com.sun.tools.javac.api.JavacTool is in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @d014059). When launching Eclipse plugins with JDK 9+, most java.* modules (including java.compiler) are loaded as root modules by default, which is not configurable. To workaround the module conflicts, I use com.sun.tools.javac.api.JavacTool.create() to obtain a compiler instance, rather than javax.tools.ToolProvider.getSystemJavaCompiler().

  • The second issue is that nbjavac.VMWrapper fails to resolve ct.sym from the OSGi classloader. The nb-javac library includes wrappers to load JDK resources, relying on the classloader mechanism for this purpose. However, its implementation assumes that NetBeans loads the nbjavac JARs as standard jar dependencies on the classpath. In the Eclipse environment, where the javac JAR is embedded within a bundle, the classloader behaves differently, causing the nbjavac implementation to fail in an OSGi bundle context. To address this, I implemented a workaround to help VMWrapper load ct.sym from the OSGi bundle resources.

image

In the latest commit, the DOM resolver already works with the embedded Javac. But for the build support, it still has some problems because the apt plugin is not loaded well. Here are some potential issues to look further.

  • The one unresolved issue is that org.eclipse.jdt.apt.pluggable.core plugin cannot be compiled well because the module namespace conflicts between the java.compiler and unnamed module for the embedded Javac jar. This occurs because the javac plugin is implemented as a fragment host of the org.eclipse.jdt.core plugin, causing any plugin that depends on jdt.core to automatically include the javac plugin in its dependencies. As a result, the transitive unnamed nbjavac module in Javac plugin conflicts with the java.compiler dependency of the apt plugin. To resolve this, we may need to refactor the alternative compiler API support in AbstractImageBuilder into an extension point. This can make the javac plugin into a standalone plugin, and isolate it from conflicts with other plugins.

image

  • Another potential issue relates to Lombok compatibility. Similar to ECJ, Lombok support can encounter compatibility issues when used with Javac. Just as a new ECJ version might disrupt Lombok functionality, a new Javac release can also break compatibility with Lombok. In jdt.ls, we typically embed the latest lombok.jar and utilize it alongside ECJ. However, with Javac, it defaults to using a project's own Lombok dependency, loading it as a standard annotation processor in Javac. I recently observed that lombok 1.18.34 is not compatible with javac 23 because of new Javadoc syntax. As a result, applications still using Lombok 1.18.34 will encounter resolution errors with Javac 23. In future, we might consider using the embedded Lombok version in jdt.ls for the Javac parsing as well.

@testforstephen
Copy link
Author

testforstephen commented Nov 12, 2024

The one unresolved issue is that org.eclipse.jdt.apt.pluggable.core plugin cannot be compiled well because the module namespace conflicts between the java.compiler and unnamed module for the embedded Javac jar. This occurs because the javac plugin is implemented as a fragment host of the org.eclipse.jdt.core plugin, causing any plugin that depends on jdt.core to automatically include the javac plugin in its dependencies. As a result, the transitive unnamed nbjavac module in Javac plugin conflicts with the java.compiler dependency of the apt plugin. To resolve this, we may need to refactor the alternative compiler API support in AbstractImageBuilder into an extension point. This can make the javac plugin into a standalone plugin, and isolate it from conflicts with other plugins.

I attempted to convert the org.eclipse.jdt.core.javac bundle from a fragment to a plugin and used an extension point to register an alternative Javac compiler with AbstractImageBuilder. The image builder successfully locates and utilizes JavacCompiler. However, JavacCompilationUnitResolver fails because it depends on several package-private classes (e.g., org.eclipse.jdt.core.dom.BindingResolver) from the org.eclipse.jdt.core plugin.

java.lang.IllegalAccessError: class org.eclipse.jdt.core.dom.JavacBindingResolver cannot access its superclass 
org.eclipse.jdt.core.dom.BindingResolver (org.eclipse.jdt.core.dom.JavacBindingResolver is in unnamed module of loader 
org.eclipse.osgi.internal.loader.EquinoxClassLoader @5f618123; org.eclipse.jdt.core.dom.BindingResolver is in unnamed module 
of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @90ba3b1)

I guess this dependency issue is why the javac plugin is set up as a fragment of jdt.core, allowing it to share the same classloader.

@@ -5,7 +5,10 @@ Bundle-SymbolicName: org.eclipse.jdt.core.javac;singleton:=true
Bundle-Version: 1.0.0.qualifier
Fragment-Host: org.eclipse.jdt.core
Automatic-Module-Name: org.eclipse.jdt.core.javac
Require-Capability: osgi.ee; filter:="(&(osgi.ee=JavaSE)(version=23))"
Require-Capability: osgi.ee; filter:="(&(osgi.ee=JavaSE)(version=17))"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://ci.eclipse.org/ls/job/jdt-core-incubator/job/PR-924/3/console , this breaks current compilation since some Java 21 features are already adopted in the code. Would 21 be fine?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it breaks Java 21 usage (e.g. some utility of String and pattern matching). We need to rewrite them with Java 17 compatible way. The purpose here is to make the minimum JDK requirement same as other JDT plugins.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, at this stage, we should minimize the changes in the code and stick to Java 21 to facilitate progress. When we've adopted nb-javac, then we'll be more easily able to move code back to Java 17. Moreover, m2e already requires Java 21, so JDT-LS already requires Java 21, so trying to go below that won't produce any benefit for JDT-LS nor vscode-java.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see redhat.java extension still requires a minimum Java 17 because it stays at m2e@2.6.0. Good to know we're going to upgrade to Java 21.

datho7561 and others added 20 commits November 12, 2024 11:19
- This also fixes the quickfix as a result

eg.

```java
Function<Integer, Integer> func = x -> {
    System.out.println(x);
};
```

becomes

```java
Function<Integer, Integer> func = x -> {
    System.out.println(x);
    return x;
};
```

Signed-off-by: David Thompson <davthomp@redhat.com>
Different versions of Java allow different modifiers on interface methods,
so ECJ generates slightly different problem ids for each of these cases.
Use the compiler settings to determine which to use.
This affects the logic of the quick fixes,
so it should fix some jdt-ls test cases.

Signed-off-by: David Thompson <davthomp@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>

Fix regression

Signed-off-by: Rob Stryker <stryker@redhat.com>

Fix regressions

Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
…compilation unit

Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
datho7561 and others added 10 commits November 14, 2024 13:24
- Do not reuse the context when compiling to multiple output directories
- Implement Alex's idea of a separate file of javac-specific error codes

Signed-off-by: David Thompson <davthomp@redhat.com>
work-in-progress:
- prefix filtering
- see if I can get the binding key bugs resolved

Fixes eclipse-jdt#938

Signed-off-by: David Thompson <davthomp@redhat.com>
In order to accomplish this,
I have changed the logic for adjusting the offset used for the node search.

This causes some regressions:
- A few cases where the test expects the AST not to be recovered but javac does recover it (eg. org.eclipse.jdt.core.tests.model.CompletionTests_1_5.test0197)
- @| in Javadoc is not handled. Before it got no completion due to not finding the correct node, but now it gets default completion, which causes some regressions. I'll fix these when dealing with @| properly
- The test cases sometimes expect the current class to be suggested in new | completion even when the type doesn't match, but sometimes they don't. I'll need to investigate further

Signed-off-by: David Thompson <davthomp@redhat.com>
When resolving, we do only need the symbols/bindings of depedencies.
Minimize the amount of option to skip eg parsing Javadoc for transitive
files that are only relevant as part of binding resolution.
- Both block and inline tags
- Take into account which ones are applicable to the declaration being
  Javadocumented

Fixes eclipse-jdt#948

Signed-off-by: David Thompson <davthomp@redhat.com>
as it's reused on the whole lifecyle of JDT.
* Also do not share the Platform filemanager with build (a bit more
isolation)
* a few other optimization to save CPU on irrelevant operations
Because resolving from .class deps is much faster than resolving from
.java deps
It's most likely useless to get linting when none of those are
requested.
@testforstephen
Copy link
Author

Following the suggestions in eclipse-equinox/equinox#697, I have removed the Require-Bundle for the dependency org.eclipse.core.runtime from org.eclipse.jdt.core and replace it with Import-Package. This change allows the locally embedded javac JAR packages to take precedence over the JRE.

However, one remaining issue with the embedded javac is that it does not include the jdk.javadoc.* packages. To render snippets for Java 23 Markdown Javadoc, we rely on classes from these packages, such as jdk.javadoc.internal.doclets.formats.html.taglets.snippet. Since these classes were introduced in JDK 23 and are missing in the embedded javac JAR, I have commented out this functionality in the PR to ensure the embedded javac passes compilation.

If we choose to support the embedded javac library, we will need to explore alternative approaches to implement snippet support.

@akurtakov
Copy link

IMO it should be reported to netbeans and hear from them whether they are willing to include these packages before making a decision.

@testforstephen
Copy link
Author

IMO it should be reported to netbeans and hear from them whether they are willing to include these packages before making a decision.

jdk.javadoc is typically used by the javadoc tool. Could anyone provide more context to explain why we need to package it to a javac library?

@mickaelistria
Copy link

jdk.javadoc is typically used by the javadoc tool. Could anyone provide more context to explain why we need to package it to a javac library?

Eclipse JDT provides semantic search/reference/bindings/completion... inside Javadoc. In JDT DOM, Javadoc are pretty standard elements and the javadoc needs to be parsed and its symbols resolved too in order to enable all the expected features. The jdk.javadoc package provides some APIs to retrieve the necessary info from JDK and turn it into JDT model.

@testforstephen
Copy link
Author

Eclipse JDT provides semantic search/reference/bindings/completion... inside Javadoc. In JDT DOM, Javadoc are pretty standard elements and the javadoc needs to be parsed and its symbols resolved too in order to enable all the expected features. The jdk.javadoc package provides some APIs to retrieve the necessary info from JDK and turn it into JDT model.

I think you mentioned here is the javadoc comments in source code. The com.sun.source.doctree package in javac (part of the jdk.compiler module) already provides interfaces for parsing these comments. And the existing nb-javac library can handle it as well.

On the other hand, jdk.javadoc module is primarily used by javadoc CLI to generate HTML documentation from the javadoc comments in the source code. This is a separate tool from javac, which is why I doubt to ask the NetBeans to package it to the javac library.

@mickaelistria
Copy link

I had missed on message, sorry. So it looks like the Javadoc snippet feature requires the jdk.compiler module. In practice, I don't think there is a way to provide that without using JDK stuff as AFAIK there is really not any other existing implementation.

@testforstephen
Copy link
Author

it's this PR #812 that uses some classes from jdk.javadoc. I see you use it to parse markdown/html. I believe there is alternative open-source markdown library we can use for this.

@mickaelistria
Copy link

I'm wondering whether it wouldn't be simpler to just ship 1 fragment for each compatible version of Java that to use nb-javac. The only drawback would be that when a newer version of Java appears, we'd have to create a new fragment for it and ensure some compatibility (this can be configured in a build to ensure we use the right Java version); but even that drawback can IMO lead to higher quality on the long run.

@testforstephen
Copy link
Author

I'm wondering whether it wouldn't be simpler to just ship 1 fragment for each compatible version of Java that to use nb-javac. The only drawback would be that when a newer version of Java appears, we'd have to create a new fragment for it and ensure some compatibility (this can be configured in a build to ensure we use the right Java version); but even that drawback can IMO lead to higher quality on the long run.

what benefits can we get from packaging multiple fragments for different java versions? could you explain it more?

@mickaelistria
Copy link

what benefits can we get from packaging multiple fragments for different java versions? could you explain it more?

We could get Javac stuff in JDT-LS running on any current or older version we decide to target.

@testforstephen
Copy link
Author

We could get Javac stuff in JDT-LS running on any current or older version we decide to target.

this can be a way to solve the compatibility issue of the javac plugins such as lombok.

@testforstephen
Copy link
Author

However, one remaining issue with the embedded javac is that it does not include the jdk.javadoc.* packages. To render snippets for Java 23 Markdown Javadoc, we rely on classes from these packages, such as jdk.javadoc.internal.doclets.formats.html.taglets.snippet. Since these classes were introduced in JDK 23 and are missing in the embedded javac JAR, I have commented out this functionality in the PR to ensure the embedded javac passes compilation.

For snippet javadoc issue in the PR #812, this is just a formatting thing. I'm not too worried about its support when we switch to nb-javac. We can implement a simple algorithm to handle it ourselves or reuse the solution of ECJ such as eclipse-jdt#60.

@mickaelistria mickaelistria force-pushed the dom-with-javac branch 2 times, most recently from 7f5c20a to 87832d6 Compare December 6, 2024 13:52
@mickaelistria
Copy link

For the license, here is the hint from EDP handbook: https://www.eclipse.org/projects/handbook/#ip-sbom . We should probably add an about.html mentioning the 3rd party GPLv2+ClassPathExtension license too.

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

Successfully merging this pull request may close these issues.

7 participants