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

[WIP] Use some javac classes #1234

Closed
wants to merge 3 commits into from
Closed

Conversation

rgrunber
Copy link
Contributor

See #1230 . @mickaelistria wishes to test the provided Jenkinsfile changes.

This requires to start the VM with ```
--add-opens java.base/java.lang=ALL-UNNAMED
--add-opens jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED --add-opens jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED --add-opens jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED --add-opens jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED

settings.

Support for newer Java constructs requires to have that Java version as
BREE.

<!--
Thank you for your Pull Request. Please provide a description and review
the requirements below.

Contributors guide: https://github.com/eclipse-jdt/.github/blob/main/CONTRIBUTING.md

Note: Security vulnerabilities should not be disclosed on GitHub, through a PR or any
other means. TODO: how to report security issues
-->

## What it does
<!-- Include relevant issues and describe how they are addressed. -->

## How to test
<!-- Explain how a reviewer can reproduce a bug, test new functionality or verify performance improvements. -->

## Author checklist

- [ ] I have thoroughly tested my changes
- [ ] The change is following the [coding conventions](https://wiki.eclipse.org/Platform/How_to_Contribute#Coding_Conventions)
- [ ] I have signed the [Eclipse Contributor Agreement (ECA)](https://www.eclipse.org/legal/ECA.php)

@iloveeclipse
Copy link
Member

I've rebased to get Jenkins fix from #1232

@mickaelistria
Copy link
Contributor

@rgrunber I've fixed a bunch of things I could reproduce locally, can you please update it with latest content from #1230 to see how CI will enjoy it?

@mickaelistria
Copy link
Contributor

@rgrunber Thanks, things are getting better. I have pushed another fix for the API Tools error. Can you please retry? (I'm sorry for this annoying workflow!)

Configures project and build so internal modules can be used.
@iloveeclipse
Copy link
Member

Looking on code changes, I wonder what is the point in changing constant names used? I don't see any other changes here beside some formatting.
I would understand if some logic implemented in JDK would be reused, but changing opcode constant names that have same values and require manual update anyway with almost every class file spec change?

@mickaelistria
Copy link
Contributor

Preamble: The goal of this PR is not (yet) to create a lot of value by itself, but more to be a basis for further discussions and development.

I wonder what is the point in changing constant names used?

At the moment, the point of this PR is to show that how to consume some JDK compiler code, and to show that there is some code JDT can just reuse there instead of redefining and maintaining itself, and under which constraints. Symbols are the most obvious candidate; they already allow to get rid of ~200 lines of code in JDT compiler.

I would understand if some logic implemented in JDK would be reused,

So far, I didn't audit JDT's and JDK's code to find all possible improvement. At the moment, I'm just trying to showcase how JDT can use JDK compiler code and which constraints it adds.

I think your comment is already a few steps ahead of the current state of this experiment. And that's enthusiasming ;) Are you already aware of other JDK code that would be great and relatively easy to reuse in JDT? If it's something I can as a add next commit to this PR to showcase more benefits, I will happily try.

@iloveeclipse
Copy link
Member

they already allow to get rid of ~200 lines of code in JDT compiler.

Not sure I saw this in the current PR, where most of changes were literal replacements of A with B constants.

Are you already aware of other JDK code that would be great and relatively easy to reuse in JDT?

No, I haven't worked much on compiler internals. But @srikanth-sankaran did that.
He is mostly offline now, but may be next week he could comment on that.

May be @stephan-herrmann could also give some insights, as he is constantly fighting with ecj vs javac differences in JLS interpretation.

Most of the code I touched did not had any public JDK interfaces, like CtSym, and Oracle also explicitly don't want to make it official API, which is sad.

@mickaelistria
Copy link
Contributor

Not sure I saw this in the current PR

File removals are easily missed in non-trivial PRs.
Screenshot from 2023-07-20 09-45-47

Most of the code I touched did not had any public JDK interfaces

That's indeed one drawback with consuming JDK's classes. I hope this PR will allow to make most benefits and drawback possible so that we will all be able discuss with concrete examples whether it can still be profitable despite the drawbacks. This will for sure depend on the constraints or goals of everyone... But again I'm trying to make it regression-free before bringing it for discussion.

@mickaelistria
Copy link
Contributor

@rgrunber I think I fixed the reported failures in #1230, can you please sync your PR with it?

@stephan-herrmann
Copy link
Contributor

Not sure I saw this in the current PR

File removals are easily missed in non-trivial PRs. Screenshot from 2023-07-20 09-45-47

In the history of that file (Opcodes.java) I see exactly one relevant change (a one-line addition) since its inception in 2001.

Compared to the mountain of complexity inherent in ecj, you are trying to take away one grain of sand. The effort of performing this change may be greater than what we might gain by it.

May be @stephan-herrmann could also give some insights, as he is constantly fighting with ecj vs javac differences in JLS interpretation.

The problem is not of any concise little algorithm, that could be plugged in and out. Much of the trouble comes from discrepancies between javac and https://docs.oracle.com/javase/specs/jls/se20/html/jls-18.html. I could point you to many bugs (JDK and JDT) where such things are discussed, but most of them have stalled, because the best experts on both sides together have not found a solution. I don't even know where to start explaining the incompatibilities, even already at the strategic level. I don't expect even a single 5-line method from JDK to be useful in ecj.

TL;DR: I seriously disbelieve that plugging any snippet of functionality from JDK can potentially benefit ecj. If you want to convince me, better not ask me until you can demonstrate substantial benefit.

@iloveeclipse iloveeclipse marked this pull request as draft July 20, 2023 14:10
@iloveeclipse
Copy link
Member

Mickael, I don't want to veto this PR so I've converted it to a draft.
Note, JDT requires Java 17 as minimal runtime. There is no reason known yet to move to minimum Java 20, definitely not because of this PR as of today.

I see it more as an experiment so far.

@mickaelistria
Copy link
Contributor

I see it more as an experiment so far.

Indeed, it is. A draft is best for this.

@srikanth-sankaran
Copy link
Contributor

I vehemently agree with @stephan-herrmann.

@mickaelistria
Copy link
Contributor

I'm still investigating a few things, like replacing the code generation from JDT by Javac one (so we keep the better parser we have in JDT but can rely on Javac for other phases where JDT doesn't require to offer more value). I will soon share an example.

Let's just see all that work as a preleminary experiment before https://www.youtube.com/watch?v=pcg-E_qyMOI and https://openjdk.org/jeps/8280389 ; the arguments presented about why a standard Classfile API do really match some of the challenges of current JDT development when it comes to coping with Java releases.

@mickaelistria
Copy link
Contributor

This one can be closed, there are much more advanced PRs on this topic.

@akurtakov
Copy link
Contributor

Closing as per Mickael's request.

@akurtakov akurtakov closed this May 16, 2024
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.

6 participants