Avoid validating all opened workingcopies when didChange a document #2587

testforstephen · 2023-04-10T09:04:13Z

This PR introduces a workspace setting java.edit.validateAllOpenBuffersOnChanges that allows users to choose the diagnostics strategy for unsaved changes when editing a Java file.

Strategy 1: Validate all open Java files (workingcopies) in the editor for errors when you're typing in a Java file. The advantage of this strategy is that it can detect errors quickly if the current changes affect other open Java files. The drawback is that it can be costly to validate all open buffers on every change.
Strategy 2: Validate only the current Java file for errors when you're typing. This strategy can use less CPU resources and respond faster if the changes introduce errors. However, it may not report potential errors in other Java files right away. If the client prefers strategy 2, we recommend that the client supports resending a request to validate the document when users switch their focus to a Java file. The format of the validate document notification is as follows:

languageClient.sendNotification('java/validateDocument', {
	textDocument: {
		uri: '<java file uri string>',
	},
});

Eskibear · 2023-04-10T09:53:16Z

To ensure the correctness of diagnostics for related files, autobuild will report their diagnostics if you save the current file.

This LGTM.

snjeza

@testforstephen @jdneo you may want to take a look at redhat-developer/vscode-java#2982 (comment)

snjeza · 2023-04-10T20:57:02Z

A related issue - #279

testforstephen · 2023-04-11T01:45:30Z

@testforstephen @jdneo you may want to take a look at redhat-developer/vscode-java#2982 (comment)

We have to process all opened files. It is possible that editing Test.java fixes or introduces an error in Test2.java.
Steps to reproduce:

create the test2() method in Test.java
invoke new Test().test() in Test2.java;
save both classes
you will see the The method test is undefined for the type Test error.
edit Test.java and update test2 to test; you don't need to save anything
it will fix The method test is undefined for the type Test in Test2.java

We understand the reason behind validating all open working copies. However, the concern is that the disadvantages outweigh the benefits. I believe you should notice similar user complaint that the more you use it, the slower the extension gets. This should be one reason for that.

Let’s consider this issue from another perspective. If the related file Test2.java is not open, renaming test2 to test in Test.java will not automatically fix the compilation errors in Test2.java. This means that validating all working copies cannot guarantee that all potential errors introduced by current changes will be immediately revealed.

In contrast, when editing a Java file in Eclipse, only the current file is immediately validated, not other working copies.

Sample 1 in Eclipse:

validatingcopy.mp4

Sample 2 in Eclipse:

validatingcopy1.mp4

snjeza · 2023-04-11T18:04:02Z

We understand the reason behind validating all open working copies. However, the concern is that the disadvantages outweigh the benefits. I believe you should notice similar user complaint that the more you use it, the slower the extension gets. This should be one reason for that.

@testforstephen Agree. cc @fbricon @rgrunber

fbricon · 2023-04-17T10:45:33Z

So I found an issue with this PR: if you use a code action that creates a missing class, once the new class has been created, then file being edited still shows an error, until the next validation trigger.

Same issue with code action creating a missing method.

testforstephen · 2023-04-17T12:52:45Z

So I found an issue with this PR: if you use a code action that creates a missing class, once the new class has been created, then file being edited still shows an error, until the next validation trigger.

Same issue with code action creating a missing method.

@fbricon One possible solution is to append a java.project.refreshDiagnostics command to the code action response of creating a missing class. This would trigger a command to update the diagnostics of the current file after the code action workspace edit is applied. I prefer to fix the code action diagnostics issue in a new separate PR.

WDYT?

testforstephen · 2023-04-17T13:13:56Z

Here is a patch code that can solve the code action diagnostics issue when creating missing class. You can test it by applying the git diff code on the current PR. However, I think it would be clearer to use a separate PR for the new issue.

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 0c353e8e..b3e48fce 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
@@ -46,6 +46,7 @@ import org.eclipse.jdt.ls.core.internal.corrections.InnovationContext;
 import org.eclipse.jdt.ls.core.internal.corrections.QuickFixProcessor;
 import org.eclipse.jdt.ls.core.internal.corrections.RefactorProcessor;
 import org.eclipse.jdt.ls.core.internal.corrections.proposals.ChangeCorrectionProposal;
+import org.eclipse.jdt.ls.core.internal.corrections.proposals.NewCUProposal;
 import org.eclipse.jdt.ls.core.internal.preferences.PreferenceManager;
 import org.eclipse.jdt.ls.core.internal.preferences.Preferences;
 import org.eclipse.jdt.ls.core.internal.text.correction.AssignToVariableAssistCommandProposal;
@@ -198,7 +199,7 @@ public class CodeActionHandler {
 		}
 		try {
 			for (ChangeCorrectionProposal proposal : proposals) {
-				Optional<Either<Command, CodeAction>> codeActionFromProposal = getCodeActionFromProposal(proposal, params.getContext());
+				Optional<Either<Command, CodeAction>> codeActionFromProposal = getCodeActionFromProposal(params.getTextDocument().getUri(), proposal, params.getContext());
 				if (codeActionFromProposal.isPresent() && !codeActions.contains(codeActionFromProposal.get())) {
 					codeActions.add(codeActionFromProposal.get());
 				}
@@ -256,7 +257,7 @@ public class CodeActionHandler {
 		}
 	}
 
-	private Optional<Either<Command, CodeAction>> getCodeActionFromProposal(ChangeCorrectionProposal proposal, CodeActionContext context) throws CoreException {
+	private Optional<Either<Command, CodeAction>> getCodeActionFromProposal(String uri, ChangeCorrectionProposal proposal, CodeActionContext context) throws CoreException {
 		String name = proposal.getName();
 
 		Command command = null;
@@ -281,6 +282,12 @@ public class CodeActionHandler {
 			CodeAction codeAction = new CodeAction(name);
 			codeAction.setKind(proposal.getKind());
 			if (command == null) { // lazy resolve the edit.
+				if (proposal instanceof NewCUProposal) {
+					codeAction.setCommand(new Command("refresh Diagnostics", "java.project.refreshDiagnostics", Arrays.asList(
+						uri, "thisFile", false
+					)));
+				}
+
 				// The relevance is in descending order while CodeActionComparator sorts in ascending order
 				codeAction.setData(new CodeActionData(proposal, -proposal.getRelevance()));
 			} else {

fbricon · 2023-04-17T14:14:56Z

I'm not fond at all about this solution, as it puts the burden on clients to implement/call a new refresh command

fbricon · 2023-04-17T14:17:24Z

BTW that patch doesn't take care of a code action modifying a different file to implement new/missing methods.

snjeza · 2023-04-17T21:09:02Z

BTW that patch doesn't take care of a code action modifying a different file to implement new/missing methods.

@fbricon @testforstephen See #279

testforstephen · 2023-04-18T02:07:49Z

I'm not fond at all about this solution, as it puts the burden on clients to implement/call a new refresh command

This refresh command is an existing delegate command on server side, no extra client effort for this.

BTW that patch doesn't take care of a code action modifying a different file to implement new/missing methods.

The patch is just a demo that we have a different solution to fix the code action problem. We can apply similar logic to cover other code action cases.

testforstephen · 2023-04-18T02:36:14Z

See #279

Thanks for sharing more history. However, I’m concerned that validating all open buffers on each editing would have a negative impact on the performance that outweighs the benefits. If you have any suggestions on how to improve it and make it more client-friendly, I’m open to hearing them.

testforstephen · 2023-04-19T07:21:47Z

I'm not fond at all about this solution, as it puts the burden on clients to implement/call a new refresh command

Let me clarify a little bit, the command java.project.refreshDiagnostics is a server-side delegate command that is also registered to the client as a workspace/executeCommand. This command is visible to the client, but fully handled by the server.
https://github.com/eclipse/eclipse.jdt.ls/blob/73ccd84d4a0f57312ece7c049c27c6e685a889fe/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTDelegateCommandHandler.java#L101

https://github.com/eclipse/eclipse.jdt.ls/blob/73ccd84d4a0f57312ece7c049c27c6e685a889fe/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/InitHandler.java#L148

If we agree the solution to fix the code action diagnostic, i can create a new PR for it. Anything further to improve in this PR?

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

testforstephen · 2023-05-04T08:53:37Z

Hi @puremourning @mfussenegger,

We are discussing the CodeAction spec, and support returning a TextEdit and a command (such as java.project.refreshDiagnostics in the CodeAction response. The jdtls provides this refreshDiagnostics command as a workspace/executeCommand command. VS Code supports this workspace command well. How about other clients? Do they support this server side workspace command for code action response?

puremourning · 2023-05-04T08:57:53Z

If I understand correctly, you are going to return an edit and a command in the same code action? If so ycmd doesn't support it. We assume it's one-or-the-other.

We have a TODO:

    fixits = []
    for code_action in code_actions:
      if 'edit' in code_action:
        # TODO: Start supporting a mix of WorkspaceEdits and Commands
        # once there's a need for such
        assert 'command' not in code_action

        # This is a WorkspaceEdit literal
        fixits.append( self.CodeActionLiteralToFixIt( request_data,
                                                      code_action ) )
        continue

      # Either a CodeAction or a Command
      assert 'command' in code_action

testforstephen · 2023-05-04T12:09:10Z

If I understand correctly, you are going to return an edit and a command in the same code action? If so ycmd doesn't support it. We assume it's one-or-the-other.

Yes, we intend to return both as the spec allows AND, not just OR.

/**
 * A code action represents a change that can be performed in code, e.g. to fix
 * a problem or to refactor code.
 *
 * A CodeAction must set either `edit` and/or a `command`. If both are supplied,
 * the `edit` is applied first, then the `command` is executed.
 */
export interface CodeAction {
...
}

testforstephen · 2023-05-05T05:39:22Z

@puremourning I opened an issue ycm-core/ycmd#1692 (Support a mix of edit and command in the code action response) since ycmd implementation is inconsistent with the LSP spec.

…lidate all buffers during document lifecycle job

testforstephen · 2023-05-23T07:58:12Z

The latest PR adds a client flag ‘validateAllOpenBuffersOnDidChange’ to control whether to check all open buffers for errors during didChange event. The default value is true, so all clients will keep the old behavior unless they explicitly set the flag to false.

I also created another PR #2664 to address the issue of inconsistent diagnostics with code action.

In VS Code, we will set the flag to false for performance reasons. In my experiment, opening 40 test cases with guava project and typing any code took more than 5 seconds to run a validation on all buffers. This is too slow.

@fbricon @rgrunber @jdneo WDYT?

testforstephen · 2023-05-23T08:06:53Z

test this please

fbricon · 2023-05-23T11:27:11Z

In VS Code, we will set the flag to false for performance reasons. In my experiment, opening 40 test cases with guava project and typing any code took more than 5 seconds to run a validation on all buffers. This is too slow.

40s with or without this PR?

puremourning · 2023-05-23T12:50:23Z

Rather than a custom capability can it be a workspace configuration? That avoids client code changes and lets the user decide rather than the client author.

testforstephen · 2023-05-23T13:32:02Z

In VS Code, we will set the flag to false for performance reasons. In my experiment, opening 40 test cases with guava project and typing any code took more than 5 seconds to run a validation on all buffers. This is too slow.

40s with or without this PR?

I tested it with 40 Java files opened in the editor, and edited one Java file with more than 2000 lines of code. The diagnostics job reports errors much faster with this PR. Without this PR, it takes 8 seconds to finish the diagnostics job and report errors. With this PR, it only takes 700 milliseconds to report errors.

…o control whether to validate all open buffers

testforstephen · 2023-05-24T07:27:53Z

Rather than a custom capability can it be a workspace configuration? That avoids client code changes and lets the user decide rather than the client author.

@puremourning Done. Changed to a workspace setting java.edit.validateAllOpenBuffersOnChanges to control the behavior.

mfussenegger · 2023-05-24T08:26:42Z

. How about other clients? Do they support this server side workspace command for code action response?

Sorry I had missed the notification. Neovim LSP supports handling workspace edit and commands within code actions.

The latest PR adds a client flag ‘validateAllOpenBuffersOnDidChange’ to control whether to check all open buffers for errors during didChange event. The default value is true, so all clients will keep the old behavior unless they explicitly set the flag to false.

Does a save still update all open buffers?

testforstephen · 2023-05-24T09:58:01Z

Does a save still update all open buffers?

No, saving a file will not revalidate all open buffers. But it will trigger an auto-build job to recompile the affected files and update their diagnostics. This PR is to discuss the diagnostics strategy for unsaved changes that occur during editing.

testforstephen · 2023-05-25T03:10:37Z

Both the PR description section #2587 (comment) and the wiki https://github.com/eclipse/eclipse.jdt.ls/wiki/Running-the-JAVA-LS-server-from-the-command-line have been updated to explain how clients can use this setting.

puremourning · 2023-05-25T17:13:40Z

So I tried this out with my client and one thing that would be nice is a way to "force" jdt to revalidate a specific buffer. That way I can trigger a revalidation of a file which was open, but not visible in the editor, but that's now visible. This makes the results for the user smoother as it doesn't lead to stale diagnostics, like in the below example:

https://asciinema.org/a/zBzR2IGroIuN5XFF0YpTa31iL

Ideally I could (from the client) force a refresh of diagnostic for the file on the right when I put my cursor into that window (in vim terms, visit the buffer).

I did try just sending didClose followed immediately by didOpen whenever visiting the buffer, but this was too slow and caused things like semantic tokens requests to fail randomly.

puremourning · 2023-05-25T18:33:45Z

Wait I just remembered that the whole reason you pinged me on this was because of the custom "refreshDiagnostics" command! #2587 (comment)

Let me see if I can abuse that for this purpose.

testforstephen added the performance label Apr 10, 2023

testforstephen mentioned this pull request Apr 10, 2023

Re-validate current document if the focus is switched to a Java file redhat-developer/vscode-java#3053

Merged

testforstephen requested review from fbricon, snjeza, rgrunber and jdneo April 10, 2023 09:07

testforstephen requested review from Eskibear and CsCherrYY April 10, 2023 10:57

snjeza reviewed Apr 10, 2023

View reviewed changes

rgrunber added this to the End April 2023 milestone Apr 13, 2023

jdneo reviewed Apr 20, 2023

View reviewed changes

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

testforstephen force-pushed the jinbo_diagnostics branch from 9da77d5 to 3c6b133 Compare April 20, 2023 07:27

rgrunber removed this from the End April 2023 milestone Apr 26, 2023

testforstephen added this to the End May 2023 milestone May 4, 2023

testforstephen added 3 commits May 23, 2023 13:30

Avoid validating all opened workingcopies when didChange a document

696aaf7

Fix the failed test case

ea04640

Use ConcurrentHashMap.newKeySet() for synchronized set

e372e23

testforstephen force-pushed the jinbo_diagnostics branch from 3c6b133 to e372e23 Compare May 23, 2023 05:48

Use flag 'validateAllOpenBuffersOnDidChange' to control whether to va…

8059f12

…lidate all buffers during document lifecycle job

testforstephen mentioned this pull request May 23, 2023

Update the outdated diagnostics when the code action for creating a missing element is invoked #2664

Merged

Use a workspace setting 'java.edit.validateAllOpenBuffersOnChanges' t…

82e91dd

…o control whether to validate all open buffers

snjeza approved these changes May 24, 2023

View reviewed changes

jdneo approved these changes May 25, 2023

View reviewed changes

testforstephen merged commit ab346ee into eclipse-jdtls:master May 25, 2023

testforstephen deleted the jinbo_diagnostics branch May 25, 2023 06:33

rgrunber mentioned this pull request May 17, 2024

Doesn't revalidate project files after classpath changes if autobuild is off. #3155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid validating all opened workingcopies when didChange a document #2587

Avoid validating all opened workingcopies when didChange a document #2587

testforstephen commented Apr 10, 2023 •

edited

Loading

Eskibear commented Apr 10, 2023

snjeza left a comment

snjeza commented Apr 10, 2023

testforstephen commented Apr 11, 2023

snjeza commented Apr 11, 2023

fbricon commented Apr 17, 2023 •

edited

Loading

testforstephen commented Apr 17, 2023

testforstephen commented Apr 17, 2023

fbricon commented Apr 17, 2023

fbricon commented Apr 17, 2023

snjeza commented Apr 17, 2023

testforstephen commented Apr 18, 2023

testforstephen commented Apr 18, 2023

testforstephen commented Apr 19, 2023

testforstephen commented May 4, 2023

puremourning commented May 4, 2023

testforstephen commented May 4, 2023

testforstephen commented May 5, 2023

testforstephen commented May 23, 2023

testforstephen commented May 23, 2023

fbricon commented May 23, 2023

puremourning commented May 23, 2023

testforstephen commented May 23, 2023

testforstephen commented May 24, 2023

mfussenegger commented May 24, 2023

testforstephen commented May 24, 2023 •

edited

Loading

testforstephen commented May 25, 2023

puremourning commented May 25, 2023 •

edited

Loading

puremourning commented May 25, 2023

Avoid validating all opened workingcopies when didChange a document #2587

Avoid validating all opened workingcopies when didChange a document #2587

Conversation

testforstephen commented Apr 10, 2023 • edited Loading

Eskibear commented Apr 10, 2023

snjeza left a comment

Choose a reason for hiding this comment

snjeza commented Apr 10, 2023

testforstephen commented Apr 11, 2023

snjeza commented Apr 11, 2023

fbricon commented Apr 17, 2023 • edited Loading

testforstephen commented Apr 17, 2023

testforstephen commented Apr 17, 2023

fbricon commented Apr 17, 2023

fbricon commented Apr 17, 2023

snjeza commented Apr 17, 2023

testforstephen commented Apr 18, 2023

testforstephen commented Apr 18, 2023

testforstephen commented Apr 19, 2023

testforstephen commented May 4, 2023

puremourning commented May 4, 2023

testforstephen commented May 4, 2023

testforstephen commented May 5, 2023

testforstephen commented May 23, 2023

testforstephen commented May 23, 2023

fbricon commented May 23, 2023

puremourning commented May 23, 2023

testforstephen commented May 23, 2023

testforstephen commented May 24, 2023

mfussenegger commented May 24, 2023

testforstephen commented May 24, 2023 • edited Loading

testforstephen commented May 25, 2023

puremourning commented May 25, 2023 • edited Loading

puremourning commented May 25, 2023

testforstephen commented Apr 10, 2023 •

edited

Loading

fbricon commented Apr 17, 2023 •

edited

Loading

testforstephen commented May 24, 2023 •

edited

Loading

puremourning commented May 25, 2023 •

edited

Loading