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

Take advantage of CodeActionLiteral client capability #1256

Merged
merged 1 commit into from
Nov 13, 2019
Merged

Take advantage of CodeActionLiteral client capability #1256

merged 1 commit into from
Nov 13, 2019

Conversation

bstaletic
Copy link
Contributor

@bstaletic bstaletic commented Nov 9, 2019

If client advertises CodeActionLiteralSupport, using that for
java.apply.workspaceEdit would allow clients to use a generic
algorithm, instead of being forced to provide a special case for jdt.ls.

Fixes #376

This actually tries to revive #1059. Differences compared to #1059:

  • Rebased onto latest master
  • Checking isSupportedCodeActionLiteral() instead of isWorkspaceApplyEditSupported().
    • This made me add isSupportedCodeActionLiteral() and I took the chance to fix the docstring from a previous faulty copy/paste.

@eclipse-ls-bot
Copy link

Can one of the admins verify this patch?

@bstaletic
Copy link
Contributor Author

Done. I just forgot to commit the test.

@snjeza
Copy link
Contributor

snjeza commented Nov 9, 2019

There are compile error.

mvn clean verify
...
Reactor Summary for JDT Language Server :: Parent 0.46.0-SNAPSHOT:
[INFO] 
[INFO] JDT Language Server :: Parent ...................... SUCCESS [  0.097 s]
[INFO] JDT Language Server :: Target Platform ............. SUCCESS [  2.648 s]
[INFO] JDT Language Server :: Core ........................ FAILURE [  9.477 s]
[INFO] JDT Language Server :: Tests ....................... SKIPPED
[INFO] JDT Language Server :: Product ..................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  02:18 min
[INFO] Finished at: 2019-11-09T21:55:59+01:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:1.5.1:compile (default-compile) on project org.eclipse.jdt.ls.core: Compilation failure: Compilation failure: 
[ERROR] /home/snjeza/projects/eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java:[208] 
[ERROR] 	codeAction.setEdit(command.arguments[0]);
[ERROR] 	                           ^^^^^^^^^
[ERROR] The field Command.arguments is not visible
[ERROR] /home/snjeza/projects/eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/SourceAssistProcessor.java:[356] 
[ERROR] 	codeAction.setEdit(edit);
[ERROR] 	           ^^^^^^^
[ERROR] The method setEdit(WorkspaceEdit) in the type CodeAction is not applicable for the arguments (TextEdit)
[ERROR] /home/snjeza/projects/eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/SourceAssistProcessor.java:[358] 
[ERROR] 	codeAction.setCommand(workspaceEdit);
[ERROR] 	           ^^^^^^^^^^
[ERROR] The method setCommand(Command) in the type CodeAction is not applicable for the arguments (WorkspaceEdit)
[ERROR] 3 problems (3 errors)
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :org.eclipse.jdt.ls.core

@bstaletic
Copy link
Contributor Author

The build errors should be fixed, but I keep getting:

[ERROR] An error occurred while transferring artifact canonical: osgi.bundle,org.eclipse.m2e.maven.runtime,1.14.0.20191029-1323 from repository https://downloa
d.eclipse.org/technology/m2e/milestones/1.14/1.14.0.20191029-1325:
[ERROR]    Unable to read repository at https://download.eclipse.org/technology/m2e/milestones/1.14/1.14.0.20191029-1325/plugins/org.eclipse.m2e.maven.runtime_
1.14.0.20191029-1323.jar.
[ERROR] Internal error: org.eclipse.tycho.repository.local.MirroringArtifactProvider$MirroringFailedException: Could not mirror artifact osgi.bundle,org.eclips
e.m2e.maven.runtime,1.14.0.20191029-1323 into the local Maven repository.See log output for details. Read timed out -> [Help 1]
org.apache.maven.InternalErrorException: Internal error: org.eclipse.tycho.repository.local.MirroringArtifactProvider$MirroringFailedException: Could not mirro
r artifact osgi.bundle,org.eclipse.m2e.maven.runtime,1.14.0.20191029-1323 into the local Maven repository.See log output for details.
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:120)

whenever I try to build jdt.ls.

@snjeza
Copy link
Contributor

snjeza commented Nov 9, 2019

The build errors should be fixed

There is still one compiler error:

ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:1.5.1:compile (default-compile) on project org.eclipse.jdt.ls.core: Compilation failure: Compilation failure: 
[ERROR] /home/snjeza/projects/eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java:[208] 
[ERROR] 	codeAction.setEdit(command.getArguments()[0]);
[ERROR] 	                   ^^^^^^^^^^^^^^^^^^^^^^^^^
[ERROR] The type of the expression must be an array type but it resolved to List<Object>
[ERROR] 1 problem (1 error)
[ERROR] -> [Help 1]

@snjeza
Copy link
Contributor

snjeza commented Nov 9, 2019

but I keep getting:
...
whenever I try to build jdt.ls.

I can't reproduce it.

@bstaletic
Copy link
Contributor Author

There is still one compiler error:

Fixed, hopefully.

@snjeza
Copy link
Contributor

snjeza commented Nov 10, 2019

Fixed, hopefully.

It is the same. You have to run:

mvn clean verify

before pushing the PR.

@bstaletic
Copy link
Contributor Author

mvn clean verify

I know I should, but like I mentioned, I had a problem with downloading a .jar. Now that I can compile jdt.ls, I ran the tests and they are passing.

@snjeza snjeza requested review from fbricon and snjeza and removed request for snjeza November 10, 2019 02:07
@snjeza
Copy link
Contributor

snjeza commented Nov 10, 2019

It works for me, but someone else must approve.

Copy link
Contributor

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

I'm OK with changes. One comment is to add the test cases into CodeActionHandlerTest.java

If client advertises `CodeActionLiteralSupport`, using that for
`java.apply.workspaceEdit` would allow clients to use a generic
algorithm, instead of being forced to provide a special case for jdt.ls.

Fixes #376

Signed-off-by: Boris Staletic <boris.staletic@gmail.com>
@fbricon fbricon merged commit 2a2e09b into eclipse-jdtls:master Nov 13, 2019
@fbricon
Copy link
Contributor

fbricon commented Nov 13, 2019

@bstaletic thanks for your contribution, keep 'em coming!

@fbricon
Copy link
Contributor

fbricon commented Nov 13, 2019

Since the PR was approved by 2 devs, I merged it before actually testing it myself. Big mistake! 1st time I open a file in vscode I get no code actions and see this in the log:

java.util.concurrent.CompletionException: java.lang.ClassCastException: class java.lang.String cannot be cast to class org.eclipse.lsp4j.WorkspaceEdit (java.lang.String is in module java.base of loader 'bootstrap'; org.eclipse.lsp4j.WorkspaceEdit is in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @56303b57)
	at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:314)
	at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:319)
	at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:645)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:177)
Caused by: java.lang.ClassCastException: class java.lang.String cannot be cast to class org.eclipse.lsp4j.WorkspaceEdit (java.lang.String is in module java.base of loader 'bootstrap'; org.eclipse.lsp4j.WorkspaceEdit is in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @56303b57)
	at org.eclipse.jdt.ls.core.internal.handlers.CodeActionHandler.getCodeActionFromProposal(CodeActionHandler.java:179)
	at org.eclipse.jdt.ls.core.internal.handlers.CodeActionHandler.getCodeActionCommands(CodeActionHandler.java:137)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$14(JDTLanguageServer.java:605)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$45(JDTLanguageServer.java:936)
	at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:642)
	... 6 more

While debugging https://github.com/eclipse/eclipse.jdt.ls/blob/2a2e09bf241e0834782052374379b2029a3726ff/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java#L179

I could see that command.getArguments() resolves to:

[extractConstant, CodeActionParams [
  textDocument = TextDocumentIdentifier [
    uri = "file:///Users/fbricon/Dev/quarkus-projects/graduh/src/main/java/foo/bar/demo/GreetingResource.java"
  ]
  range = Range [
    start = Position [
      line = 13
      character = 18
    ]
    end = Position [
      line = 13
      character = 18
    ]
  ]
  context = CodeActionContext [
    diagnostics = ArrayList ()
    only = null
  ]
]]

@bstaletic
Copy link
Contributor Author

@fbricon So the command.getArguments() was an instance of CodeActionParams instead of Collections.singletonList containing one WorkspaceEdit.

If I'm not mistaken, this patch should fix that bug:

diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java
index 71e66b8..6521962 100644
--- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java
+++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java
@@ -187,6 +187,7 @@ public class CodeActionHandler {
                }

                Command command;
+               boolean commandContainsWorkspaceEdit = false;
                if (proposal instanceof CUCorrectionCommandProposal) {
                        CUCorrectionCommandProposal commandProposal = (CUCorrectionCommandProposal) proposal;
                        command = new Command(name, commandProposal.getCommand(), commandProposal.getCommandArguments());
@@ -199,12 +200,13 @@ public class CodeActionHandler {
                                return Optional.empty();
                        }
                        command = new Command(name, COMMAND_ID_APPLY_EDIT, Collections.singletonList(edit));
+                       commandContainsWorkspaceEdit = true;
                }

                if (preferenceManager.getClientPreferences().isSupportedCodeActionKind(proposal.getKind())) {
                        CodeAction codeAction = new CodeAction(name);
                        codeAction.setKind(proposal.getKind());
-                       if (preferenceManager.getClientPreferences().isSupportedCodeActionLiteral()) {
+                       if (preferenceManager.getClientPreferences().isSupportedCodeActionLiteral() && commandContainsWorkspaceEdit) {
                                codeAction.setEdit((WorkspaceEdit)command.getArguments().get(0));
                        } else {
                                codeAction.setCommand(command);

Since it's the quickest thing I could write to see if my assumption is right, would you mind testing this patch before I push it?

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.

No delegateCommandHandler for java.apply.workspaceEdit
6 participants