Skip to content

Commit 60fa066

Browse files
DanTupcommit-bot@chromium.org
DanTup
authored and
commit-bot@chromium.org
committed
Filter LSP completion to exclude symbols already imported via another URI
Change-Id: I39d807ef88b939feffcba2b333dd76595257c768 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105760 Commit-Queue: Danny Tuppeny <dantup@google.com> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
1 parent 8d39e34 commit 60fa066

3 files changed

Lines changed: 199 additions & 7 deletions

File tree

‎pkg/analysis_server/lib/src/lsp/handlers/handler_completion.dart‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,37 @@ class CompletionHandler
8787
));
8888
}
8989

90+
/// Build a list of existing imports so we can filter out any suggestions
91+
/// that resolve to the same underlying declared symbol.
92+
/// Map with key "elementName/elementDeclaringLibraryUri"
93+
/// Value is a set of imported URIs that import that element.
94+
Map<String, Set<String>> _buildLookupOfImportedSymbols(
95+
ResolvedUnitResult unit) {
96+
final alreadyImportedSymbols = <String, Set<String>>{};
97+
final importElementList = unit.libraryElement.imports;
98+
for (var import in importElementList) {
99+
final importedLibrary = import.importedLibrary;
100+
if (importedLibrary == null) continue;
101+
102+
for (var element in import.namespace.definedNames.values) {
103+
if (element.librarySource != null) {
104+
final declaringLibraryUri = element.librarySource.uri;
105+
final elementName = element.name;
106+
107+
final key =
108+
_createImportedSymbolKey(elementName, declaringLibraryUri);
109+
alreadyImportedSymbols.putIfAbsent(key, () => Set<String>());
110+
alreadyImportedSymbols[key]
111+
.add('${importedLibrary.librarySource.uri}');
112+
}
113+
}
114+
}
115+
return alreadyImportedSymbols;
116+
}
117+
118+
String _createImportedSymbolKey(String name, Uri declaringUri) =>
119+
'$name/$declaringUri';
120+
90121
Future<ErrorOr<List<CompletionItem>>> _getItems(
91122
TextDocumentClientCapabilitiesCompletion completionCapabilities,
92123
HashSet<CompletionItemKind> clientSupportedCompletionKinds,
@@ -142,6 +173,10 @@ class CompletionHandler
142173
unit,
143174
);
144175

176+
// Build a fast lookup for imported symbols so that we can filter out
177+
// duplicates.
178+
final alreadyImportedSymbols = _buildLookupOfImportedSymbols(unit);
179+
145180
includedSuggestionSets.forEach((includedSet) {
146181
final library = server.declarationsTracker.getLibrary(includedSet.id);
147182
if (library == null) {
@@ -157,7 +192,23 @@ class CompletionHandler
157192
// Filter to only the kinds we should return.
158193
.where((item) =>
159194
includedElementKinds.contains(protocolElementKind(item.kind)))
160-
.map((item) => declarationToCompletionItem(
195+
.where((item) {
196+
// Check existing imports to ensure we don't already import
197+
// this element (this exact element from its declaring
198+
// library, not just something with the same name). If we do
199+
// we'll want to skip it.
200+
final declaringUri = item.parent != null
201+
? item.parent.locationLibraryUri
202+
: item.locationLibraryUri;
203+
final key = _createImportedSymbolKey(item.name, declaringUri);
204+
final importingUris = alreadyImportedSymbols[key];
205+
206+
// Keep it only if there are either:
207+
// - no URIs importing it
208+
// - the URIs importing it include this one
209+
return importingUris == null ||
210+
importingUris.contains('${library.uri}');
211+
}).map((item) => declarationToCompletionItem(
161212
completionCapabilities,
162213
clientSupportedCompletionKinds,
163214
unit.path,

‎pkg/analysis_server/lib/src/lsp/handlers/handler_completion_resolve.dart‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ class CompletionResolveHandler
113113
);
114114
}
115115

116-
var newLabel = item.label;
116+
var newInsertText = item.label;
117117
final builder = DartChangeBuilder(session);
118118
await builder.addFileEdit(libraryPath, (builder) {
119119
final result = builder.importLibraryElement(
@@ -124,7 +124,7 @@ class CompletionResolveHandler
124124
requestedElement: requestedElement,
125125
);
126126
if (result.prefix != null) {
127-
newLabel = '${result.prefix}.$newLabel';
127+
newInsertText = '${result.prefix}.$newInsertText';
128128
}
129129
});
130130

@@ -155,9 +155,9 @@ class CompletionResolveHandler
155155
final documentation = asStringOrMarkupContent(formats, dartDoc);
156156

157157
return success(CompletionItem(
158-
newLabel,
158+
item.label,
159159
item.kind,
160-
data.displayUri != null
160+
data.displayUri != null && thisFilesChanges.isNotEmpty
161161
? "Auto import from '${data.displayUri}'\n\n${item.detail ?? ''}"
162162
.trim()
163163
: item.detail,
@@ -166,13 +166,13 @@ class CompletionResolveHandler
166166
item.preselect,
167167
item.sortText,
168168
item.filterText,
169-
newLabel,
169+
newInsertText,
170170
item.insertTextFormat,
171171
new TextEdit(
172172
// TODO(dantup): If `clientSupportsSnippets == true` then we should map
173173
// `selection` in to a snippet (see how Dart Code does this).
174174
toRange(lineInfo, item.data.rOffset, item.data.rLength),
175-
newLabel,
175+
newInsertText,
176176
),
177177
thisFilesChanges
178178
.expand((change) =>

‎pkg/analysis_server/test/lsp/completion_test.dart‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,16 @@ main() {
1717

1818
@reflectiveTest
1919
class CompletionTest extends AbstractLspAnalysisServerTest {
20+
expectAutoImportCompletion(List<CompletionItem> items, String file) {
21+
expect(
22+
items.singleWhere(
23+
(c) => c.detail?.contains("Auto import from '$file'") ?? false,
24+
orElse: () => null,
25+
),
26+
isNotNull,
27+
);
28+
}
29+
2030
test_completionKinds_default() async {
2131
newFile(join(projectFolderPath, 'file.dart'));
2232
newFolder(join(projectFolderPath, 'folder'));
@@ -296,6 +306,137 @@ main() {
296306
'''));
297307
}
298308

309+
test_suggestionSets_doesNotFilterSymbolsWithSameName() async {
310+
// Classes here are not re-exports, so should not be filtered out.
311+
newFile(
312+
join(projectFolderPath, 'source_file1.dart'),
313+
content: 'class MyDuplicatedClass {}',
314+
);
315+
newFile(
316+
join(projectFolderPath, 'source_file2.dart'),
317+
content: 'class MyDuplicatedClass {}',
318+
);
319+
newFile(
320+
join(projectFolderPath, 'source_file3.dart'),
321+
content: 'class MyDuplicatedClass {}',
322+
);
323+
324+
final content = '''
325+
main() {
326+
MyDuplicated^
327+
}
328+
''';
329+
330+
final initialAnalysis = waitForAnalysisComplete();
331+
await initialize(
332+
workspaceCapabilities:
333+
withApplyEditSupport(emptyWorkspaceClientCapabilities));
334+
await openFile(mainFileUri, withoutMarkers(content));
335+
await initialAnalysis;
336+
final res = await getCompletion(mainFileUri, positionFromMarker(content));
337+
338+
final completions =
339+
res.where((c) => c.label == 'MyDuplicatedClass').toList();
340+
expect(completions, hasLength(3));
341+
342+
// Resolve the completions so we can get the auto-import text.
343+
final resolvedCompletions =
344+
await Future.wait(completions.map(resolveCompletion));
345+
346+
expectAutoImportCompletion(resolvedCompletions, '../source_file1.dart');
347+
expectAutoImportCompletion(resolvedCompletions, '../source_file2.dart');
348+
expectAutoImportCompletion(resolvedCompletions, '../source_file3.dart');
349+
}
350+
351+
test_suggestionSets_filtersOutAlreadyImportedSymbols() async {
352+
newFile(
353+
join(projectFolderPath, 'source_file.dart'),
354+
content: '''
355+
class MyExportedClass {}
356+
''',
357+
);
358+
newFile(
359+
join(projectFolderPath, 'reexport1.dart'),
360+
content: '''
361+
export 'source_file.dart';
362+
''',
363+
);
364+
newFile(
365+
join(projectFolderPath, 'reexport2.dart'),
366+
content: '''
367+
export 'source_file.dart';
368+
''',
369+
);
370+
371+
final content = '''
372+
import '../reexport1.dart';
373+
374+
main() {
375+
MyExported^
376+
}
377+
''';
378+
379+
final initialAnalysis = waitForAnalysisComplete();
380+
await initialize(
381+
workspaceCapabilities:
382+
withApplyEditSupport(emptyWorkspaceClientCapabilities));
383+
await openFile(mainFileUri, withoutMarkers(content));
384+
await initialAnalysis;
385+
final res = await getCompletion(mainFileUri, positionFromMarker(content));
386+
387+
final completions = res.where((c) => c.label == 'MyExportedClass').toList();
388+
expect(completions, hasLength(1));
389+
final resolved = await resolveCompletion(completions.first);
390+
// It should not include auto-import text since it's already imported.
391+
expect(resolved.detail, isNull);
392+
}
393+
394+
test_suggestionSets_includesReexportedSymbolsForEachFile() async {
395+
newFile(
396+
join(projectFolderPath, 'source_file.dart'),
397+
content: '''
398+
class MyExportedClass {}
399+
''',
400+
);
401+
newFile(
402+
join(projectFolderPath, 'reexport1.dart'),
403+
content: '''
404+
export 'source_file.dart';
405+
''',
406+
);
407+
newFile(
408+
join(projectFolderPath, 'reexport2.dart'),
409+
content: '''
410+
export 'source_file.dart';
411+
''',
412+
);
413+
414+
final content = '''
415+
main() {
416+
MyExported^
417+
}
418+
''';
419+
420+
final initialAnalysis = waitForAnalysisComplete();
421+
await initialize(
422+
workspaceCapabilities:
423+
withApplyEditSupport(emptyWorkspaceClientCapabilities));
424+
await openFile(mainFileUri, withoutMarkers(content));
425+
await initialAnalysis;
426+
final res = await getCompletion(mainFileUri, positionFromMarker(content));
427+
428+
final completions = res.where((c) => c.label == 'MyExportedClass').toList();
429+
expect(completions, hasLength(3));
430+
431+
// Resolve the completions so we can get the auto-import text.
432+
final resolvedCompletions =
433+
await Future.wait(completions.map(resolveCompletion));
434+
435+
expectAutoImportCompletion(resolvedCompletions, '../source_file.dart');
436+
expectAutoImportCompletion(resolvedCompletions, '../reexport1.dart');
437+
expectAutoImportCompletion(resolvedCompletions, '../reexport2.dart');
438+
}
439+
299440
test_suggestionSets_insertsIntoPartFiles() async {
300441
// File we'll be adding an import for.
301442
newFile(

0 commit comments

Comments
 (0)