Only do context sensitive import rewrite when resolving completion it…

…ems (#2453) * Only do context sensitive import rewrite when resolving completion items - fix the performance downgrade introduced by #2232. With that change, triggering completion via keyboard shortcut will take a long time. Now with the change, it will only do context sensitive import rewrite when resolving completion items. --------- Signed-off-by: sheche <[email protected]>
eclipse-jdtls · Feb 15, 2023 · 3e464e1 · 3e464e1
1 parent eef3e3f
commit 3e464e1
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 29 deletions.
diff --git a/...org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalReplacementProvider.java b/...org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalReplacementProvider.java
@@ -41,6 +41,7 @@
 import org.eclipse.jdt.core.dom.IBinding;
 import org.eclipse.jdt.core.dom.ITypeBinding;
 import org.eclipse.jdt.core.dom.rewrite.ImportRewrite;
+import org.eclipse.jdt.core.dom.rewrite.ImportRewrite.ImportRewriteContext;
 import org.eclipse.jdt.core.manipulation.SharedASTProviderCore;
 import org.eclipse.jdt.internal.codeassist.CompletionEngine;
 import org.eclipse.jdt.internal.corext.codemanipulation.ContextSensitiveImportRewriteContext;
@@ -91,39 +92,31 @@ public class CompletionProposalReplacementProvider {
 	private final ClientPreferences client;
 	private Preferences preferences;
 	private String anonymousTypeNewBody;
+	/**
+	 * whether the provider is used during `completionItem/resolve` request.
+	 */
+	private boolean isResolvingRequest;
 
-	public CompletionProposalReplacementProvider(ICompilationUnit compilationUnit, CompletionContext context, int offset, Preferences preferences, ClientPreferences clientPrefs) {
+	public CompletionProposalReplacementProvider(ICompilationUnit compilationUnit, CompletionContext context, int offset,
+			Preferences preferences, ClientPreferences clientPrefs, boolean isResolvingRequest) {
 		super();
 		this.compilationUnit = compilationUnit;
 		this.context = context;
 		this.offset = offset;
 		this.preferences = preferences == null ? new Preferences() : preferences;
 		this.client = clientPrefs;
+		this.isResolvingRequest = isResolvingRequest;
 	}
 
 	/**
-	 * Update the replacement together with additionalTextEdits for the given item.
-	 * It's originally designed to defer expensive calculation of the imports into completion/resolve stage.
-	 * @param proposal
-	 * @param item
-	 * @param trigger
-	 */
-	public void updateAdditionalTextEdits(CompletionProposal proposal, CompletionItem item, char trigger) {
-		updateReplacement(proposal, item, trigger, true);
-	}
-
-	/**
-	 * Updates the replacement but NO additional replacement for the given item.
-	 *
+	 * Update the replacement.
+	 * 
+	 * When {@link #isResolvingRequest} is <code>true</code>, additionalTextEdits will also be resolved.
 	 * @param proposal
 	 * @param item
 	 * @param trigger
 	 */
 	public void updateReplacement(CompletionProposal proposal, CompletionItem item, char trigger) {
-		updateReplacement(proposal, item, trigger, false);
-	}
-
-	private void updateReplacement(CompletionProposal proposal, CompletionItem item, char trigger, boolean isResolving) {
 		// reset importRewrite
 		this.importRewrite = TypeProposalUtils.createImportRewrite(compilationUnit);
 
@@ -207,7 +200,7 @@ private void updateReplacement(CompletionProposal proposal, CompletionItem item,
 			item.setTextEdit(Either.forLeft(new org.eclipse.lsp4j.TextEdit(insertReplaceEdit.getInsert(), text)));
 		}
 
-		if (!isImportCompletion(proposal) && (!client.isResolveAdditionalTextEditsSupport() || isResolving)) {
+		if (!isImportCompletion(proposal) && (!client.isResolveAdditionalTextEditsSupport() || isResolvingRequest)) {
 			addImports(additionalTextEdits);
 			if(!additionalTextEdits.isEmpty()){
 				item.setAdditionalTextEdits(additionalTextEdits);
@@ -1033,12 +1026,19 @@ private String computeJavaTypeReplacementString(CompletionProposal proposal) {
 
 		/* Add imports if the preference is on. */
 		if (importRewrite != null) {
-			CompilationUnit cu = SharedASTProviderCore.getAST(compilationUnit, SharedASTProviderCore.WAIT_NO, new NullProgressMonitor());
-			ContextSensitiveImportRewriteContext rewriteContext = null;
-			if (cu != null) {
-				rewriteContext = new ContextSensitiveImportRewriteContext(cu, this.offset, this.importRewrite);
+			ImportRewriteContext context = null;
+			// Only get more context-aware result during 'completionItem/resolve' request.
+			// This is because 'ContextSensitiveImportRewriteContext.findInContext()'' is a very
+			// heavy operation. If we do that when listing the completion items, the performance
+			// will downgrade a lot.
+			if (isResolvingRequest) {
+				CompilationUnit cu = SharedASTProviderCore.getAST(compilationUnit, SharedASTProviderCore.WAIT_NO, new NullProgressMonitor());
+				if (cu != null) {
+					context = new ContextSensitiveImportRewriteContext(cu, this.offset, this.importRewrite);
+				}
 			}
-			return importRewrite.addImport(qualifiedTypeName, rewriteContext);
+
+			return importRewrite.addImport(qualifiedTypeName, context);
 		}
 
 		// fall back for the case we don't have an import rewrite (see

diff --git a/....core/src/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalRequestor.java b/....core/src/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalRequestor.java
@@ -338,7 +338,14 @@ public void acceptContext(CompletionContext context) {
 		this.context = context;
 		response.setContext(context);
 		this.descriptionProvider = new CompletionProposalDescriptionProvider(unit, context);
-		this.proposalProvider = new CompletionProposalReplacementProvider(unit, context, response.getOffset(), preferenceManager.getPreferences(), preferenceManager.getClientPreferences());
+		this.proposalProvider = new CompletionProposalReplacementProvider(
+			unit,
+			context,
+			response.getOffset(),
+			preferenceManager.getPreferences(),
+			preferenceManager.getClientPreferences(),
+			false
+		);
 	}
 
 

diff --git a/...e.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResolveHandler.java b/...e.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResolveHandler.java
@@ -138,8 +138,15 @@ public CompletionItem resolve(CompletionItem param, IProgressMonitor monitor) {
 		}
 
 		if (manager.getClientPreferences().isResolveAdditionalTextEditsSupport()) {
-			CompletionProposalReplacementProvider proposalProvider = new CompletionProposalReplacementProvider(unit, completionResponse.getContext(), completionResponse.getOffset(), manager.getPreferences(), manager.getClientPreferences());
-			proposalProvider.updateAdditionalTextEdits(completionResponse.getProposals().get(proposalId), param, '\0');
+			CompletionProposalReplacementProvider proposalProvider = new CompletionProposalReplacementProvider(
+				unit,
+				completionResponse.getContext(),
+				completionResponse.getOffset(),
+				manager.getPreferences(),
+				manager.getClientPreferences(),
+				true
+			);
+			proposalProvider.updateReplacement(completionResponse.getProposals().get(proposalId), param, '\0');
 		}
 		if (data.containsKey(DATA_FIELD_DECLARATION_SIGNATURE)) {
 			String typeName = stripSignatureToFQN(String.valueOf(data.get(DATA_FIELD_DECLARATION_SIGNATURE)));

diff --git a/...pse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandlerTest.java b/...pse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandlerTest.java
@@ -3504,7 +3504,10 @@ public void testCompletion_QualifiedName2() throws Exception {
 	}
 
 	@Test
-	public void testCompletion_withConflictingTypeNames() throws Exception{
+	public void testCompletion_withConflictingTypeNames() throws Exception {
+		ClientPreferences mockCapabilies = Mockito.mock(ClientPreferences.class);
+		Mockito.when(preferenceManager.getClientPreferences()).thenReturn(mockCapabilies);
+		Mockito.when(mockCapabilies.isResolveAdditionalTextEditsSupport()).thenReturn(true);
 		getWorkingCopy("src/java/List.java",
 			"package util;\n" +
 			"public class List {\n" +
@@ -3528,7 +3531,8 @@ public void testCompletion_withConflictingTypeNames() throws Exception{
 		List<CompletionItem> items = list.getItems().stream().filter(p -> "java.util.List".equals(p.getDetail()))
 			.collect(Collectors.toList());
 		assertFalse("java.util.List not found",items.isEmpty());
-		assertEquals("java.util.List", items.get(0).getTextEdit().getLeft().getNewText());
+		CompletionItem resolved = server.resolveCompletionItem(list.getItems().get(0)).join();
+		assertEquals("java.util.List", resolved.getTextEdit().getLeft().getNewText());
 	}
 
 	@Test