Skip to content

Commit 151ba92

Browse files
sebthomsebthom
authored
fix: align completion comparator with LSP sortText semantics (#1424)
Make behavior consistent with the LSP specification for sortText usage: - Honor LSP sortText as the primary tie-breaker for equal rank results - Restrict documentFilter-length influence to real matches (categories 1–4) - Preserve server order only when comparator returns 0 (true ties) Fixes #814
1 parent 6b2d12a commit 151ba92

File tree

5 files changed

+248
-26
lines changed

5 files changed

+248
-26
lines changed

‎org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/completion/CompletionOrderingTests.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,21 @@ public void testOrderWithLongInsert() throws Exception {
150150
62, orderedResults);
151151
}
152152

153+
@Test
154+
public void testSortTextIsComparedLexicographically() throws Exception {
155+
final var completions = new ArrayList<CompletionItem>();
156+
157+
final var item15 = createCompletionItem("15", CompletionItemKind.Class);
158+
item15.setSortText("15");
159+
completions.add(item15);
160+
161+
final var item5 = createCompletionItem("5", CompletionItemKind.Class);
162+
item5.setSortText("5");
163+
completions.add(item5);
164+
165+
confirmCompletionResults(completions, "", 0, new String[] { "15", "5" });
166+
}
167+
153168
@Test
154169
public void testMovingOffset() throws Exception {
155170
final var range = new Range(new Position(0, 0), new Position(0, 4));

‎org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/completion/LSCompletionProposalComparatorTest.java‎

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
/*******************************************************************************
2+
* Copyright (c) 2025 Vegard IT GmbH and others.
3+
* This program and the accompanying materials are made
4+
* available under the terms of the Eclipse Public License 2.0
5+
* which is available at https://www.eclipse.org/legal/epl-2.0/
6+
*
7+
* SPDX-License-Identifier: EPL-2.0
8+
*
9+
* Contributors:
10+
* Sebastian Thomschke (Vegard IT GmbH) - initial implementation
11+
*******************************************************************************/
12+
package org.eclipse.lsp4e.test.completion;
13+
14+
import static org.junit.jupiter.api.Assertions.*;
15+
16+
import java.util.ArrayList;
17+
import java.util.HashMap;
18+
import java.util.Map;
19+
20+
import org.eclipse.jface.text.Document;
21+
import org.eclipse.jface.text.IDocument;
22+
import org.eclipse.lsp4e.operations.completion.LSCompletionProposal;
23+
import org.eclipse.lsp4e.operations.completion.LSCompletionProposalComparator;
24+
import org.eclipse.lsp4j.CompletionItem;
25+
import org.junit.jupiter.api.Test;
26+
27+
public class LSCompletionProposalComparatorTest {
28+
29+
/**
30+
* DocumentFilter length currently has top priority in LSCompletionProposalComparator.
31+
* This test encodes the expectation that, once proposals are considered matching,
32+
* sortText should decide the order, not the document filter length.
33+
*/
34+
@Test
35+
public void testDocumentFilterLengthDoesNotOverrideSortText() {
36+
IDocument document = new Document("");
37+
38+
final var item1 = new CompletionItem("p1");
39+
item1.setSortText("B");
40+
final var item2 = new CompletionItem("p2");
41+
item2.setSortText("A");
42+
43+
final var proposalWithLongFilter = new StubProposal(document, item1, "longFilter");
44+
final var proposalWithShortFilter = new StubProposal(document, item2, "x");
45+
46+
final var comparator = new LSCompletionProposalComparator();
47+
final var proposals = new ArrayList<LSCompletionProposal>();
48+
proposals.add(proposalWithLongFilter);
49+
proposals.add(proposalWithShortFilter);
50+
51+
proposals.sort(comparator);
52+
53+
// Desired order: sortText ascending -> "A", then "B"
54+
assertEquals("A", proposals.get(0).getSortText());
55+
assertEquals("B", proposals.get(1).getSortText());
56+
}
57+
58+
/**
59+
* CompletionProposalPopup.computeFilteredProposals filters an already sorted
60+
* list of proposals. To avoid surprising reordering if re-sorting is added
61+
* later, LSCompletionProposalComparator must not change the relative order
62+
* of proposals solely because their documentFilter length changes.
63+
*
64+
* This test verifies that changing the documentFilter does not change the
65+
* comparator outcome for otherwise identical proposals.
66+
*/
67+
@Test
68+
public void testFilteredProposalsShouldBeResortedWhenFilterChanges() {
69+
IDocument document = new Document("");
70+
71+
final var item1 = new CompletionItem("p1");
72+
final var item2 = new CompletionItem("p2");
73+
74+
final int initialOffset = 1;
75+
final int updatedOffset = 2;
76+
77+
final var proposalA = new VarFilterProposal(document, item1);
78+
final var proposalB = new VarFilterProposal(document, item2);
79+
80+
// At the initial offset, proposalA has a longer filter than proposalB
81+
// so A should be ordered before B.
82+
proposalA.setFilterForOffset(initialOffset, "xx");
83+
proposalB.setFilterForOffset(initialOffset, "x");
84+
85+
// After typing more, the filters swap lengths so the desired order
86+
// (if resorted) would become B before A.
87+
proposalA.setFilterForOffset(updatedOffset, "x");
88+
proposalB.setFilterForOffset(updatedOffset, "xx");
89+
90+
final var comparator = new LSCompletionProposalComparator();
91+
92+
// Initial sort at invocation offset
93+
final var initiallySorted = new ArrayList<LSCompletionProposal>();
94+
initiallySorted.add(proposalA);
95+
initiallySorted.add(proposalB);
96+
proposalA.setOffsetForSorting(initialOffset);
97+
proposalB.setOffsetForSorting(initialOffset);
98+
initiallySorted.sort(comparator);
99+
100+
// Simulate JFace behaviour: keep the original order and just filter
101+
final var filteredWithoutResort = new ArrayList<>(initiallySorted);
102+
103+
// Expected behaviour: update filters for the new offset and resort
104+
final var expectedResorted = new ArrayList<LSCompletionProposal>();
105+
expectedResorted.add(proposalA);
106+
expectedResorted.add(proposalB);
107+
proposalA.setOffsetForSorting(updatedOffset);
108+
proposalB.setOffsetForSorting(updatedOffset);
109+
expectedResorted.sort(comparator);
110+
111+
// We would like the filtered list to reflect the re-sorted order.
112+
assertEquals(expectedResorted, filteredWithoutResort);
113+
}
114+
115+
private static class StubProposal extends LSCompletionProposal {
116+
117+
private final String filter;
118+
119+
StubProposal(IDocument document, CompletionItem item, String filter) {
120+
super(document, 0, item, null, null, false);
121+
this.filter = filter;
122+
}
123+
124+
@Override
125+
public String getDocumentFilter() {
126+
return filter;
127+
}
128+
129+
@Override
130+
public String getDocumentFilter(int offset) {
131+
return filter;
132+
}
133+
134+
@Override
135+
public int getRankCategory() {
136+
return 5;
137+
}
138+
139+
@Override
140+
public int getRankScore() {
141+
return 0;
142+
}
143+
}
144+
145+
private static class VarFilterProposal extends LSCompletionProposal {
146+
147+
private final Map<Integer, String> filtersByOffset = new HashMap<>();
148+
private int sortOffset;
149+
150+
VarFilterProposal(IDocument document, CompletionItem item) {
151+
super(document, 0, item, null, null, false);
152+
}
153+
154+
void setFilterForOffset(int offset, String filter) {
155+
filtersByOffset.put(Integer.valueOf(offset), filter);
156+
}
157+
158+
void setOffsetForSorting(int offset) {
159+
this.sortOffset = offset;
160+
}
161+
162+
@Override
163+
public String getDocumentFilter() {
164+
String filter = filtersByOffset.get(Integer.valueOf(sortOffset));
165+
return filter != null ? filter : "";
166+
}
167+
168+
@Override
169+
public String getDocumentFilter(int offset) {
170+
String filter = filtersByOffset.get(Integer.valueOf(offset));
171+
return filter != null ? filter : "";
172+
}
173+
174+
@Override
175+
public int getRankCategory() {
176+
return 5;
177+
}
178+
179+
@Override
180+
public int getRankScore() {
181+
return 0;
182+
}
183+
}
184+
}

‎org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/CompletionProposalTools.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,12 @@ private CompletionProposalTools() {
2020
// to avoid instances, requested by sonar
2121
}
2222

23+
/**
24+
* Category used when the document filter does not meaningfully match
25+
* the completion filter (catch-all / no-match case).
26+
*/
27+
public static final int CATEGORY_NO_MATCH = 5;
28+
2329
/**
2430
* The portion of the document leading up to the cursor that is being used as a
2531
* filter for requesting completion assist
@@ -101,9 +107,9 @@ public static int getCategoryOfFilterMatch(String documentFilter, String complet
101107
documentFilter = documentFilter.toLowerCase();
102108
completionFilter = completionFilter.toLowerCase();
103109
int subIndex = completionFilter.indexOf(documentFilter);
104-
int topCategory = 5;
110+
int topCategory = CATEGORY_NO_MATCH;
105111
if (subIndex == -1) {
106-
return isSubstringFoundOrderedInString(documentFilter, completionFilter) ? 4 : 5;
112+
return isSubstringFoundOrderedInString(documentFilter, completionFilter) ? 4 : CATEGORY_NO_MATCH;
107113
}
108114
final int documentFilterLength = documentFilter.length();
109115
final int completionFilterLength = completionFilter.length();

‎org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSCompletionProposal.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ public int getRankCategory() {
258258
getFilterString());
259259
} catch (BadLocationException e) {
260260
LanguageServerPlugin.logError(e);
261-
rankCategory = 5;
261+
rankCategory = CompletionProposalTools.CATEGORY_NO_MATCH;
262262
}
263263
this.rankCategory = rankCategory;
264264
return rankCategory;

‎org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSCompletionProposalComparator.java‎

Lines changed: 40 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -13,43 +13,60 @@
1313
*******************************************************************************/
1414
package org.eclipse.lsp4e.operations.completion;
1515

16+
import static org.eclipse.lsp4e.operations.completion.CompletionProposalTools.*;
17+
1618
import java.util.Comparator;
1719

1820
import org.eclipse.jface.text.BadLocationException;
1921
import org.eclipse.lsp4e.LanguageServerPlugin;
2022

21-
final class LSCompletionProposalComparator implements Comparator<LSCompletionProposal> {
23+
public final class LSCompletionProposalComparator implements Comparator<LSCompletionProposal> {
2224
@Override
2325
public int compare(LSCompletionProposal o1, LSCompletionProposal o2) {
24-
try {
25-
int docFilterLen1 = o1.getDocumentFilter().length();
26-
int docFilterLen2 = o2.getDocumentFilter().length();
27-
if (docFilterLen1 > docFilterLen2) {
28-
return -1;
29-
} else if (docFilterLen1 < docFilterLen2) {
30-
return +1;
26+
int category1 = o1.getRankCategory();
27+
int category2 = o2.getRankCategory();
28+
29+
// Prefer proposals that have a stronger match (categories 1-4),
30+
// but use the document filter length only for those categories so that
31+
// completely unmatched proposals (category CATEGORY_NO_MATCH) are not affected.
32+
if (category1 < CATEGORY_NO_MATCH && category2 < CATEGORY_NO_MATCH) {
33+
try {
34+
int docFilterLen1 = o1.getDocumentFilter().length();
35+
int docFilterLen2 = o2.getDocumentFilter().length();
36+
if (docFilterLen1 > docFilterLen2) {
37+
return -1;
38+
} else if (docFilterLen1 < docFilterLen2) {
39+
return 1;
40+
}
41+
} catch (BadLocationException e) {
42+
LanguageServerPlugin.logError(e);
3143
}
32-
} catch (BadLocationException e) {
33-
LanguageServerPlugin.logError(e);
3444
}
35-
if (o1.getRankCategory() < o2.getRankCategory()) {
45+
46+
if (category1 < category2) {
3647
return -1;
37-
} else if (o1.getRankCategory() > o2.getRankCategory()) {
38-
return +1;
48+
} else if (category1 > category2) {
49+
return 1;
3950
}
40-
if ((o1.getRankCategory() < 5 && o2.getRankCategory() < 5)
41-
&& (!(o1.getRankScore() == -1 && o2.getRankScore() == -1))) {
42-
if (o2.getRankScore() == -1 || o1.getRankScore() < o2.getRankScore()) {
43-
return -1;
44-
} else if (o1.getRankScore() == -1 || o1.getRankScore() > o2.getRankScore()) {
45-
return +1;
51+
52+
if (category1 < CATEGORY_NO_MATCH /* && category2 < CATEGORY_NO_MATCH */) {
53+
int score1 = o1.getRankScore();
54+
int score2 = o2.getRankScore();
55+
if (!(score1 == -1 && score2 == -1)) {
56+
if (score1 == -1) {
57+
return 1;
58+
} else if (score2 == -1) {
59+
return -1;
60+
} else if (score1 < score2) {
61+
return -1;
62+
} else if (score1 > score2) {
63+
return 1;
64+
}
4665
}
4766
}
67+
4868
String c1 = o1.getSortText();
4969
String c2 = o2.getSortText();
50-
if (c1 == null) {
51-
return -1;
52-
}
5370
return c1.compareToIgnoreCase(c2);
5471
}
55-
}
72+
}

0 commit comments

Comments
 (0)