[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>

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

@@ -60,11 +60,12 @@ class CompletionResolveHandler
           '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 @@ class CompletionResolveHandler
     _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 @@ class CompletionResolveHandler
 
         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 @@ class CompletionResolveHandler
 
         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.

pkg/analysis_server/lib/src/services/correction/dart/import_library.dart

@@ -16,6 +16,7 @@ import 'package:analyzer/dart/element/element.dart';
 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 @@ class ImportLibrary extends MultiCorrectionProducer {
     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 @@ class ImportLibrary extends MultiCorrectionProducer {
 
   /// 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 @@ class ImportLibrary extends MultiCorrectionProducer {
         // 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 @@ class _ImportAbsoluteLibrary extends CorrectionProducer {
   @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 _ImportLibraryShow extends CorrectionProducer {
 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 @@ class _ImportRelativeLibrary extends CorrectionProducer {
   Future<void> compute(ChangeBuilder builder) async {
     await builder.addDartFileEdit(file, (builder) {
       if (builder is DartFileEditBuilderImpl) {
-        builder.importLibraryWithRelativeUri(_relativeURI);
+        _uriText = builder.importLibraryWithRelativeUri(_library);
       }
     });
   }

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 @@ void f() {
     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 @@ void f() {
     '''));
   }
 
+  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 @@ void f() {
     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 @@ void f() {
       expect(res, expectedResults);
     }
   }
+
+  void _enableLints(List<String> lintNames) {
+    registerLintRules();
+    final lintsYaml = lintNames.map((name) => '    - $name\n').join();
+    newFile(analysisOptionsPath, content: '''
+linter:
+  rules:
+$lintsYaml
+''');
+  }
 }
 
 @reflectiveTest

pkg/analysis_server/test/lsp/diagnostic_test.dart

@@ -88,7 +88,7 @@ String b = "Test";
 linter:
   rules:
     - invalid_lint_rule_name
-''').path;
+''');
 
     final firstDiagnosticsUpdate = waitForDiagnostics(analysisOptionsUri);
     await initialize();
@@ -102,7 +102,7 @@ linter:
   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);

pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart

@@ -17,6 +17,7 @@ import 'package:analyzer_plugin/protocol/protocol_common.dart'
     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 @@ class DartFileEditBuilderImpl extends FileEditBuilderImpl
   /// 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 @@ class DartFileEditBuilderImpl extends FileEditBuilderImpl
       : 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 @@ class DartFileEditBuilderImpl extends FileEditBuilderImpl
     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 @@ class DartFileEditBuilderImpl extends FileEditBuilderImpl
     if (librariesToImport.isNotEmpty) {
       _addLibraryImports(librariesToImport.values);
     }
-    if (librariesToRelativelyImport.isNotEmpty) {
-      _addLibraryImports(librariesToRelativelyImport.values);
-    }
   }
 
   @override
@@ -1480,8 +1467,12 @@ class DartFileEditBuilderImpl extends FileEditBuilderImpl
     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 @@ class DartFileEditBuilderImpl extends FileEditBuilderImpl
   }
 
   /// 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 @@ class DartFileEditBuilderImpl extends FileEditBuilderImpl
     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`

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;
+}

pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart

@@ -346,6 +346,9 @@ abstract class DartFileEditBuilder implements FileEditBuilder {
   ///
   /// 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.