[DAS] Fix auto-complete update combinators
We already have a quick-fix that allows us to use the same existing import by editing the combinators. The auto-completer now should do the same.
Bug: https://github.com/Dart-Code/Dart-Code/issues/5238
Change-Id: I0f3a50299abb9890370a6c8934b49036f7e03196
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/404525
Auto-Submit: Felipe Morschel <git@fmorschel.dev>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_completion_resolve.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_completion_resolve.dart
index 73065c5..75bfb67 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handler_completion_resolve.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_completion_resolve.dart
@@ -86,10 +86,17 @@
return cancelled(token);
}
+ var element =
+ elementReference != null
+ ? await ElementLocation2.decode(
+ elementReference,
+ ).locateIn(session)
+ : null;
+
var builder = ChangeBuilder(session: session);
await builder.addDartFileEdit(file, (builder) {
for (var uri in importUris) {
- builder.importLibraryElement(uri);
+ builder.importLibraryElement(uri, showName: element?.name3);
}
});
@@ -123,12 +130,6 @@
// Look up documentation if we can get an element for this item.
Either2<MarkupContent, String>? documentation;
- var element =
- elementReference != null
- ? await ElementLocation2.decode(
- elementReference,
- ).locateIn(session)
- : null;
if (element != null) {
var formats = clientCapabilities.completionDocumentationFormats;
var dartDocInfo = server.getDartdocDirectiveInfoForSession(session);
diff --git a/pkg/analysis_server/lib/src/services/correction/dart/import_library.dart b/pkg/analysis_server/lib/src/services/correction/dart/import_library.dart
index aa3f692..f8bec04 100644
--- a/pkg/analysis_server/lib/src/services/correction/dart/import_library.dart
+++ b/pkg/analysis_server/lib/src/services/correction/dart/import_library.dart
@@ -96,6 +96,8 @@
for (var instantiatedExtension in instantiatedExtensions) {
// If the import has a combinator that needs to be updated, then offer
// to update it.
+ // TODO(FMorschel): We should fix all combinators for the import, if
+ // we don't, we may not import at all.
var combinators = import.combinators;
if (combinators.length == 1) {
var combinator = combinators[0];
@@ -624,7 +626,7 @@
_uriText = builder.importLibraryWithAbsoluteUri(
_library,
prefix: _prefix,
- shownName: _show,
+ showName: _show,
useShow: _show != null,
);
}
@@ -873,7 +875,7 @@
_uriText = builder.importLibraryWithRelativeUri(
_library,
prefix: _prefix,
- shownName: _show,
+ showName: _show,
useShow: _show != null,
);
}
diff --git a/pkg/analysis_server/test/lsp/completion_dart_test.dart b/pkg/analysis_server/test/lsp/completion_dart_test.dart
index 6bbbb75..59cc809 100644
--- a/pkg/analysis_server/test/lsp/completion_dart_test.dart
+++ b/pkg/analysis_server/test/lsp/completion_dart_test.dart
@@ -3663,6 +3663,131 @@
expect(res.isIncomplete, isTrue);
}
+ Future<void> test_unimportedSymbols_libraryImported_hidingMultiple() async {
+ newFile(join(projectFolderPath, 'lib', 'my_classes.dart'), '''
+class MyClass1 {}
+class MyClass2 {}
+class MyClass3 {}
+''');
+
+ var content = '''
+import 'package:test/my_classes.dart' hide MyClass1, MyClass2;
+void f() {
+ MyClas^
+}
+''';
+
+ var expectedContent = '''
+import 'package:test/my_classes.dart' hide MyClass2;
+void f() {
+ MyClass1
+}
+''';
+
+ var completionLabel = 'MyClass1';
+
+ await _checkCompletionEdits(
+ mainFileUri,
+ content,
+ completionLabel,
+ expectedContent,
+ );
+ }
+
+ Future<void> test_unimportedSymbols_libraryImported_hidingOne() async {
+ newFile(join(projectFolderPath, 'lib', 'my_classes.dart'), '''
+class MyClass1 {}
+class MyClass2 {}
+''');
+
+ var content = '''
+import 'package:test/my_classes.dart' hide MyClass1;
+void f() {
+ MyClas^
+}
+''';
+
+ var expectedContent = '''
+import 'package:test/my_classes.dart';
+void f() {
+ MyClass1
+}
+''';
+
+ var completionLabel = 'MyClass1';
+
+ await _checkCompletionEdits(
+ mainFileUri,
+ content,
+ completionLabel,
+ expectedContent,
+ );
+ }
+
+ Future<void> test_unimportedSymbols_libraryImported_showingOther() async {
+ newFile(join(projectFolderPath, 'lib', 'my_classes.dart'), '''
+class MyClass1 {}
+class MyClass2 {}
+''');
+
+ var content = '''
+import 'package:test/my_classes.dart' show MyClass2;
+void f() {
+ MyClas^
+}
+''';
+
+ var expectedContent = '''
+import 'package:test/my_classes.dart' show MyClass1, MyClass2;
+void f() {
+ MyClass1
+}
+''';
+
+ var completionLabel = 'MyClass1';
+
+ await _checkCompletionEdits(
+ mainFileUri,
+ content,
+ completionLabel,
+ expectedContent,
+ );
+ }
+
+ // Code completion doesn't include prefixes for auto-imports so when an
+ // auto-import is added it must be unprefixed even if the library exists with
+ // a prefix (we cannot modify the inserted text during resolve).
+ Future<void> test_unimportedSymbols_libraryImported_withPrefix() async {
+ newFile(join(projectFolderPath, 'lib', 'my_classes.dart'), '''
+class MyClass1 {}
+class MyClass2 {}
+''');
+
+ var content = '''
+import 'package:test/my_classes.dart' as p1 show MyClass2;
+void f() {
+ MyClas^
+}
+''';
+
+ var expectedContent = '''
+import 'package:test/my_classes.dart' as p1 show MyClass2;
+import 'package:test/my_classes.dart';
+void f() {
+ MyClass1
+}
+''';
+
+ var completionLabel = 'MyClass1';
+
+ await _checkCompletionEdits(
+ mainFileUri,
+ content,
+ completionLabel,
+ expectedContent,
+ );
+ }
+
/// This test reproduces a bug where the pathKey hash used in
/// available_declarations.dart would not change with the contents of the file
/// (as it always used 0 as the modification stamp) which would prevent
diff --git a/pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart b/pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
index ae41b16..ee84cee 100644
--- a/pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
+++ b/pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
@@ -1608,7 +1608,7 @@
var newImport = elementLibrariesToImport[element] = _importLibrary(
uriToImport,
isExplicitImport: false,
- shownName: element.name3,
+ showName: element.name3,
useShow: useShow,
);
@@ -1620,15 +1620,24 @@
}
@override
- String importLibrary(Uri uri,
- {String? prefix, String? showName, bool useShow = false}) {
+ String importLibrary(
+ Uri uri, {
+ String? prefix,
+ String? showName,
+ bool useShow = false,
+ }) {
return _importLibrary(uri,
- prefix: prefix, shownName: showName, useShow: useShow)
+ prefix: prefix, showName: showName, useShow: useShow)
.uriText;
}
@override
- ImportLibraryElementResult importLibraryElement(Uri uri) {
+ ImportLibraryElementResult importLibraryElement(
+ Uri uri, {
+ String? prefix,
+ String? showName,
+ bool useShow = false,
+ }) {
if (resolvedUnit.libraryElement2.uri == uri) {
return ImportLibraryElementResultImpl(null);
}
@@ -1637,24 +1646,32 @@
in resolvedUnit.libraryElement2.firstFragment.libraryImports2) {
var importedLibrary = import.importedLibrary2;
if (importedLibrary != null && importedLibrary.uri == uri) {
- return ImportLibraryElementResultImpl(import.prefix2?.element.name3);
+ var importPrefix = import.prefix2?.element.name3;
+ if (import.hasCombinator) {
+ if (importPrefix == null && showName != null) {
+ _handleCombinators(import, showName);
+ return ImportLibraryElementResultImpl(importPrefix);
+ }
+ } else {
+ return ImportLibraryElementResultImpl(importPrefix);
+ }
}
}
- importLibrary(uri);
+ importLibrary(uri, prefix: prefix, showName: showName, useShow: useShow);
return ImportLibraryElementResultImpl(null);
}
String importLibraryWithAbsoluteUri(
Uri uri, {
String? prefix,
- String? shownName,
+ String? showName,
bool useShow = false,
}) {
return _importLibrary(
uri,
prefix: prefix,
- shownName: shownName,
+ showName: showName,
useShow: useShow,
forceAbsolute: true,
).uriText;
@@ -1663,13 +1680,13 @@
String importLibraryWithRelativeUri(
Uri uri, {
String? prefix,
- String? shownName,
+ String? showName,
bool useShow = false,
}) {
return _importLibrary(
uri,
prefix: prefix,
- shownName: shownName,
+ showName: showName,
useShow: useShow,
forceAbsolute: true,
forceRelative: true,
@@ -2281,6 +2298,44 @@
return uri.toString();
}
+ /// If the [import] already has combinators, update them to import [showName].
+ ///
+ /// If the combinator is a `show`, then [showName] will be added to the list
+ /// of shown names.
+ ///
+ /// If the combinator is a `hide`, then [showName] will be removed from the
+ /// list of hidden names.
+ void _handleCombinators(LibraryImport import, String showName) {
+ for (var show in import.showCombinators) {
+ var names = show.shownNames.toList();
+ if (!names.contains(showName)) {
+ names.add(showName);
+ names.sort();
+ addSimpleReplacement(
+ range.startOffsetEndOffset(show.offset, show.end),
+ 'show ${names.join(', ')}',
+ );
+ }
+ }
+ for (var hide in import.hideCombinators) {
+ var names = hide.hiddenNames.toList();
+ if (names.contains(showName)) {
+ names.remove(showName);
+ if (names.isEmpty) {
+ addSimpleReplacement(
+ range.startOffsetEndOffset(hide.offset - 1, hide.end),
+ '',
+ );
+ } else {
+ addSimpleReplacement(
+ range.startOffsetEndOffset(hide.offset, hide.end),
+ 'hide ${names.join(', ')}',
+ );
+ }
+ }
+ }
+ }
+
/// Arranges to have an import added for the library with the given [uri].
///
/// [uri] may be converted from an absolute URI to a relative URI depending on
@@ -2291,28 +2346,28 @@
/// If [prefix] is null, will use [importPrefixGenerator] to generate one or
/// reuse an existing prefix for this import.
///
- /// If [shownName] is supplied then any new import will show only this
+ /// If [showName] is supplied then any new import will show only this
/// element, or if an import already exists it will be added to 'show' or
/// removed from 'hide' if appropriate.
_LibraryImport _importLibrary(
Uri uri, {
String? prefix,
- String? shownName,
+ String? showName,
bool isExplicitImport = true,
bool useShow = false,
bool forceAbsolute = false,
bool forceRelative = false,
}) {
var import = (libraryChangeBuilder ?? this).librariesToImport[uri];
- var shownNames = <List<String>>[];
- var hiddenNames = <List<String>>[];
+ var existingShownNames = <List<String>>[];
+ var existingHiddenNames = <List<String>>[];
if (import != null) {
if (prefix != null) {
import.prefixes.add(prefix);
}
- if (shownName != null) {
- import.ensureShown(shownName, useShow: useShow);
+ if (showName != null) {
+ import.ensureShown(showName, useShow: useShow);
}
// If this was an explicit import request, ensure the existing import
// is marked as such so it cannot be removed by other optimizations.
@@ -2342,9 +2397,9 @@
for (var combinator in element.combinators) {
switch (combinator) {
case ShowElementCombinator():
- shownNames.add(combinator.shownNames.toList());
+ existingShownNames.add(combinator.shownNames.toList());
case HideElementCombinator():
- hiddenNames.add(combinator.hiddenNames.toList());
+ existingHiddenNames.add(combinator.hiddenNames.toList());
}
}
}
@@ -2354,11 +2409,11 @@
uriText: uriText,
prefix: prefix ?? '',
isExplicitlyImported: isExplicitImport,
- shownNames: shownNames,
- hiddenNames: hiddenNames,
+ shownNames: existingShownNames,
+ hiddenNames: existingHiddenNames,
);
- if (shownName != null) {
- import.ensureShown(shownName, useShow: useShow);
+ if (showName != null) {
+ import.ensureShown(showName, useShow: useShow);
}
(libraryChangeBuilder ?? this).librariesToImport[uri] = import;
}
@@ -2802,3 +2857,13 @@
};
}
}
+
+extension on LibraryImport {
+ bool get hasCombinator => combinators.isNotEmpty;
+
+ Iterable<HideElementCombinator> get hideCombinators =>
+ combinators.whereType<HideElementCombinator>();
+
+ Iterable<ShowElementCombinator> get showCombinators =>
+ combinators.whereType<ShowElementCombinator>();
+}
diff --git a/pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart b/pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart
index 2314e49..985c5e0 100644
--- a/pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart
+++ b/pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart
@@ -445,11 +445,28 @@
/// Ensures that the library with the given [uri] is imported.
///
- /// If there is already an import for the requested library, returns the
- /// import prefix of the existing import directive.
- ///
/// If there is no existing import, a new import is added.
- ImportLibraryElementResult importLibraryElement(Uri uri);
+ ///
+ /// If a [prefix] is provided new imports will use it in the import directive.
+ ///
+ /// If there is already an import for the requested library, returns the
+ /// import prefix of the existing import directive unless [showName] is
+ /// provided, in which case the rules below apply.
+ ///
+ /// If [showName] is provided, one of the following can occur:
+ /// - If the library is already imported with a prefix, a new import with no
+ /// prefix will be added.
+ /// - If the library is already imported without a prefix, the existing import
+ /// will be modified to show the given [showName] if it contains a combinator.
+ ///
+ /// If [useShow] is `true`, new imports will show only the given [showName],
+ /// instead of importing the library without a show clause.
+ ImportLibraryElementResult importLibraryElement(
+ Uri uri, {
+ String? prefix,
+ String? showName,
+ bool useShow = false,
+ });
/// Returns whether the given library [uri] is already imported or will be
/// imported by a scheduled edit.
diff --git a/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_dart_test.dart b/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_dart_test.dart
index 7c873d6..78b15d9d 100644
--- a/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_dart_test.dart
+++ b/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_dart_test.dart
@@ -7,6 +7,7 @@
import 'package:analyzer/dart/element/element2.dart';
import 'package:analyzer/dart/element/nullability_suffix.dart';
import 'package:analyzer/dart/element/type.dart';
+import 'package:analyzer/file_system/file_system.dart';
import 'package:analyzer/source/source_range.dart';
import 'package:analyzer/src/dart/element/inheritance_manager3.dart';
import 'package:analyzer/src/test_utilities/find_node.dart';
@@ -27,6 +28,7 @@
defineReflectiveTests(DartFileEditBuilderImplTest);
defineReflectiveTests(DartLinkedEditBuilderImplTest);
defineReflectiveTests(ImportLibraryTest);
+ defineReflectiveTests(ImportLibraryElementTest);
defineReflectiveTests(WriteOverrideTest);
});
}
@@ -2886,6 +2888,206 @@
}
@reflectiveTest
+class ImportLibraryElementTest extends AbstractContextTest
+ with DartChangeBuilderMixin {
+ Future<void> test_show_name_differentCombinators() async {
+ _createOther();
+ await _assertImportLibraryElement(
+ initialCode: '''
+import 'dart:aaa';
+import 'other.dart' hide B, C show A, D;
+
+class A {}
+''',
+ uriString: 'package:test/other.dart',
+ showName: 'B',
+ expectedCode: '''
+import 'dart:aaa';
+import 'other.dart' hide C show A, B, D;
+
+class A {}
+''',
+ );
+ }
+
+ Future<void> test_show_name_hideCombinator_multipleNames() async {
+ _createOther();
+ await _assertImportLibraryElement(
+ initialCode: '''
+import 'dart:aaa';
+import 'other.dart' hide A, B;
+
+class A {}
+''',
+ uriString: 'package:test/other.dart',
+ showName: 'B',
+ expectedCode: '''
+import 'dart:aaa';
+import 'other.dart' hide A;
+
+class A {}
+''',
+ );
+ }
+
+ Future<void> test_show_name_hideCombinator_name() async {
+ _createOther();
+ await _assertImportLibraryElement(
+ initialCode: '''
+import 'dart:aaa';
+import 'other.dart' hide B;
+
+class A {}
+''',
+ uriString: 'package:test/other.dart',
+ showName: 'B',
+ expectedCode: '''
+import 'dart:aaa';
+import 'other.dart';
+
+class A {}
+''',
+ );
+ }
+
+ Future<void> test_show_name_multipleHide() async {
+ _createOther();
+ await _assertImportLibraryElement(
+ initialCode: '''
+import 'dart:aaa';
+import 'other.dart' hide B hide A, D;
+
+class A {}
+''',
+ uriString: 'package:test/other.dart',
+ showName: 'B',
+ expectedCode: '''
+import 'dart:aaa';
+import 'other.dart' hide A, D;
+
+class A {}
+''',
+ );
+ }
+
+ Future<void> test_show_name_multipleShow() async {
+ _createOther();
+ await _assertImportLibraryElement(
+ initialCode: '''
+import 'dart:aaa';
+import 'other.dart' show B show A, D;
+
+class A {}
+''',
+ uriString: 'package:test/other.dart',
+ showName: 'B',
+ expectedCode: '''
+import 'dart:aaa';
+import 'other.dart' show B show A, B, D;
+
+class A {}
+''',
+ );
+ }
+
+ Future<void> test_show_name_no_combinator() async {
+ _createOther();
+ await _assertImportLibraryElement(
+ initialCode: '''
+import 'dart:aaa';
+
+class A {}
+''',
+ uriString: 'package:test/other.dart',
+ showName: 'B',
+ expectedCode: '''
+import 'dart:aaa';
+
+import 'package:test/other.dart';
+
+class A {}
+''',
+ );
+ }
+
+ Future<void> test_show_name_prefix() async {
+ _createOther();
+ await _assertImportLibraryElement(
+ initialCode: '''
+import 'dart:aaa';
+
+import 'package:test/other.dart' as b;
+
+class A {}
+''',
+ uriString: 'package:test/other.dart',
+ showName: 'B',
+ expectedCode: '''
+import 'dart:aaa';
+
+import 'package:test/other.dart' as b;
+
+class A {}
+''',
+ );
+ }
+
+ Future<void> test_show_name_showCombinator() async {
+ _createOther();
+ await _assertImportLibraryElement(
+ initialCode: '''
+import 'dart:aaa';
+import 'other.dart' show A;
+
+class A {}
+''',
+ uriString: 'package:test/other.dart',
+ showName: 'B',
+ expectedCode: '''
+import 'dart:aaa';
+import 'other.dart' show A, B;
+
+class A {}
+''',
+ );
+ }
+
+ Future<void> _assertImportLibraryElement({
+ required String initialCode,
+ required String uriString,
+ required String expectedCode,
+ String? prefix,
+ String? showName,
+ bool createEditsForImports = true,
+ }) async {
+ var path = convertPath('/home/test/lib/test.dart');
+ addSource(path, initialCode);
+ var builder = await newBuilder();
+ await builder.addDartFileEdit(path,
+ createEditsForImports: createEditsForImports, (builder) {
+ var uri = Uri.parse(uriString);
+ builder.importLibraryElement(uri, prefix: prefix, showName: showName);
+ });
+
+ var resultCode = initialCode;
+ var edits = getEdits(builder);
+ for (var edit in edits) {
+ resultCode = edit.apply(resultCode);
+ }
+ expect(resultCode, expectedCode);
+ }
+
+ File _createOther() {
+ return newFile(convertPath('/home/test/lib/other.dart'), '''
+class A {}
+class B {}
+class C {}
+class D {}
+''');
+ }
+}
+
+@reflectiveTest
class ImportLibraryTest extends AbstractContextTest
with DartChangeBuilderMixin {
Future<void> test_dart_afterDart_last() async {
