Improve code actions by snjeza · Pull Request #3322 · eclipse-jdtls/eclipse.jdt.ls

snjeza · 2024-11-06T21:24:52Z

Fixes #3321
Requires redhat-developer/vscode-java#3845

I have configured code actions in a similar way Eclipse does it.
Test vsix: https://github.com/snjeza/vscode-test/raw/refs/heads/master/java-1.38.5.vsix

snjeza · 2024-11-07T00:27:39Z

@rgrunber @fbricon @mickaelistria @testforstephen @jdneo @robstryker Please see b5e88fe#diff-56e3932686a1ea3ebee6857099f5fe41a37cb3ef5f85ebceac12df645c0b053aR435
Eclipse shows the Inline Local Constantrefactoring at

public class E {
    public void foo(String[] parameters, int j) {
        int temp = parameters.length + j;
        int |temp1 = temp;
        System.out.println(temp);
    }
}

and the following dialog

snjeza · 2024-11-07T22:30:13Z

Test this please

rgrunber · 2024-11-12T20:09:21Z

 					// Inline Constant (static final field)
 					if (RefactoringAvailabilityTesterCore.isInlineConstantAvailable((IField) varBinding.getJavaElement())) {
 						InlineConstantRefactoring refactoring = new InlineConstantRefactoring(context.getCompilationUnit(), context.getASTRoot(), context.getSelectionOffset(), context.getSelectionLength());
-						if (refactoring != null && refactoring.checkInitialConditions(new NullProgressMonitor()).isOK() && refactoring.getReferences(new NullProgressMonitor(), new RefactoringStatus()).length > 0) {


Nice, so with this, we correctly defer these calculations to resolve right ? I noticed ChangeCorrectionProposalCore.getChange() only gets called from codeAction/resolve. That would definitely improve things both for javac and ecj.

Nice, so with this, we correctly defer these calculations to resolve right

Do you want me to defer Inline Constant, too?
I should change InlineVariableTest.testInlineLocalVariableWithNoReference -

eclipse.jdt.ls/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/refactoring/InlineVariableTest.java

Line 76 in f155e1f

public void testInlineLocalVariableWithNoReferences() throws Exception {

I guess the issue is that checkInitialConditions is what determines whether there are any references to inline, but by defering it to codeAction/resolve, you end up showing the dialog in cases where it would have been hidden. It seems like a small price to pay for improving performance though, and it's not horribly wrong.

I guess the issue is that checkInitialConditions is what determines whether there are any references to inline,

Right.

but by defering it to codeAction/resolve, you end up showing the dialog in cases where it would have been hidden. It seems like a small price to pay for improving performance though, and it's not horribly wrong.

We can solve it in a separate issue. The dialog requires a client update.

snjeza · 2024-11-12T21:08:12Z

@rgrunber @mickaelistria @fbricon @datho7561 You may want to take a look at 5f8662f#diff-270b2ca4a5a67f649246cad6e57d708531a75d91fdadb33c7186f0b02369745eR340
It is related to https://github.com/eclipse-jdtls/eclipse-jdt-core-incubator/blob/89eb53fc0e0736266a036be2173ffea925ce3394/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/ReconcileWorkingCopyOperation.java#L221-L223
javac always creates an AST. Should we always save it?

rgrunber

Looks pretty good overall. Just need to review that "change signature part".

snjeza · 2024-11-12T21:38:49Z

Looks pretty good overall. Just need to review that "change signature part".

You have to use redhat-developer/vscode-java#3845 or try https://github.com/snjeza/vscode-test/raw/refs/heads/master/java-1.38.3.vsix

snjeza · 2024-11-13T19:05:22Z

Test this please.

rgrunber

Overall, looks pretty good. Just one issue I found with the "Introduce Parameter" seeming to permit things that it later denies.

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

snjeza · 2024-11-16T00:07:29Z

test this please

rgrunber · 2024-11-19T17:58:22Z

Just from testing on lemminx with triggering code actions on XMLLanguageServer.initialize()

Before (javac)

[Trace - 12:44:42] Received response 'textDocument/codeAction - (310)' in 2629ms.
[Trace - 12:44:46] Received response 'textDocument/codeAction - (315)' in 2263ms.
[Trace - 12:44:51] Received response 'textDocument/codeAction - (317)' in 2336ms.
[Trace - 12:44:56] Received response 'textDocument/codeAction - (319)' in 2315ms.
[Trace - 12:45:01] Received response 'textDocument/codeAction - (321)' in 2336ms.

After (javac)

[Trace - 12:51:40] Received response 'textDocument/codeAction - (49)' in 99ms.
[Trace - 12:51:43] Received response 'textDocument/codeAction - (54)' in 103ms.
[Trace - 12:51:46] Received response 'textDocument/codeAction - (56)' in 97ms.
[Trace - 12:51:48] Received response 'textDocument/codeAction - (58)' in 129ms.
[Trace - 12:51:50] Received response 'textDocument/codeAction - (60)' in 109ms.

🧑‍💻 ⬅️ 🕶️

rgrunber

Overall, I think this is ready to be merged. It will definitely improve the code action performance, and given that it gets triggered per cursor re-location, it should free up the worker threads for other computations.

@snjeza , what's the situtation with the unit.reconcile(..) issue ? Is there anything to do there at a later time ?

snjeza · 2024-11-19T18:59:22Z

what's the situtation with the unit.reconcile(..) issue ? Is there anything to do there at a later time ?

We can't do anything.

There were changes to no longer provide the `signature` within the code-action command arguments but instead require an additional request from the client. See: - eclipse-jdtls/eclipse.jdt.ls#3322 - redhat-developer/vscode-java#3845 Closes: #743

snjeza requested review from fbricon, jdneo, mickaelistria, robstryker and testforstephen November 6, 2024 21:24

This was referenced Nov 6, 2024

Improve code actions redhat-developer/vscode-java#3845

Merged

Improve code actions #3321

Closed

rgrunber reviewed Nov 6, 2024

View reviewed changes

Comment thread org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java Outdated

snjeza force-pushed the issue-3321 branch 2 times, most recently from 8606a1d to b5e88fe Compare November 7, 2024 00:07

snjeza force-pushed the issue-3321 branch from b5e88fe to cab0b1a Compare November 7, 2024 21:40

snjeza force-pushed the issue-3321 branch from cab0b1a to 5f8662f Compare November 10, 2024 16:33

rgrunber reviewed Nov 12, 2024

View reviewed changes

Comment thread org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/corrections/RefactorProcessor.java

rgrunber reviewed Nov 12, 2024

View reviewed changes

Comment thread ....jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BaseDocumentLifeCycleHandler.java

rgrunber reviewed Nov 12, 2024

View reviewed changes

Comment thread ....jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BaseDocumentLifeCycleHandler.java Outdated

rgrunber reviewed Nov 12, 2024

View reviewed changes

snjeza force-pushed the issue-3321 branch from 5f8662f to 775d8d6 Compare November 13, 2024 18:37

datho7561 mentioned this pull request Nov 14, 2024

Add API for collecting problems without building AST to ICompilationUnitResolver eclipse-jdtls/eclipse-jdt-core-incubator#947

Closed

rgrunber reviewed Nov 15, 2024

View reviewed changes

Improve code actions

98e7a82

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

snjeza force-pushed the issue-3321 branch from 775d8d6 to 98e7a82 Compare November 15, 2024 23:34

rgrunber approved these changes Nov 19, 2024

View reviewed changes

rgrunber merged commit 839831f into eclipse-jdtls:master Nov 19, 2024

rgrunber mentioned this pull request Nov 22, 2024

MalformedTreeException: Overlapping text edits #3330

Open

mfussenegger mentioned this pull request Jan 23, 2025

Update change-signature handling for upstream changes mfussenegger/nvim-jdtls#744

Merged

Conversation

snjeza commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

snjeza commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snjeza commented Nov 7, 2024

Uh oh!

rgrunber Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

snjeza Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

rgrunber Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snjeza Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

snjeza commented Nov 12, 2024

Uh oh!

Uh oh!

rgrunber left a comment

Choose a reason for hiding this comment

Uh oh!

snjeza commented Nov 12, 2024

Uh oh!

snjeza commented Nov 13, 2024

Uh oh!

rgrunber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snjeza commented Nov 16, 2024

Uh oh!

rgrunber commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgrunber left a comment

Choose a reason for hiding this comment

Uh oh!

snjeza commented Nov 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

snjeza commented Nov 6, 2024 •

edited

Loading

snjeza commented Nov 7, 2024 •

edited

Loading

rgrunber Nov 12, 2024 •

edited

Loading

rgrunber commented Nov 19, 2024 •

edited

Loading