Skip to content

Remove access to CompletionEngine.completionToken #1128

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.

Sign up for GitHub

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

Jump to bottom
Open
datho7561 wants to merge 895 commits into
base: dom-with-javac
Choose a base branch
from
Open

Remove access to CompletionEngine.completionToken #1128

datho7561 wants to merge 895 commits into from

Conversation

@datho7561
Copy link

@datho7561 datho7561 commented Jan 15, 2025

No description provided.

@datho7561 datho7561 mentioned this pull request Jan 15, 2025
Merged
@mickaelistria
Copy link

mickaelistria commented Jan 16, 2025

What does it fix?
IMO, it's better to try using the CompletionEngine as utility class whenever possible rather than duplicating, as long as it's not using the Parser to proceed. Is it the case here?

@datho7561
Copy link
Author

datho7561 commented Jan 16, 2025

We cannot access CompletionEngine.completionToken if we are outside of the codeassist package, due to the current access level of the field. If we want to move the DOMCompletionEngine to the javac fragment, we will either need to change the access level or not access the field.

@robstryker
Copy link

robstryker commented Jan 16, 2025

My thoughts:

  1. We shouldn't be aiming to move DOMCompletionEngine into javac fragment. We should be aiming (hopefully) to contribute it upstream.

  2. Using CompletionEngine rather than duplicating might make sense, but it also might not. If a full decoupling requires copying a very few simple and obvious methods with minimal logic, or if a copied version can be drastically minimized because we know the nested engine hasn't had any state added to it, then the simpler solution of copying and removing unused logic should be preferred.

@akurtakov
Copy link

akurtakov commented Jan 16, 2025

AFAICT the reason to move DomCompletionEngine to javac fragment is to make it possible to use our work with o.e.jdt.core bundle from I-build without modification.
That is a must have for us to make it easier for people to start using it , it really doesn't matter whether it's javac fragment or some other our bundle as it's far from being upstreamable. Bundles coming https://github.com/eclipse-jdt should be perfectly fine for the project to work with.

@datho7561
Copy link
Author

datho7561 commented Jan 20, 2025 •
edited
Loading

After adding @robstryker 's PR, I'm having second thoughts on the implementation he used. We need to subclass InternalCompletionProposal. Why not override the methods that access the completion engine in the subclass? That way we don't need to make changes upstream. Ah I see why, dumb idea, please ignore that

@datho7561 datho7561 force-pushed the dom-with-javac-dont-use-completion-token branch from f29724f to 6336a21 Compare January 20, 2025 21:07
@datho7561
Copy link
Author

datho7561 commented Jan 21, 2025

@mickaelistria if you have some time, could you please take a look at this PR? This is a prerequisite for #1129, since when we move DOMCompletionEngine to the javac fragment we won't be able to access CompletionEngine internals.

@mickaelistria
Copy link

mickaelistria commented Jan 22, 2025

We cannot access CompletionEngine.completionToken if we are outside of the codeassist package, due to the current access level of the field. If we want to move the DOMCompletionEngine to the javac fragment, we will either need to change the access level or not access the field.

Couldn't we just put the DOMCompletionEngine in the same package (even if moved into fragment)? That's already what we're doing for Bindings.

@datho7561
Copy link
Author

datho7561 commented Jan 22, 2025

Couldn't we just put the DOMCompletionEngine in the same package (even if moved into fragment)? That's already what we're doing for Bindings.

You're right; we could do that. However, I still think it's important to decouple from CompletionEngine as much as possible. Many of the methods we are reusing from CompletionEngine are trivial to re-implement (e.g. they return a constant int). Other methods that we are currently reusing from CompletionEngine only work properly if the CompletionParser has been invoked and bunch of internal state has been populated, so we shouldn't be using them if we want correct results. If we rely too much on modifying the internal state of the nested CompletionEngine, the code becomes difficult to read and follow. For instance, I believe it's more clear what's happening when a static helper method calculates a list of completion results given some parameters than when we set a bunch fields on the nested completion engine then invoke a non-static method.

@mickaelistria
Copy link

mickaelistria commented Jan 22, 2025

I think decoupling is eventually nice, but it's not a requirement and it's certainly more work. So IMO, at the moment, let's just use the same package. We may rework the coupling later when needed.

@datho7561
Copy link
Author

datho7561 commented Jan 22, 2025

If we decouple from CompletionEngine now, we prevent adding more coupling to the CompletionEngine in the short term. The more coupling we have, the harder it will be to decouple.

This PR fully decouples from the CompletionEngine; the work is already completed.

@mickaelistria
Copy link

mickaelistria commented Jan 22, 2025

The proposed decoupling requires change in upstream APIs. The methods such as incrementXXX/decrementXXX are not conventional API and I don't think they'll be easily welcome in JDT.
While decoupling is nice, ability to work best with upstream with minimal change upstream is currently more critical.

Rob Stryker and others added 14 commits January 22, 2025 16:29
@datho7561
Reverting removal of suspected typo, was not typo, was part of test
11c5b5f 
Signed-off-by: Rob Stryker <[email protected]>
@testforstephen @datho7561
Cache JCCompilationUnit from compilation job for reuse in problem con…
ab78e53 
…verter (eclipse-jdt#853)
@mickaelistria @datho7561
Fix some javadoc issue mapping
d707682
@mickaelistria @datho7561
Set AccDeprected flag at DOM level
481d7aa 
Fix eclipse-jdtls#394
@datho7561
Fix ClassCastException when working with module projects
fff1d4f 
- Remove `JavacFileObject`, use file objects from file manager

Signed-off-by: David Thompson <[email protected]>
@datho7561
Completion to add override for inherited method
503a26c 
eg. try completion at the `|` for the following cases:

```java
public Foo {
    |
}
```

```java
public Foo {
    toStr|
}
```

Closes eclipse-jdt#859

Signed-off-by: David Thompson <[email protected]>
@datho7561
Improvements to variable and method completion in method bodies
8bf3adf 
- Filter out private variables and methods from parent classes
- Filter out non-static variables and methods when completing inside a
  static method
- Distinguish between field completion and local variable completion
  (these have different icons in Eclipse)
- Set flags (eg. `public`, `protected`, `final`) for the completion items
  (these have different icons in Eclipse)

Fixes eclipse-jdt#861

Signed-off-by: David Thompson <[email protected]>
@datho7561
Handle incomplete qualified names
e8ad2d1 
eg. completion at the `|`

```java
public class HelloWorld {

    public static void main(String... args) {
        HelloWorld.|
    }
}
```

Handles static methods and members, as well as `this`, `super` and `class`.

Signed-off-by: David Thompson <[email protected]>
@datho7561
Completion on super.
1685a5a 
Fixes eclipse-jdt#865

Signed-off-by: David Thompson <[email protected]>
@datho7561
Map a problem id related to super()
6a0fd2a 
Signed-off-by: David Thompson <[email protected]>
@datho7561
Method argument completion
f13a581 
Fill in method arguments with best guesses of the values,
using the existing mechanism.

- Refactor binding collection logic to filter out non-static members
  when applicable

Closes eclipse-jdt#868

Signed-off-by: David Thompson <[email protected]>
@datho7561
Improve annotation completion
45f67d2 
- I broke Gayan's existing support in my previous patches (sorry Gayan), this PR fixes it
- Support completing right after `@`
- Improve annotation parameter completion

Signed-off-by: David Thompson <[email protected]>
@testforstephen @datho7561
Set debug info parameters (-g) to javac compiler
e5e4b5d
@datho7561
Enable completion tests
4ed6116 
- Fix some low hanging completion test errors

Signed-off-by: David Thompson <[email protected]>
@mickaelistria mickaelistria force-pushed the dom-with-javac branch 2 times, most recently from c906abc to b53a413 Compare February 7, 2025 10:49
@robstryker robstryker force-pushed the dom-with-javac branch 2 times, most recently from 08ec5cf to 10fe71a Compare February 7, 2025 20:43
@mickaelistria mickaelistria force-pushed the dom-with-javac branch 2 times, most recently from c2b9da2 to c640bdc Compare February 10, 2025 10:41
@rgrunber rgrunber force-pushed the dom-with-javac branch 3 times, most recently from 43ddedd to 6545d02 Compare March 20, 2025 19:45
@mickaelistria mickaelistria force-pushed the dom-with-javac branch 2 times, most recently from 1a69e37 to 786a635 Compare April 2, 2025 07:42
@robstryker robstryker force-pushed the dom-with-javac branch 3 times, most recently from b1b4e0e to 309f390 Compare September 8, 2025 20:48
@robstryker robstryker force-pushed the dom-with-javac branch 2 times, most recently from 5738537 to 9962d57 Compare December 2, 2025 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mickaelistria mickaelistria Awaiting requested review from mickaelistria

Assignees

@datho7561 datho7561

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@datho7561 @mickaelistria @robstryker @akurtakov @testforstephen @snjeza @rgrunber