[analysis_server] Use relative imports for completion imports when prefer_relative_imports lint is enabled
Change-Id: Ib72066102db1883a21cf7a42e18a81efcfb47c54
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/220200
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: 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 30935d6..72903a8 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
@@ -60,11 +60,12 @@
'Requests not before server is initilized');
}
- final lineInfo = server.getLineInfo(data.file);
+ final file = data.file;
+ final lineInfo = server.getLineInfo(file);
if (lineInfo == null) {
return error(
ErrorCodes.InternalError,
- 'Line info not available for ${data.file}',
+ 'Line info not available for $file',
null,
);
}
@@ -99,7 +100,7 @@
_latestCompletionItem = item;
while (item == _latestCompletionItem && timer.elapsed < timeout) {
try {
- final analysisDriver = server.getAnalysisDriver(data.file);
+ final analysisDriver = server.getAnalysisDriver(file);
final session = analysisDriver?.currentSession;
// We shouldn't not get a driver/session, but if we did perhaps the file
@@ -139,7 +140,7 @@
var newInsertText = item.insertText ?? item.label;
final builder = ChangeBuilder(session: session);
- await builder.addDartFileEdit(data.file, (builder) {
+ await builder.addDartFileEdit(file, (builder) {
final result = builder.importLibraryElement(library.uri);
if (result.prefix != null) {
newInsertText = '${result.prefix}.$newInsertText';
@@ -152,9 +153,9 @@
final changes = builder.sourceChange;
final thisFilesChanges =
- changes.edits.where((e) => e.file == data.file).toList();
+ changes.edits.where((e) => e.file == file).toList();
final otherFilesChanges =
- changes.edits.where((e) => e.file != data.file).toList();
+ changes.edits.where((e) => e.file != file).toList();
// If this completion involves editing other files, we'll need to build
// a command that the client will call to apply those edits later.
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 415f155..4a949c4 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
@@ -16,6 +16,7 @@
import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/source/source_range.dart';
import 'package:analyzer_plugin/src/utilities/change_builder/change_builder_dart.dart';
+import 'package:analyzer_plugin/src/utilities/library.dart';
import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
import 'package:analyzer_plugin/utilities/range_factory.dart';
@@ -150,22 +151,6 @@
return false;
}
- /// Returns the relative URI from the passed [library] to the given [path].
- ///
- /// If the [path] is not in the [library]'s directory, `null` is returned.
- String? _getRelativeUriFromLibrary(LibraryElement library, String path) {
- var librarySource = library.librarySource;
- var pathContext = resourceProvider.pathContext;
- var libraryDirectory = pathContext.dirname(librarySource.fullName);
- var sourceDirectory = pathContext.dirname(path);
- if (pathContext.isWithin(libraryDirectory, path) ||
- pathContext.isWithin(sourceDirectory, libraryDirectory)) {
- var relativeFile = pathContext.relative(path, from: libraryDirectory);
- return pathContext.split(relativeFile).join('/');
- }
- return null;
- }
-
Iterable<CorrectionProducer> _importExtensionInLibrary(
Uri uri, DartType targetType, String memberName) sync* {
// Look to see whether the library at the [uri] is already imported. If it
@@ -207,25 +192,28 @@
/// Returns a list of one or two import corrections.
///
- /// If [relativeUri] is `null`, only one correction, with an absolute import
- /// path, is returned. Otherwise, a correction with an absolute import path
- /// and a correction with a relative path are returned. If the
+ /// If [includeRelativeFix] is `false`, only one correction, with an absolute
+ /// import path, is returned. Otherwise, a correction with an absolute import
+ /// path and a correction with a relative path are returned. If the
/// `prefer_relative_imports` lint rule is enabled, the relative path is
/// returned first.
- Iterable<CorrectionProducer> _importLibrary(FixKind fixKind, Uri library,
- [String? relativeUri]) {
- if (relativeUri == null || relativeUri.isEmpty) {
+ Iterable<CorrectionProducer> _importLibrary(
+ FixKind fixKind,
+ Uri library, {
+ bool includeRelativeFix = false,
+ }) {
+ if (!includeRelativeFix) {
return [_ImportAbsoluteLibrary(fixKind, library)];
}
if (isLintEnabled(LintNames.prefer_relative_imports)) {
return [
- _ImportRelativeLibrary(fixKind, relativeUri),
+ _ImportRelativeLibrary(fixKind, library),
_ImportAbsoluteLibrary(fixKind, library),
];
} else {
return [
_ImportAbsoluteLibrary(fixKind, library),
- _ImportRelativeLibrary(fixKind, relativeUri),
+ _ImportRelativeLibrary(fixKind, library),
];
}
}
@@ -312,10 +300,13 @@
// Good: direct declaration.
fixKind = DartFixKind.IMPORT_LIBRARY_PROJECT1;
}
- // Add the fix.
- var relativeUri =
- _getRelativeUriFromLibrary(libraryElement, declaration.path);
- yield* _importLibrary(fixKind, declaration.uri, relativeUri);
+ // If both files are in the same package's lib folder, also include a
+ // relative import.
+ var includeRelativeUri = canBeRelativeImport(
+ declaration.uri, libraryElement.librarySource.uri);
+ // Add the fix(es).
+ yield* _importLibrary(fixKind, declaration.uri,
+ includeRelativeFix: includeRelativeUri);
}
}
@@ -415,7 +406,9 @@
@override
Future<void> compute(ChangeBuilder builder) async {
await builder.addDartFileEdit(file, (builder) {
- _uriText = builder.importLibrary(_library);
+ if (builder is DartFileEditBuilderImpl) {
+ _uriText = builder.importLibraryWithAbsoluteUri(_library);
+ }
});
}
}
@@ -532,12 +525,14 @@
class _ImportRelativeLibrary extends CorrectionProducer {
final FixKind _fixKind;
- final String _relativeURI;
+ final Uri _library;
- _ImportRelativeLibrary(this._fixKind, this._relativeURI);
+ String _uriText = '';
+
+ _ImportRelativeLibrary(this._fixKind, this._library);
@override
- List<Object> get fixArguments => [_relativeURI];
+ List<Object> get fixArguments => [_uriText];
@override
FixKind get fixKind => _fixKind;
@@ -546,7 +541,7 @@
Future<void> compute(ChangeBuilder builder) async {
await builder.addDartFileEdit(file, (builder) {
if (builder is DartFileEditBuilderImpl) {
- builder.importLibraryWithRelativeUri(_relativeURI);
+ _uriText = builder.importLibraryWithRelativeUri(_library);
}
});
}
diff --git a/pkg/analysis_server/test/lsp/completion_dart_test.dart b/pkg/analysis_server/test/lsp/completion_dart_test.dart
index c2e57dd..cbacc55 100644
--- a/pkg/analysis_server/test/lsp/completion_dart_test.dart
+++ b/pkg/analysis_server/test/lsp/completion_dart_test.dart
@@ -4,9 +4,11 @@
import 'package:analysis_server/lsp_protocol/protocol_generated.dart';
import 'package:analysis_server/src/lsp/constants.dart';
+import 'package:analysis_server/src/services/linter/lint_names.dart';
import 'package:analyzer_plugin/protocol/protocol_common.dart' as plugin;
import 'package:analyzer_plugin/protocol/protocol_generated.dart' as plugin;
import 'package:collection/collection.dart';
+import 'package:linter/src/rules.dart';
import 'package:test/test.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';
@@ -1621,6 +1623,36 @@
expect(resolved.detail, isNull);
}
+ Future<void> test_suggestionSets_importsPackageUri() async {
+ newFile(
+ join(projectFolderPath, 'lib', 'my_class.dart'),
+ content: 'class MyClass {}',
+ );
+
+ final content = '''
+void f() {
+ MyClas^
+}
+ ''';
+
+ final expectedContent = '''
+import 'package:test/my_class.dart';
+
+void f() {
+ MyClass
+}
+ ''';
+
+ final completionLabel = 'MyClass';
+
+ await _checkCompletionEdits(
+ mainFileUri,
+ content,
+ completionLabel,
+ expectedContent,
+ );
+ }
+
Future<void>
test_suggestionSets_includesReexportedSymbolsForEachFile() async {
newFile(
@@ -2018,6 +2050,78 @@
'''));
}
+ Future<void> test_suggestionSets_preferRelativeImportsLib_insideLib() async {
+ _enableLints([LintNames.prefer_relative_imports]);
+ final importingFilePath =
+ join(projectFolderPath, 'lib', 'nested1', 'main.dart');
+ final importingFileUri = Uri.file(importingFilePath);
+ final importedFilePath =
+ join(projectFolderPath, 'lib', 'nested2', 'imported.dart');
+
+ // Create a file that will be auto-imported from completion.
+ newFile(importedFilePath, content: 'class MyClass {}');
+
+ final content = '''
+void f() {
+ MyClas^
+}
+ ''';
+
+ final expectedContent = '''
+import '../nested2/imported.dart';
+
+void f() {
+ MyClass
+}
+ ''';
+
+ final completionLabel = 'MyClass';
+
+ await _checkCompletionEdits(
+ importingFileUri,
+ content,
+ completionLabel,
+ expectedContent,
+ );
+ }
+
+ Future<void> test_suggestionSets_preferRelativeImportsLib_outsideLib() async {
+ // Files outside of the lib folder should still get absolute imports to
+ // files inside lib, even with the lint enabled.
+ _enableLints([LintNames.prefer_relative_imports]);
+ final importingFilePath =
+ join(projectFolderPath, 'bin', 'nested1', 'main.dart');
+ final importingFileUri = Uri.file(importingFilePath);
+ final importedFilePath =
+ join(projectFolderPath, 'lib', 'nested2', 'imported.dart');
+
+ // Create a file that will be auto-imported from completion.
+ newFile(importedFilePath, content: 'class MyClass {}');
+
+ final content = '''
+void f() {
+ MyClas^
+}
+ ''';
+
+ final expectedContent = '''
+import 'package:test/nested2/imported.dart';
+
+void f() {
+ MyClass
+}
+ ''';
+
+ final completionLabel = 'MyClass';
+
+ await _checkCompletionEdits(
+ importingFileUri,
+ content,
+ completionLabel,
+ expectedContent,
+ );
+ }
+
Future<void> test_suggestionSets_unavailableIfDisabled() async {
newFile(
join(projectFolderPath, 'other_file.dart'),
@@ -2100,6 +2204,39 @@
expect(updated, contains('a.abcdefghij'));
}
+ /// Sets up the server with a file containing [content] and checks that
+ /// accepting a specific completion produces [expectedContent].
+ ///
+ /// [content] should contain a `^` at the location where completion should be
+ /// invoked/accepted.
+ Future<void> _checkCompletionEdits(
+ Uri fileUri,
+ String content,
+ String completionLabel,
+ String expectedContent,
+ ) async {
+ final initialAnalysis = waitForAnalysisComplete();
+ await initialize(
+ workspaceCapabilities:
+ withApplyEditSupport(emptyWorkspaceClientCapabilities));
+ await openFile(fileUri, withoutMarkers(content));
+ await initialAnalysis;
+ final res = await getCompletion(fileUri, positionFromMarker(content));
+
+ final completion = res.where((c) => c.label == completionLabel).single;
+ final resolvedCompletion = await resolveCompletion(completion);
+
+ // Apply both the main completion edit and the additionalTextEdits atomically.
+ final newContent = applyTextEdits(
+ withoutMarkers(content),
+ [toTextEdit(resolvedCompletion.textEdit!)]
+ .followedBy(resolvedCompletion.additionalTextEdits!)
+ .toList(),
+ );
+
+ expect(newContent, equals(expectedContent));
+ }
+
Future<void> _checkResultsForTriggerCharacters(String content,
List<String> triggerCharacters, Matcher expectedResults) async {
await initialize();
@@ -2114,6 +2251,16 @@
expect(res, expectedResults);
}
}
+
+ void _enableLints(List<String> lintNames) {
+ registerLintRules();
+ final lintsYaml = lintNames.map((name) => ' - $name\n').join();
+ newFile(analysisOptionsPath, content: '''
+linter:
+ rules:
+$lintsYaml
+''');
+ }
}
@reflectiveTest
diff --git a/pkg/analysis_server/test/lsp/diagnostic_test.dart b/pkg/analysis_server/test/lsp/diagnostic_test.dart
index fc6dffe..3b7fc18 100644
--- a/pkg/analysis_server/test/lsp/diagnostic_test.dart
+++ b/pkg/analysis_server/test/lsp/diagnostic_test.dart
@@ -88,7 +88,7 @@
linter:
rules:
- invalid_lint_rule_name
-''').path;
+''');
final firstDiagnosticsUpdate = waitForDiagnostics(analysisOptionsUri);
await initialize();
@@ -102,7 +102,7 @@
Future<void> test_analysisOptionsFile_packageInclude() async {
newFile(analysisOptionsPath, content: '''
include: package:pedantic/analysis_options.yaml
-''').path;
+''');
// Verify there's an error for the import.
final firstDiagnosticsUpdate = waitForDiagnostics(analysisOptionsUri);
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 9b0c895..104fc79 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
@@ -17,6 +17,7 @@
hide Element, ElementKind;
import 'package:analyzer_plugin/src/utilities/change_builder/change_builder_core.dart';
import 'package:analyzer_plugin/src/utilities/charcodes.dart';
+import 'package:analyzer_plugin/src/utilities/library.dart';
import 'package:analyzer_plugin/src/utilities/string_utilities.dart';
import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
import 'package:analyzer_plugin/utilities/change_builder/change_builder_dart.dart';
@@ -1337,11 +1338,6 @@
/// the names used in generated code, to information about these imports.
Map<Uri, _LibraryToImport> librariesToImport = {};
- /// A mapping from libraries that need to be imported relatively in order to
- /// make visible the names used in generated code, to information about these
- /// imports.
- Map<String, _LibraryToImport> librariesToRelativelyImport = {};
-
/// Initialize a newly created builder to build a source file edit within the
/// change being built by the given [changeBuilder]. The file being edited has
/// the given [resolvedUnit] and [timeStamp].
@@ -1350,10 +1346,7 @@
: super(changeBuilder, resolvedUnit.path, timeStamp);
@override
- bool get hasEdits =>
- super.hasEdits ||
- librariesToImport.isNotEmpty ||
- librariesToRelativelyImport.isNotEmpty;
+ bool get hasEdits => super.hasEdits || librariesToImport.isNotEmpty;
@override
void addInsertion(
@@ -1402,9 +1395,6 @@
for (var entry in librariesToImport.entries) {
copy.librariesToImport[entry.key] = entry.value;
}
- for (var entry in librariesToRelativelyImport.entries) {
- copy.librariesToRelativelyImport[entry.key] = entry.value;
- }
return copy;
}
@@ -1418,9 +1408,6 @@
if (librariesToImport.isNotEmpty) {
_addLibraryImports(librariesToImport.values);
}
- if (librariesToRelativelyImport.isNotEmpty) {
- _addLibraryImports(librariesToRelativelyImport.values);
- }
}
@override
@@ -1480,8 +1467,12 @@
return ImportLibraryElementResultImpl(null);
}
- String importLibraryWithRelativeUri(String uriText, [String? prefix]) {
- return _importLibraryWithRelativeUri(uriText, prefix).uriText;
+ String importLibraryWithAbsoluteUri(Uri uri, [String? prefix]) {
+ return _importLibrary(uri, prefix: prefix, forceAbsolute: true).uriText;
+ }
+
+ String importLibraryWithRelativeUri(Uri uri, [String? prefix]) {
+ return _importLibrary(uri, prefix: prefix, forceRelative: true).uriText;
}
@override
@@ -1720,24 +1711,55 @@
}
/// Computes the best URI to import [uri] into the target library.
- String _getLibraryUriText(Uri uri) {
- if (uri.scheme == 'file') {
- var pathContext = resolvedUnit.session.resourceProvider.pathContext;
- var whatPath = pathContext.fromUri(uri);
+ ///
+ /// [uri] may be converted from an absolute URI to a relative URI depending on
+ /// user preferences/lints unless [forceAbsolute] or [forceRelative] are `true`.
+ String _getLibraryUriText(
+ Uri uri, {
+ bool forceAbsolute = false,
+ bool forceRelative = false,
+ }) {
+ var pathContext = resolvedUnit.session.resourceProvider.pathContext;
+
+ /// Returns the relative path to import [whatPath] into [resolvedUnit].
+ String getRelativePath(String whatPath) {
var libraryPath = resolvedUnit.libraryElement.source.fullName;
var libraryFolder = pathContext.dirname(libraryPath);
var relativeFile = pathContext.relative(whatPath, from: libraryFolder);
return pathContext.split(relativeFile).join('/');
}
+
+ if (uri.isScheme('file')) {
+ var whatPath = pathContext.fromUri(uri);
+ return getRelativePath(whatPath);
+ }
+ var preferRelative = _isLintEnabled('prefer_relative_imports');
+ if (forceRelative || (preferRelative && !forceAbsolute)) {
+ if (canBeRelativeImport(uri, resolvedUnit.uri)) {
+ var whatPath = resolvedUnit.session.uriConverter.uriToPath(uri);
+ if (whatPath != null) {
+ return getRelativePath(whatPath);
+ }
+ }
+ }
return uri.toString();
}
/// Arrange to have an import added for the library with the given [uri].
- _LibraryToImport _importLibrary(Uri uri) {
+ ///
+ /// [uri] may be converted from an absolute URI to a relative URI depending on
+ /// user preferences/lints unless [forceAbsolute] or [forceRelative] are `true`.
+ _LibraryToImport _importLibrary(
+ Uri uri, {
+ String? prefix,
+ bool forceAbsolute = false,
+ bool forceRelative = false,
+ }) {
var import = (libraryChangeBuilder ?? this).librariesToImport[uri];
if (import == null) {
- var uriText = _getLibraryUriText(uri);
- var prefix =
+ var uriText = _getLibraryUriText(uri,
+ forceAbsolute: forceAbsolute, forceRelative: forceRelative);
+ prefix ??=
importPrefixGenerator != null ? importPrefixGenerator!(uri) : null;
import = _LibraryToImport(uriText, prefix);
(libraryChangeBuilder ?? this).librariesToImport[uri] = import;
@@ -1745,23 +1767,17 @@
return import;
}
- /// Arrange to have an import added for the library with the given relative
- /// [uriText].
- _LibraryToImport _importLibraryWithRelativeUri(String uriText,
- [String? prefix]) {
- var import = librariesToRelativelyImport[uriText];
- if (import == null) {
- import = _LibraryToImport(uriText, prefix);
- librariesToRelativelyImport[uriText] = import;
- }
- return import;
- }
-
/// Return `true` if the [element] is defined in the target library.
bool _isDefinedLocally(Element element) {
return element.library == resolvedUnit.libraryElement;
}
+ bool _isLintEnabled(String lintName) {
+ final analysisOptions =
+ resolvedUnit.session.analysisContext.analysisOptions;
+ return analysisOptions.isLintEnabled(lintName);
+ }
+
/// Create an edit to replace the return type of the innermost function
/// containing the given [node] with the type `Future`. The [typeProvider] is
/// used to check the current return type, because if it is already `Future`
diff --git a/pkg/analyzer_plugin/lib/src/utilities/library.dart b/pkg/analyzer_plugin/lib/src/utilities/library.dart
new file mode 100644
index 0000000..8fa8383
--- /dev/null
+++ b/pkg/analyzer_plugin/lib/src/utilities/library.dart
@@ -0,0 +1,16 @@
+// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+/// Checks whether importing the library with URI [library2] into the
+/// library with URI [library1] could be a relative import.
+///
+/// Both URIs must be package: URIs and belong to the same package for this to
+/// be true.
+bool canBeRelativeImport(Uri library1, Uri library2) {
+ return library1.isScheme('package') &&
+ library2.isScheme('package') &&
+ library1.pathSegments.isNotEmpty &&
+ library2.pathSegments.isNotEmpty &&
+ library1.pathSegments.first == library2.pathSegments.first;
+}
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 6bd6c78..4eababe 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
@@ -346,6 +346,9 @@
///
/// Returns the text of the URI that will be used in the import directive.
/// It can be different than the given [Uri].
+ ///
+ /// [uri] may be converted from an absolute URI to a relative URI depending on
+ /// user preferences/lints.
String importLibrary(Uri uri);
/// Ensure that the library with the given [uri] is imported.
