Improve code actions #3322

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

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

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

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

@@ -334,15 +398,27 @@ private boolean getInlineProposal(IInvocationContext context, ASTNode node, Coll
 					// 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.

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

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

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?

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

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.

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/RefactorProposalUtility.java

...se.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/ChangeSignatureInfoHandler.java

...dt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/RefactorProposalUtility.java

Signed-off-by: Snjezana Peco <[email protected]>

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.

snjeza requested review from fbricon, mickaelistria, robstryker, jdneo 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

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java Outdated Show resolved Hide resolved

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

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/corrections/RefactorProcessor.java Show resolved Hide resolved

rgrunber reviewed Nov 12, 2024

View reviewed changes

....jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BaseDocumentLifeCycleHandler.java Show resolved Hide resolved

rgrunber reviewed Nov 12, 2024

View reviewed changes

....jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/BaseDocumentLifeCycleHandler.java Outdated Show resolved Hide resolved

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 <[email protected]>

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
6 of 7 checks passed

rgrunber mentioned this pull request Nov 22, 2024

MalformedTreeException: Overlapping text edits #3330

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve code actions #3322

Improve code actions #3322

snjeza commented Nov 6, 2024 •

edited

Loading

snjeza commented Nov 7, 2024 •

edited

Loading

snjeza commented Nov 7, 2024

rgrunber Nov 12, 2024

snjeza Nov 12, 2024

rgrunber Nov 12, 2024 •

edited

Loading

snjeza Nov 12, 2024

snjeza commented Nov 12, 2024

rgrunber left a comment

snjeza commented Nov 12, 2024

snjeza commented Nov 13, 2024

rgrunber left a comment

snjeza commented Nov 16, 2024

rgrunber commented Nov 19, 2024 •

edited

Loading

rgrunber left a comment

snjeza commented Nov 19, 2024

Improve code actions #3322

Improve code actions #3322

Conversation

snjeza commented Nov 6, 2024 • edited Loading

snjeza commented Nov 7, 2024 • edited Loading

snjeza commented Nov 7, 2024

rgrunber Nov 12, 2024

Choose a reason for hiding this comment

snjeza Nov 12, 2024

Choose a reason for hiding this comment

rgrunber Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

snjeza Nov 12, 2024

Choose a reason for hiding this comment

snjeza commented Nov 12, 2024

rgrunber left a comment

Choose a reason for hiding this comment

snjeza commented Nov 12, 2024

snjeza commented Nov 13, 2024

rgrunber left a comment

Choose a reason for hiding this comment

snjeza commented Nov 16, 2024

rgrunber commented Nov 19, 2024 • edited Loading

rgrunber left a comment

Choose a reason for hiding this comment

snjeza commented Nov 19, 2024

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