From 079a845678f510775862479c8d32137461c72676 Mon Sep 17 00:00:00 2001 From: Riccardo Strina Date: Tue, 4 Aug 2026 15:31:19 +0200 Subject: [PATCH] Fix overlapping edits in postfix completions Replace the complete postfix expression with one primary text edit. Keep independent context edits, such as imports, in additionalTextEdits so completion edits conform to the LSP contract. Update postfix tests for resolution, item defaults, and edit overlap. Signed-off-by: Riccardo Strina --- .../template/java/PostfixTemplateEngine.java | 21 ++- .../handlers/CompletionResolveHandler.java | 2 +- .../handlers/PostfixCompletionTest.java | 136 +++++++++--------- 3 files changed, 74 insertions(+), 85 deletions(-) diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/corext/template/java/PostfixTemplateEngine.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/corext/template/java/PostfixTemplateEngine.java index c0f572ffe2..641f62beb3 100644 --- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/corext/template/java/PostfixTemplateEngine.java +++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/corext/template/java/PostfixTemplateEngine.java @@ -46,6 +46,7 @@ import org.eclipse.lsp4j.CompletionItemLabelDetails; import org.eclipse.lsp4j.Range; import org.eclipse.lsp4j.TextEdit; +import org.eclipse.lsp4j.jsonrpc.messages.Either; public class PostfixTemplateEngine { private static String Switch_Name = "switch"; //$NON-NLS-1$ @@ -112,10 +113,10 @@ public List complete(IDocument document, int offset, ICompilatio context.setActiveTemplateName(template.getName()); content = evaluateGenericTemplate(context, template); } - setTextEdit(item, content, completionItemDefaults); + setTextEdit(item, content, range); if (!getClientPreferences().isResolveAdditionalTextEditsSupport()) { - setAdditionalTextEdit(item, compilationUnit, context, range, template); + setAdditionalTextEdit(item, compilationUnit, context, template); } if (isCompletionItemLabelDetailsSupport()) { @@ -151,19 +152,13 @@ public List complete(IDocument document, int offset, ICompilatio return res; } - private void setTextEdit(final CompletionItem item, String content, CompletionItemDefaults completionItemDefaults) { - if (getClientPreferences().isCompletionListItemDefaultsSupport() && completionItemDefaults.getEditRange() != null) { - item.setTextEditText(content); - } else { - item.setInsertText(content); - } + private void setTextEdit(final CompletionItem item, String content, Range range) { + item.setTextEdit(Either.forLeft(new TextEdit(range, content))); } public static void setAdditionalTextEdit(final CompletionItem item, ICompilationUnit compilationUnit, - JavaPostfixContext context, Range range, Template template) { + JavaPostfixContext context, Template template) { List additionalEdits = new ArrayList<>(); - // use additional test edit to remove the code that needs to be replaced - additionalEdits.add(new TextEdit(range, "")); List jdtTextEdits = context.getAdditionalTextEdits(template.getName()); if (jdtTextEdits != null && !jdtTextEdits.isEmpty()) { for (org.eclipse.text.edits.TextEdit edit : jdtTextEdits) { @@ -171,7 +166,9 @@ public static void setAdditionalTextEdit(final CompletionItem item, ICompilation additionalEdits.addAll(converter.convert()); } } - item.setAdditionalTextEdits(additionalEdits); + if (!additionalEdits.isEmpty()) { + item.setAdditionalTextEdits(additionalEdits); + } } private boolean isCompletionItemLabelDetailsSupport() { diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResolveHandler.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResolveHandler.java index 41e12909d6..2e62195d88 100644 --- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResolveHandler.java +++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionResolveHandler.java @@ -150,7 +150,7 @@ public CompletionItem resolve(CompletionItem param, IProgressMonitor monitor) { } if (manager.getClientPreferences().isResolveAdditionalTextEditsSupport()) { - PostfixTemplateEngine.setAdditionalTextEdit(param, unit, postfixContext, range, template); + PostfixTemplateEngine.setAdditionalTextEdit(param, unit, postfixContext, template); } } diff --git a/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/PostfixCompletionTest.java b/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/PostfixCompletionTest.java index 9204349b68..5145908250 100644 --- a/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/PostfixCompletionTest.java +++ b/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/PostfixCompletionTest.java @@ -80,6 +80,7 @@ public void tearDown() throws Exception { public void testCastLazyResolve() throws JavaModelException { try { preferences.setCompletionLazyResolveTextEditEnabled(true); + when(preferenceManager.getClientPreferences().isResolveAdditionalTextEditsSupport()).thenReturn(true); //@formatter:off ICompilationUnit unit = getWorkingCopy( "src/org/sample/Test.java", @@ -98,10 +99,12 @@ public void testCastLazyResolve() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("cast", item.getLabel()); - assertEquals(item.getInsertText(), "((${1})${inner_expression})${0}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 8)), range); + Range range = new Range(new Position(3, 2), new Position(3, 8)); + assertPostfixTextEdit(item, "((${1})${inner_expression})${0}", range); + + CompletionItem resolved = server.resolveCompletionItem(item).join(); + assertPostfixTextEdit(resolved, "((${1})a)${0}", range); } finally { preferences.setCompletionLazyResolveTextEditEnabled(false); } @@ -127,10 +130,8 @@ public void test_assert() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("assert", item.getLabel()); - assertEquals(item.getInsertText(), "assert identifier;"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 19)), range); + assertPostfixTextEdit(item, "assert identifier;", new Range(new Position(3, 2), new Position(3, 19))); } @Test @@ -157,11 +158,9 @@ public void test_cast() throws JavaModelException { assertEquals("cast", item.getLabel()); assertNull(item.getLabelDetails().getDetail()); assertEquals("Casts the expression to a new type", item.getLabelDetails().getDescription()); - assertEquals(item.getInsertText(), "((${1})a)${0}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); assertEquals(item.getInsertTextMode(), InsertTextMode.AdjustIndentation); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 8)), range); + assertPostfixTextEdit(item, "((${1})a)${0}", new Range(new Position(3, 2), new Position(3, 8))); } @Test @@ -185,11 +184,9 @@ public void test_if() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("if", item.getLabel()); - assertEquals(item.getInsertText(), "if (a) {\n\t${0}\n}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); assertNull(item.getInsertTextMode()); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 6)), range); + assertPostfixTextEdit(item, "if (a) {\n\t${0}\n}", new Range(new Position(3, 2), new Position(3, 6))); } @Test @@ -212,10 +209,8 @@ public void test_else() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("else", item.getLabel()); - assertEquals(item.getInsertText(), "if (!a) {\n\t${0}\n}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 8)), range); + assertPostfixTextEdit(item, "if (!a) {\n\t${0}\n}", new Range(new Position(3, 2), new Position(3, 8))); } @Test @@ -238,10 +233,8 @@ public void test_for() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("for", item.getLabel()); - assertEquals(item.getInsertText(), "for (String ${1:a2} : a) {\n\t${0}\n}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 7)), range); + assertPostfixTextEdit(item, "for (String ${1:a2} : a) {\n\t${0}\n}", new Range(new Position(3, 2), new Position(3, 7))); } @Test @@ -264,10 +257,8 @@ public void test_fori() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("fori", item.getLabel()); - assertEquals(item.getInsertText(), "for (int ${1:a2} = 0; ${1:a2} < a.length; ${1:a2}++) {\n\t${0}\n}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 8)), range); + assertPostfixTextEdit(item, "for (int ${1:a2} = 0; ${1:a2} < a.length; ${1:a2}++) {\n\t${0}\n}", new Range(new Position(3, 2), new Position(3, 8))); } @Test @@ -290,10 +281,8 @@ public void test_forr() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("forr", item.getLabel()); - assertEquals(item.getInsertText(), "for (int ${1:a2} = a.length - 1; ${1:a2} >= 0; ${1:a2}--) {\n\t${0}\n}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 8)), range); + assertPostfixTextEdit(item, "for (int ${1:a2} = a.length - 1; ${1:a2} >= 0; ${1:a2}--) {\n\t${0}\n}", new Range(new Position(3, 2), new Position(3, 8))); } @Test @@ -316,10 +305,8 @@ public void test_nnull() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("nnull", item.getLabel()); - assertEquals(item.getInsertText(), "if (a != null) {\n\t${0}\n}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 9)), range); + assertPostfixTextEdit(item, "if (a != null) {\n\t${0}\n}", new Range(new Position(3, 2), new Position(3, 9))); } @Test @@ -342,10 +329,8 @@ public void test_null() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("null", item.getLabel()); - assertEquals(item.getInsertText(), "if (a == null) {\n\t${0}\n}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 8)), range); + assertPostfixTextEdit(item, "if (a == null) {\n\t${0}\n}", new Range(new Position(3, 2), new Position(3, 8))); } @Test @@ -368,10 +353,8 @@ public void test_opt() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("opt", item.getLabel()); - assertEquals(item.getInsertText(), "Optional.ofNullable(identifier)"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 16)), range); + assertPostfixTextEdit(item, "Optional.ofNullable(identifier)", new Range(new Position(3, 2), new Position(3, 16))); } @Test @@ -394,10 +377,8 @@ public void test_not() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("not", item.getLabel()); - assertEquals(item.getInsertText(), "!a"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 7)), range); + assertPostfixTextEdit(item, "!a", new Range(new Position(3, 2), new Position(3, 7))); } @Test @@ -420,10 +401,8 @@ public void test_sysout() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("sysout", item.getLabel()); - assertEquals(item.getInsertText(), "System.out.println(a);${0}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 10)), range); + assertPostfixTextEdit(item, "System.out.println(a);${0}", new Range(new Position(3, 2), new Position(3, 10))); } @Test @@ -448,9 +427,9 @@ public void test_sysout_itemDefaults_enabled() throws Exception { CompletionItem ci = list.getItems().stream().filter(item -> item.getLabel().startsWith("sysout")).findFirst().orElse(null); assertNotNull(ci); - assertEquals("System.out.println(new Test());${0}", ci.getTextEditText()); - //check that the fields covered by itemDefaults are set to null - assertNull(ci.getTextEdit()); + assertPostfixTextEdit(ci, "System.out.println(new Test());${0}", new Range(new Position(4, 2), new Position(4, 17))); + // The full postfix range differs from the shared item default, so this item must override it. + assertNull(ci.getTextEditText()); assertNull(ci.getInsertTextFormat()); assertNull(ci.getInsertTextMode()); assertEquals(CompletionItemKind.Snippet, ci.getKind()); @@ -477,10 +456,8 @@ public void test_sysout_object() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("sysout", item.getLabel()); - assertEquals(item.getInsertText(), "System.out.println(foo);${0}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(4, 2), new Position(4, 12)), range); + assertPostfixTextEdit(item, "System.out.println(foo);${0}", new Range(new Position(4, 2), new Position(4, 12))); } @Test @@ -504,10 +481,8 @@ public void test_sysoutv_object() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("sysoutv", item.getLabel()); - assertEquals(item.getInsertText(), "System.out.println(\"foo = \" + foo);${0}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(4, 2), new Position(4, 13)), range); + assertPostfixTextEdit(item, "System.out.println(\"foo = \" + foo);${0}", new Range(new Position(4, 2), new Position(4, 13))); } @Test @@ -531,10 +506,8 @@ public void test_sysouf_object() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("sysouf", item.getLabel()); - assertEquals(item.getInsertText(), "System.out.printf(\"\", foo);${0}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(4, 2), new Position(4, 12)), range); + assertPostfixTextEdit(item, "System.out.printf(\"\", foo);${0}", new Range(new Position(4, 2), new Position(4, 12))); } @Test @@ -558,10 +531,8 @@ public void test_syserr_object() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("syserr", item.getLabel()); - assertEquals(item.getInsertText(), "System.err.println(foo);${0}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(4, 2), new Position(4, 12)), range); + assertPostfixTextEdit(item, "System.err.println(foo);${0}", new Range(new Position(4, 2), new Position(4, 12))); } @Test @@ -583,9 +554,7 @@ public void test_format() throws JavaModelException { CompletionItem item = list.getItems().stream().filter(i -> i.getKind() == CompletionItemKind.Snippet).findFirst().orElse(null); assertEquals("format", item.getLabel()); - assertEquals(item.getTextEditText(), "String.format(a, ${0});"); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 10)), range); + assertPostfixTextEdit(item, "String.format(a, ${0});", new Range(new Position(3, 2), new Position(3, 10))); } @Test @@ -609,10 +578,8 @@ public void test_throw() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("throw", item.getLabel()); - assertEquals(item.getInsertText(), "throw e;"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(4, 2), new Position(4, 9)), range); + assertPostfixTextEdit(item, "throw e;", new Range(new Position(4, 2), new Position(4, 9))); } @Test @@ -635,10 +602,8 @@ public void test_var() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("var", item.getLabel()); - assertEquals(item.getInsertText(), "String ${1:a2} = a;${0}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 7)), range); + assertPostfixTextEdit(item, "String ${1:a2} = a;${0}", new Range(new Position(3, 2), new Position(3, 7))); } @Test @@ -662,12 +627,13 @@ public void test_var2() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("var", item.getLabel()); - assertEquals(item.getInsertText(), "List ${1:emptyList} = Collections.emptyList();${0}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); + assertPrimaryTextEdit(item, "List ${1:emptyList} = Collections.emptyList();${0}", + new Range(new Position(4, 2), new Position(4, 29))); List additionalTextEdits = item.getAdditionalTextEdits(); - Range range =additionalTextEdits.get(0).getRange(); - assertEquals(new Range(new Position(4, 2), new Position(4, 29)), range); + assertNotNull(additionalTextEdits); assertTrue(additionalTextEdits.stream().anyMatch(e -> e.getNewText().contains("import java.util.List;"))); + assertAdditionalTextEditsDoNotOverlapPrimary(item); } @Test @@ -690,10 +656,8 @@ public void test_par() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("par", item.getLabel()); - assertEquals(item.getInsertText(), "(a)"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 7)), range); + assertPostfixTextEdit(item, "(a)", new Range(new Position(3, 2), new Position(3, 7))); } @Test @@ -716,10 +680,8 @@ public void test_while() throws JavaModelException { List items = new ArrayList<>(list.getItems()); CompletionItem item = items.get(0); assertEquals("while", item.getLabel()); - assertEquals(item.getInsertText(), "while (a) {\n\t${0}\n}"); assertEquals(item.getInsertTextFormat(), InsertTextFormat.Snippet); - Range range = item.getAdditionalTextEdits().get(0).getRange(); - assertEquals(new Range(new Position(3, 2), new Position(3, 9)), range); + assertPostfixTextEdit(item, "while (a) {\n\t${0}\n}", new Range(new Position(3, 2), new Position(3, 9))); } @Test @@ -829,6 +791,36 @@ private String createCompletionRequest(ICompilationUnit unit, int line, int kar) .replace("${char}", String.valueOf(kar)); } + private void assertPostfixTextEdit(CompletionItem item, String newText, Range range) { + assertPrimaryTextEdit(item, newText, range); + assertAdditionalTextEditsDoNotOverlapPrimary(item); + } + + private void assertPrimaryTextEdit(CompletionItem item, String newText, Range range) { + assertNotNull(item.getTextEdit()); + assertTrue(item.getTextEdit().isLeft()); + assertEquals(new TextEdit(range, newText), item.getTextEdit().getLeft()); + } + + private void assertAdditionalTextEditsDoNotOverlapPrimary(CompletionItem item) { + if (item.getAdditionalTextEdits() == null) { + return; + } + Range primaryRange = item.getTextEdit().getLeft().getRange(); + for (TextEdit additionalEdit : item.getAdditionalTextEdits()) { + assertFalse(rangesOverlap(primaryRange, additionalEdit.getRange())); + } + } + + private boolean rangesOverlap(Range left, Range right) { + return compare(left.getStart(), right.getEnd()) < 0 && compare(right.getStart(), left.getEnd()) < 0; + } + + private int compare(Position left, Position right) { + int lineComparison = Integer.compare(left.getLine(), right.getLine()); + return lineComparison != 0 ? lineComparison : Integer.compare(left.getCharacter(), right.getCharacter()); + } + private void mockLSP3Client() { mockLSPClient(true, true, true); }