[analysis_server] Add support for adding imports with show/hide + update Move to File
Change-Id: I5c732c969a9a89af76b82e60ac652cf00bb878bc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/306920
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analysis_server/lib/src/services/refactoring/move_top_level_to_file.dart b/pkg/analysis_server/lib/src/services/refactoring/move_top_level_to_file.dart
index 0208349..e228d86 100644
--- a/pkg/analysis_server/lib/src/services/refactoring/move_top_level_to_file.dart
+++ b/pkg/analysis_server/lib/src/services/refactoring/move_top_level_to_file.dart
@@ -39,7 +39,7 @@
MoveTopLevelToFile(super.context);
@override
- bool get isExperimental => true;
+ bool get isExperimental => false;
@override
CodeActionKind get kind => DartCodeActionKind.RefactorMove;
@@ -183,17 +183,20 @@
void _addImportsForMovingDeclarations(
DartFileEditBuilder builder, ImportAnalyzer analyzer) {
for (var entry in analyzer.movingReferences.entries) {
+ var element = entry.key;
var imports = entry.value;
for (var import in imports) {
var library = import.importedLibrary;
if (library == null || library.isDartCore) {
continue;
}
- // TODO(dantup): This does not support show/hide. We should be able to
- // pass them in (and have them merge with any existing imports or
- // pending imports).
- builder.importLibrary(library.source.uri,
- prefix: import.prefix?.element.name);
+ var hasShowCombinator =
+ import.combinators.whereType<ShowElementCombinator>().isNotEmpty;
+ builder.importLibrary(
+ library.source.uri,
+ prefix: import.prefix?.element.name,
+ showName: hasShowCombinator ? element.name : null,
+ );
}
}
}
diff --git a/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart b/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart
index 081d9fd..9a8a252 100644
--- a/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart
+++ b/pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart
@@ -944,6 +944,119 @@
modifiedOtherFileContent: modifiedOtherFileContent);
}
+ /// Test moving declarations to a file that imports a library that exports a
+ /// referenced declaration, but currently hides it.
+ Future<void> test_imports_showHide_destinationHides() async {
+ var libFilePath = join(projectFolderPath, 'lib', 'a.dart');
+ var srcFilePath = join(projectFolderPath, 'lib', 'src', 'a.dart');
+ var destinationFileName = 'moving.dart';
+ var destinationFilePath =
+ join(projectFolderPath, 'lib', destinationFileName);
+ addSource(libFilePath, 'export "src/a.dart";');
+ addSource(srcFilePath, 'class A {}');
+ addSource(destinationFilePath, '''
+import 'package:test/a.dart' hide A;
+''');
+ var originalSource = '''
+import 'package:test/a.dart';
+
+A? staying;
+A? mov^ing;
+''';
+ var modifiedSource = '''
+import 'package:test/a.dart';
+
+A? staying;
+''';
+ var declarationName = 'moving';
+
+ var expectedDestinationContent = '''
+import 'package:test/a.dart' hide A;
+import 'package:test/a.dart';
+
+A? moving;
+''';
+ await _singleDeclaration(
+ originalSource: originalSource,
+ modifiedSource: modifiedSource,
+ declarationName: declarationName,
+ newFileName: destinationFileName,
+ newFileContent: expectedDestinationContent);
+ }
+
+ /// Test moving declarations to a file that imports a library that exports a
+ /// referenced declaration, but currently hides it.
+ Future<void> test_imports_showHide_destinationHides_sourceShows() async {
+ var libFilePath = join(projectFolderPath, 'lib', 'a.dart');
+ var srcFilePath = join(projectFolderPath, 'lib', 'src', 'a.dart');
+ var destinationFileName = 'moving.dart';
+ var destinationFilePath =
+ join(projectFolderPath, 'lib', destinationFileName);
+ addSource(libFilePath, 'export "src/a.dart";');
+ addSource(srcFilePath, 'class A {}');
+ addSource(destinationFilePath, '''
+import 'package:test/a.dart' hide A;
+''');
+ var originalSource = '''
+import 'package:test/a.dart' show A;
+
+A? staying;
+A? mov^ing;
+''';
+ var modifiedSource = '''
+import 'package:test/a.dart' show A;
+
+A? staying;
+''';
+ var declarationName = 'moving';
+
+ var expectedDestinationContent = '''
+import 'package:test/a.dart' hide A;
+import 'package:test/a.dart' show A;
+
+A? moving;
+''';
+ await _singleDeclaration(
+ originalSource: originalSource,
+ modifiedSource: modifiedSource,
+ declarationName: declarationName,
+ newFileName: destinationFileName,
+ newFileContent: expectedDestinationContent);
+ }
+
+ /// Test that if the moving declaration was imported with 'show' that any new
+ /// import added to the destination also only shows it.
+ Future<void> test_imports_showHide_sourceShows() async {
+ var libFilePath = join(projectFolderPath, 'lib', 'a.dart');
+ var srcFilePath = join(projectFolderPath, 'lib', 'src', 'a.dart');
+ addSource(libFilePath, 'export "src/a.dart";');
+ addSource(srcFilePath, 'class A {}');
+ var originalSource = '''
+import 'package:test/a.dart' show A;
+
+A? staying;
+A? mov^ing;
+''';
+ var modifiedSource = '''
+import 'package:test/a.dart' show A;
+
+A? staying;
+''';
+ var declarationName = 'moving';
+ var newFileName = 'moving.dart';
+ var newFileContent = '''
+import 'package:test/a.dart' show A;
+
+A? moving;
+''';
+ await _singleDeclaration(
+ originalSource: originalSource,
+ modifiedSource: modifiedSource,
+ declarationName: declarationName,
+ newFileName: newFileName,
+ newFileContent: newFileContent);
+ }
+
Future<void> test_kind_class() async {
var originalSource = '''
class A {}
@@ -1084,6 +1197,12 @@
await expectCodeAction(simpleClassRefactorTitle);
}
+ Future<void> test_protocol_available_withoutExperimentalOptIn() async {
+ addTestSource(simpleClassContent);
+ await initializeServer(experimentalOptInFlag: false);
+ await expectCodeAction(simpleClassRefactorTitle);
+ }
+
Future<void> test_protocol_clientModifiedValues() async {
addTestSource(simpleClassContent);
@@ -1105,12 +1224,6 @@
expect(content[newFilePath], expectedNewFileContent);
}
- Future<void> test_protocol_unavailable_withoutExperimentalOptIn() async {
- addTestSource(simpleClassContent);
- await initializeServer(experimentalOptInFlag: false);
- await expectNoCodeAction(simpleClassRefactorTitle);
- }
-
Future<void> test_protocol_unavailable_withoutFileCreateSupport() async {
addTestSource(simpleClassContent);
await initializeServer(fileCreateSupport: false);
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 c6e10cc..2b14f8c 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
@@ -1549,9 +1549,21 @@
///
/// If the library [element] is declared in is inside the `src` folder, will
/// try to locate a public URI to import instead.
+ ///
+ /// If [useShow] is `true`, new imports will be added that `show` only the
+ /// requested element (or if there is a pending import for the library, added
+ /// to its `show` combinator).
Future<void> importElementLibrary(Element element,
- {Map<Element, LibraryElement?>? resultCache}) async {
- if (_isDefinedLocally(element) || _getImportElement(element) != null) {
+ {Map<Element, LibraryElement?>? resultCache,
+ bool useShow = false}) async {
+ if (_isDefinedLocally(element)) {
+ return;
+ }
+
+ var existingImport = _getImportElement(element);
+ var name = element.name;
+ if (existingImport != null && name != null) {
+ existingImport.ensureShown(name);
return;
}
@@ -1559,21 +1571,23 @@
await TopLevelDeclarations(resolvedUnit)
.publiclyExporting(element, resultCache: resultCache);
if (libraryWithElement != null) {
- _elementLibrariesToImport[element] =
- _importLibrary(libraryWithElement.source.uri);
+ _elementLibrariesToImport[element] = _importLibrary(
+ libraryWithElement.source.uri,
+ showName: useShow ? element.name : null,
+ );
return;
}
// If we didn't find one, use the original URI.
var uri = element.source?.uri;
if (uri != null) {
- _importLibrary(uri);
+ _importLibrary(uri, showName: useShow ? element.name : null);
}
}
@override
- String importLibrary(Uri uri, {String? prefix}) {
- return _importLibrary(uri, prefix: prefix).uriText;
+ String importLibrary(Uri uri, {String? prefix, String? showName}) {
+ return _importLibrary(uri, prefix: prefix, showName: showName).uriText;
}
@override
@@ -1680,6 +1694,14 @@
builder.write(' as ');
builder.write(prefix);
}
+ if (import.showNames.isNotEmpty) {
+ builder.write(' show ');
+ builder.write(import.showNames.join(', '));
+ }
+ if (import.hideNames.isNotEmpty) {
+ builder.write(' hide ');
+ builder.write(import.hideNames.join(', '));
+ }
builder.write(';');
}
}
@@ -1885,8 +1907,9 @@
var definedNames = import.namespace.definedNames;
if (definedNames.containsValue(element)) {
return _LibraryImport(
- uriText: import.librarySource.uri.toString(),
- prefix: import.prefix?.element.displayName ?? '');
+ uriText: import.librarySource.uri.toString(),
+ prefix: import.prefix?.element.displayName ?? '',
+ );
}
}
@@ -1945,9 +1968,14 @@
/// If [prefix] is an empty string, adds the import without a prefix.
/// If [prefix] is null, will use [importPrefixGenerator] to generate one or
/// reuse an existing prefix for this import.
+ ///
+ /// 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? showName,
bool forceAbsolute = false,
bool forceRelative = false,
}) {
@@ -1956,12 +1984,19 @@
if (prefix != null) {
import.prefixes.add(prefix);
}
+ if (showName != null) {
+ import.ensureShown(showName);
+ }
} else {
var uriText = _getLibraryUriText(uri,
forceAbsolute: forceAbsolute, forceRelative: forceRelative);
prefix ??=
importPrefixGenerator != null ? importPrefixGenerator!(uri) : null;
- import = _LibraryImport(uriText: uriText, prefix: prefix ?? '');
+ import = _LibraryImport(
+ uriText: uriText,
+ prefix: prefix ?? '',
+ showNames: showName != null ? {showName} : null,
+ );
(libraryChangeBuilder ?? this).librariesToImport[uri] = import;
}
@@ -2077,7 +2112,19 @@
/// with other prefixes for a library that is both prefixed and unprefixed.
final Set<String> prefixes = {};
- _LibraryImport({required this.uriText, required String prefix}) {
+ /// Names this import has in its `show` combinator.
+ final Set<String> showNames;
+
+ /// Names this import has in its `hide` combinator.
+ final Set<String> hideNames;
+
+ _LibraryImport({
+ required this.uriText,
+ required String prefix,
+ Set<String>? showNames,
+ Set<String>? hideNames,
+ }) : showNames = showNames ?? {},
+ hideNames = hideNames ?? {} {
prefixes.add(prefix);
}
@@ -2095,4 +2142,15 @@
other.uriText == uriText &&
!const SetEquality().equals(other.prefixes, prefixes);
}
+
+ /// Ensures [name] is visible for this import.
+ ///
+ /// If the import already has a show combinator, this name will be added.
+ /// If the import hides this name, it will be unhidden.
+ void ensureShown(String name) {
+ if (showNames.isNotEmpty) {
+ showNames.add(name);
+ }
+ hideNames.remove(name);
+ }
}
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 3d5694d..4c97c44 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
@@ -355,12 +355,14 @@
/// Arrange to have an import added for the library with the given [uri]. If a
/// [prefix] is provided it will be used in the import directive.
///
+ /// If [showName] is provided, new imports will show only this name.
+ ///
/// Return the text of the URI that will be used in the import directive.
/// It can be different than the given [uri].
///
/// The [uri] may be converted from an absolute URI to a relative URI
/// depending on user preferences/lints.
- String importLibrary(Uri uri, {String? prefix});
+ String importLibrary(Uri uri, {String? prefix, String? showName});
/// Ensure that the library with the given [uri] is imported.
///
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 bc3ccd0..715aa54 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
@@ -2122,6 +2122,152 @@
expect(cache[futureOrElement], resolvedFakeUnit.libraryElement);
}
+ /// Test that importing elements where the library is already imported
+ /// (without a show/hide cmobinator) produces no additional edits.
+ Future<void> test_importElementLibrary_useShow_existingImport() async {
+ var content = '''
+import 'package:test/a.dart';
+''';
+ var otherContent = '''
+class A {}
+class B {}
+class C {}
+''';
+ var otherUnit = await resolveContent('/home/test/lib/a.dart', otherContent);
+ var unit = await resolveContent('/home/test/lib/test.dart', content);
+
+ final edits = await _getClassImportEdits(
+ destinationUnit: unit,
+ importedUnit: otherUnit,
+ classNames: {'A', 'B'},
+ useShow: true,
+ );
+ expect(edits, isEmpty);
+ }
+
+ /// Test that importing elements where the library is already imported but
+ /// hides the element causes the element to be shown.
+ Future<void>
+ test_importElementLibrary_useShow_existingImport_hidesElement() async {
+ var content = '''
+import 'package:test/a.dart' hide A;
+''';
+ var otherContent = '''
+class A {}
+class B {}
+class C {}
+''';
+ var otherUnit = await resolveContent('/home/test/lib/a.dart', otherContent);
+ var unit = await resolveContent('/home/test/lib/test.dart', content);
+
+ final edits = await _getClassImportEdits(
+ destinationUnit: unit,
+ importedUnit: otherUnit,
+ classNames: {'A', 'B'},
+ useShow: true,
+ );
+ final resultContent = SourceEdit.applySequence(content, edits);
+ // TODO(dantup): Currently we can only add new imports and not update
+ // existing ones.
+ expect(
+ resultContent,
+ equals(
+ '''
+import 'package:test/a.dart' hide A;
+import 'package:test/a.dart' show A;
+''',
+ ));
+ }
+
+ /// Test that importing elements where the library is already imported and
+ /// already shows the elements produces no edits.
+ Future<void>
+ test_importElementLibrary_useShow_existingImport_showsElement() async {
+ var content = '''
+import 'package:test/a.dart' show A, B, C;
+''';
+ var otherContent = '''
+class A {}
+class B {}
+class C {}
+''';
+ var otherUnit = await resolveContent('/home/test/lib/a.dart', otherContent);
+ var unit = await resolveContent('/home/test/lib/test.dart', content);
+
+ final edits = await _getClassImportEdits(
+ destinationUnit: unit,
+ importedUnit: otherUnit,
+ classNames: {'A', 'B'},
+ useShow: true,
+ );
+ expect(edits, isEmpty);
+ }
+
+ /// Test that importing elements where the library is already imported but it
+ /// only shows other elements results in the element being shown.
+ Future<void>
+ test_importElementLibrary_useShow_existingImport_showsOthers() async {
+ var content = '''
+import 'package:test/a.dart' show C;
+''';
+ var otherContent = '''
+class A {}
+class B {}
+class C {}
+''';
+ var otherUnit = await resolveContent('/home/test/lib/a.dart', otherContent);
+ var unit = await resolveContent('/home/test/lib/test.dart', content);
+
+ final edits = await _getClassImportEdits(
+ destinationUnit: unit,
+ importedUnit: otherUnit,
+ classNames: {'A', 'B'},
+ useShow: true,
+ );
+ final resultContent = SourceEdit.applySequence(content, edits);
+ // TODO(dantup): Currently we can only add new imports and not update
+ // existing ones (such as to add to 'show' or remove 'hide'). Updating
+ // existing imports requires some changes to how prefixes are currently
+ // handles so we can separate prefixes from the shown elements.
+ expect(
+ resultContent,
+ equals(
+ '''
+import 'package:test/a.dart' show C;
+import 'package:test/a.dart' show A, B;
+''',
+ ));
+ }
+
+ /// Test that that we can add a new import elements using 'show'.
+ Future<void> test_importElementLibrary_useShow_newImport() async {
+ var otherUnit = await resolveContent(
+ '/home/test/lib/a.dart',
+ '''
+class A {}
+class B {}
+''',
+ );
+ var unit = await resolveContent(
+ '/home/test/lib/test.dart',
+ '',
+ );
+
+ final edits = await _getClassImportEdits(
+ destinationUnit: unit,
+ importedUnit: otherUnit,
+ classNames: {'A', 'B'},
+ useShow: true,
+ );
+ expect(edits, hasLength(1));
+ expect(
+ edits[0].replacement,
+ equals(
+ '''import 'package:test/a.dart' show A, B;
+''',
+ ));
+ }
+
Future<void> test_multipleEdits_concurrently() async {
var initialCode = '00';
var path = convertPath('/home/test/lib/test.dart');
@@ -2174,6 +2320,30 @@
expect(edits, hasLength(1));
expect(edits[0].replacement, equalsIgnoringWhitespace('Future<String>'));
}
+
+ /// Computes edits to import the class [className] from [importedUnit] into
+ /// [destinationUnit].
+ ///
+ /// If [useShow] is set, will try to add a show combinator for this class.
+ Future<List<SourceEdit>> _getClassImportEdits({
+ required ResolvedUnitResult destinationUnit,
+ required ResolvedUnitResult importedUnit,
+ required Set<String> classNames,
+ bool useShow = false,
+ }) async {
+ var findNode = FindNode(importedUnit.content, importedUnit.unit);
+ var builder = await newBuilder();
+ await builder.addDartFileEdit(destinationUnit.path, (builder) async {
+ var builderImpl = builder as DartFileEditBuilderImpl;
+ for (var className in classNames) {
+ var classElement =
+ findNode.classDeclaration(className).declaredElement!;
+ await builderImpl.importElementLibrary(classElement, useShow: useShow);
+ }
+ });
+
+ return getEdits(builder);
+ }
}
@reflectiveTest
diff --git a/pkg/analyzer_plugin/test/src/utilities/change_builder/dart/dart_change_builder_mixin.dart b/pkg/analyzer_plugin/test/src/utilities/change_builder/dart/dart_change_builder_mixin.dart
index 10af9f1..a6b4a7b 100644
--- a/pkg/analyzer_plugin/test/src/utilities/change_builder/dart/dart_change_builder_mixin.dart
+++ b/pkg/analyzer_plugin/test/src/utilities/change_builder/dart/dart_change_builder_mixin.dart
@@ -20,11 +20,12 @@
expect(sourceChange, isNotNull);
var fileEdits = sourceChange.edits;
- expect(fileEdits, hasLength(1));
+ if (fileEdits.isEmpty) {
+ return [];
+ }
- var fileEdit = fileEdits[0];
- expect(fileEdit, isNotNull);
- return fileEdit.edits;
+ expect(fileEdits, hasLength(1));
+ return fileEdits[0].edits;
}
/// Return a newly created Dart change builder.
