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

Incremental delete of duplicate interfaces (IInvocationContextCore and IProblemLocationCore) #1138

Conversation

robstryker
Copy link
Contributor

@robstryker robstryker commented Jan 26, 2024

What it does

There is a massive duplication of interfaces that have appeared as code has moved from jdt.ui to jdt.core.manipulations. Most of these interfaces never had any reason to exist, as the original interfaces could have simply been pushed down to the core plugin directly. This PR is the first step in removing some of the identical and duplicate interfaces.

This PR consists of a single commit above the above-mentioned PR dependency, and it effectively does the following: Removal of internal interfaces IInvocationContextCore and IProblemLocationCore

Both of these interfaces are identically duplicated by IInvocationContext and IProblemLocation respectively. This should be a very painless migration, even for illegal extenders like jdt.ls.

Why this is needed

While pushing code related to the completion subprocessors from jdt.ui to jdt.core.manipulations, one specific (and hefty) subprocessor, LocalCorrectionsSubProcessor, uses a lot of proposals that wrap CleanUp objects, using either the ICleanUp or ICleanUpCore interfaces, as well as the AbstractCleanUp or AbstractCleanUpCore abstract classes.

While identical interfaces are not much of a problem, having two parallel-but-slightly-different abstract classes leads to two parallel heirarchies, and clients can not simply extend both or support both very easily. While trying to untangle these specific issues, the myriad of other duplicate (and otherwise harmless) interfaces that are in the affected code also begin to add complexity to the solution.

With that in mind, removing the most unoffensive interfaces first becomes the safest course of action, while at the same time building our way up to tackling the larger parallel-heirarchy issue via small and manageable changes to the heirarchy as the situation becomes less cluttered.

How to test

Removal of two identically-duplicated internal interfaces should not have much of an affect on the behavior of the codebase at all. The existing test suite should be more than sufficient.

Author checklist

@robstryker robstryker changed the title Incremental delete of duplicate interfaces part1 Incremental delete of duplicate interfaces (IInvocationContextCore and IProblemLocationCore) Jan 26, 2024
@robstryker robstryker force-pushed the IncrementalDeleteOfDuplicateInterfacesPart1 branch 2 times, most recently from 7058105 to 97c1f2c Compare January 30, 2024 20:54
@rgrunber
Copy link
Contributor

The IInvocationContextCore & IProblemLocationCore were created so that JDT-LS could make use of that functionality directly. If they can be removed, and replaced with the "non-core" named versions that are accessible in jdt.core.manipulation, that seems fine. There's a JDT-LS release planned soon, so I might prefer to wait until maybe the end of the week before merging, assuming all is fine.

@robstryker
Copy link
Contributor Author

Inspecting the changed files to see how many changed files have internal in their path results in the following:

[rob@fedora eclipse.jdt.ui] (IncrementalDeleteOfDuplicateInterfacesPart1)$ git diff --stat=1000 HEAD~2 HEAD | grep -v internal | grep -v tests
 org.eclipse.jdt.core.manipulation/.settings/.api_filters                                                                                     |   8 ++++++++
 {org.eclipse.jdt.ui/ui => org.eclipse.jdt.core.manipulation/proposals}/org/eclipse/jdt/ui/text/java/IInvocationContext.java                  |  13 +++----------
 org.eclipse.jdt.core.manipulation/proposals/org/eclipse/jdt/ui/text/java/IProblemLocation.java                                               |  12 +-----------
 org.eclipse.jdt.ui/.settings/.api_filters                 

In short, the only API classes that have been modified are IProblemLocation and IInvocationContext, and the API tools have determined that these interfaces did not in fact change at all.

@jukzi jukzi force-pushed the IncrementalDeleteOfDuplicateInterfacesPart1 branch from 97c1f2c to 88421e5 Compare January 31, 2024 10:53
@jukzi
Copy link
Contributor

jukzi commented Jan 31, 2024

green build now

@robstryker robstryker force-pushed the IncrementalDeleteOfDuplicateInterfacesPart1 branch 2 times, most recently from 5bdf5b3 to fc8c1b3 Compare February 1, 2024 16:15
@rgrunber rgrunber self-requested a review February 1, 2024 20:58
@rgrunber rgrunber added this to the 4.31 M3 milestone Feb 1, 2024
@robstryker robstryker force-pushed the IncrementalDeleteOfDuplicateInterfacesPart1 branch from 8eb7922 to 7b3e049 Compare February 2, 2024 16:33
@robstryker robstryker force-pushed the IncrementalDeleteOfDuplicateInterfacesPart1 branch from 7b3e049 to 6ff28f0 Compare February 6, 2024 15:23
Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Change looks good to me. Just a few comments on some minor things.

org.eclipse.jdt.ui/.settings/.api_filters Show resolved Hide resolved
org.eclipse.jdt.ui/jar-in-jar-loader.zip Outdated Show resolved Hide resolved
Rob Stryker added 2 commits February 7, 2024 11:03
…ationCore

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

Adjust problem filters

Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
@robstryker robstryker force-pushed the IncrementalDeleteOfDuplicateInterfacesPart1 branch from 6ff28f0 to 1b26d3c Compare February 7, 2024 16:05
@rgrunber rgrunber merged commit 9288f3f into eclipse-jdt:master Feb 7, 2024
7 checks passed
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.

3 participants