[anaysis_server] Preserve ordering of fixes with the same priority in LSP server

See https://github.com/Dart-Code/Dart-Code/issues/4522.

Change-Id: I6bd10af40c77cd10c7897ae4d4ce3bf8e8988454
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/302620
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

pkg/analysis_server/lib/src/lsp/handlers/code_actions/abstract_code_actions_producer.dart

@@ -17,6 +17,14 @@ import 'package:analyzer/source/line_info.dart';
 import 'package:analyzer/src/dart/analysis/results.dart' as engine;
 import 'package:meta/meta.dart';
 
+typedef CodeActionWithPriority = ({CodeAction action, int priority});
+
+typedef CodeActionWithPriorityAndIndex = ({
+  CodeAction action,
+  int priority,
+  int index
+});
+
 /// A base for classes that produce [CodeAction]s for the LSP handler.
 abstract class AbstractCodeActionsProducer
     with RequestHandlerMixin<LspAnalysisServer> {
@@ -127,12 +135,3 @@ abstract class AbstractCodeActionsProducer
     }
   }
 }
-
-/// A wrapper that contains an LSP [CodeAction] and a server-supplied priority
-/// used for sorting before sending to the client.
-class CodeActionWithPriority {
-  final CodeAction action;
-  final int priority;
-
-  CodeActionWithPriority(this.action, this.priority);
-}

pkg/analysis_server/lib/src/lsp/handlers/code_actions/analysis_options.dart

@@ -79,7 +79,7 @@ class AnalysisOptionsCodeActionsProducer extends AbstractCodeActionsProducer {
         fixes.map((fix) {
           final action =
               createFixAction(fix.change, diagnostic, path, lineInfo);
-          return CodeActionWithPriority(action, fix.kind.priority);
+          return (action: action, priority: fix.kind.priority);
         }),
       );
     }

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

@@ -120,7 +120,7 @@ class DartCodeActionsProducer extends AbstractCodeActionsProducer {
       return assists.map((assist) {
         final action =
             createAssistAction(assist.change, unit.path, unit.lineInfo);
-        return CodeActionWithPriority(action, assist.kind.priority);
+        return (action: action, priority: assist.kind.priority);
       }).toList();
     } on InconsistentAnalysisException {
       // If an InconsistentAnalysisException occurs, it's likely the user modified
@@ -169,7 +169,7 @@ class DartCodeActionsProducer extends AbstractCodeActionsProducer {
             fixes.map((fix) {
               final action =
                   createFixAction(fix.change, diagnostic, path, lineInfo);
-              return CodeActionWithPriority(action, fix.kind.priority);
+              return (action: action, priority: fix.kind.priority);
             }),
           );
         }

pkg/analysis_server/lib/src/lsp/handlers/code_actions/plugins.dart

@@ -70,12 +70,12 @@ class PluginCodeActionsProducer extends AbstractCodeActionsProducer {
   @override
   Future<List<Either2<CodeAction, Command>>> getSourceActions() async => [];
 
-  CodeActionWithPriority _convertAssist(
-          plugin.PrioritizedSourceChange assist) =>
-      CodeActionWithPriority(
-        createAssistAction(assist.change, path, lineInfo),
-        assist.priority,
-      );
+  CodeActionWithPriority _convertAssist(plugin.PrioritizedSourceChange assist) {
+    return (
+      action: createAssistAction(assist.change, path, lineInfo),
+      priority: assist.priority,
+    );
+  }
 
   Iterable<CodeActionWithPriority> _convertFixes(
       plugin.AnalysisErrorFixes fixes) {
@@ -86,9 +86,9 @@ class PluginCodeActionsProducer extends AbstractCodeActionsProducer {
       clientSupportsCodeDescription: supportsCodeDescription,
     );
     return fixes.fixes.map(
-      (fix) => CodeActionWithPriority(
-        createFixAction(fix.change, diagnostic, path, lineInfo),
-        fix.priority,
+      (fix) => (
+        action: createFixAction(fix.change, diagnostic, path, lineInfo),
+        priority: fix.priority,
       ),
     );
   }

pkg/analysis_server/lib/src/lsp/handlers/code_actions/pubspec.dart

@@ -74,7 +74,7 @@ class PubspecCodeActionsProducer extends AbstractCodeActionsProducer {
         fixes.map((fix) {
           final action =
               createFixAction(fix.change, diagnostic, path, lineInfo);
-          return CodeActionWithPriority(action, fix.kind.priority);
+          return (action: action, priority: fix.kind.priority);
         }),
       );
     }

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

@@ -230,10 +230,16 @@ class _CodeActionSorter {
 
   List<Either2<CodeAction, Command>> sort(
       List<CodeActionWithPriority> actions) {
-    final dedupedCodeActions = _dedupeActions(actions, range.start);
-    dedupedCodeActions.sort(_compareCodeActions);
+    final dedupedActions = _dedupeActions(actions, range.start);
 
-    return dedupedCodeActions
+    // Add each index so we can do a stable sort on priority.
+    final dedupedActionsWithIndex = dedupedActions.indexed.map((item) {
+      final (index, action) = item;
+      return (action: action.action, priority: action.priority, index: index);
+    }).toList();
+    dedupedActionsWithIndex.sort(_compareCodeActions);
+
+    return dedupedActionsWithIndex
         .where((action) => shouldIncludeKind(action.action.kind))
         .map((action) => Either2<CodeAction, Command>.t1(action.action))
         .toList();
@@ -257,14 +263,25 @@ class _CodeActionSorter {
   int _columnDistance(Position a, Position b) =>
       (a.character - b.character).abs();
 
-  /// A function that can be used to sort [CodeActionWithPriority]s.
+  /// A function that can be used to sort [CodeActionWithPriorityAndIndex]es.
   ///
   /// The highest number priority will be sorted before lower number priorities.
-  /// Items with the same priority are sorted alphabetically by their title.
-  int _compareCodeActions(CodeActionWithPriority a, CodeActionWithPriority b) {
+  /// Items with the same priority are sorted by their index (ascending).
+  int _compareCodeActions(
+    CodeActionWithPriorityAndIndex a,
+    CodeActionWithPriorityAndIndex b,
+  ) {
+    // Priority, descending.
     if (a.priority != b.priority) {
       return b.priority - a.priority;
     }
+    // Index, ascending.
+    assert(a.index != b.index);
+    if (a.index != b.index) {
+      return a.index - b.index;
+    }
+    // We should never have the same index, but just in case - ensure the sort
+    // is stable.
     return a.action.title.compareTo(b.action.title);
   }
 
@@ -311,8 +328,8 @@ class _CodeActionSorter {
 
       // Build a new CodeAction that merges the diagnostics from each same
       // code action onto a single one.
-      return CodeActionWithPriority(
-        CodeAction(
+      return (
+        action: CodeAction(
           title: first.title,
           kind: first.kind,
           // Merge diagnostics from all of the matching CodeActions.
@@ -323,7 +340,7 @@ class _CodeActionSorter {
           edit: first.edit,
           command: first.command,
         ),
-        priority,
+        priority: priority,
       );
     }).toList();
   }

pkg/analysis_server/test/lsp/code_actions_fixes_test.dart

@@ -98,6 +98,101 @@ class FixesCodeActionsTest extends AbstractCodeActionsTest {
     expect(contents[filePath], equals(expectedContent));
   }
 
+  Future<void> test_addImport_noPreference() async {
+    newFile(
+      join(projectFolderPath, 'lib', 'class.dart'),
+      'class MyClass {}',
+    );
+
+    final code = TestCode.parse('''
+MyCla^ss? a;
+''');
+
+    newFile(mainFilePath, code.code);
+    await initialize(
+      textDocumentCapabilities: withCodeActionKinds(
+          emptyTextDocumentClientCapabilities, [CodeActionKind.QuickFix]),
+    );
+
+    final codeActions =
+        await getCodeActions(mainFileUri, position: code.position.position);
+    final codeActionTitles = codeActions.map((action) =>
+        action.map((command) => command.title, (action) => action.title));
+
+    expect(
+      codeActionTitles,
+      // With no preference, server defaults to absolute.
+      containsAllInOrder([
+        "Import library 'package:test/class.dart'",
+        "Import library 'class.dart'",
+      ]),
+    );
+  }
+
+  Future<void> test_addImport_preferAbsolute() async {
+    _enableLints(['always_use_package_imports']);
+
+    newFile(
+      join(projectFolderPath, 'lib', 'class.dart'),
+      'class MyClass {}',
+    );
+
+    final code = TestCode.parse('''
+MyCla^ss? a;
+''');
+
+    newFile(mainFilePath, code.code);
+    await initialize(
+      textDocumentCapabilities: withCodeActionKinds(
+          emptyTextDocumentClientCapabilities, [CodeActionKind.QuickFix]),
+    );
+
+    final codeActions =
+        await getCodeActions(mainFileUri, position: code.position.position);
+    final codeActionTitles = codeActions.map((action) =>
+        action.map((command) => command.title, (action) => action.title));
+
+    expect(
+      codeActionTitles,
+      containsAllInOrder([
+        "Import library 'package:test/class.dart'",
+        "Import library 'class.dart'",
+      ]),
+    );
+  }
+
+  Future<void> test_addImport_preferRelative() async {
+    _enableLints(['prefer_relative_imports']);
+
+    newFile(
+      join(projectFolderPath, 'lib', 'class.dart'),
+      'class MyClass {}',
+    );
+
+    final code = TestCode.parse('''
+MyCla^ss? a;
+''');
+
+    newFile(mainFilePath, code.code);
+    await initialize(
+      textDocumentCapabilities: withCodeActionKinds(
+          emptyTextDocumentClientCapabilities, [CodeActionKind.QuickFix]),
+    );
+
+    final codeActions =
+        await getCodeActions(mainFileUri, position: code.position.position);
+    final codeActionTitles = codeActions.map((action) =>
+        action.map((command) => command.title, (action) => action.title));
+
+    expect(
+      codeActionTitles,
+      containsAllInOrder([
+        "Import library 'class.dart'",
+        "Import library 'package:test/class.dart'",
+      ]),
+    );
+  }
+
   Future<void> test_analysisOptions() async {
     registerLintRules();
 
@@ -400,12 +495,13 @@ void main() {
     expect(
       codeActionKinds,
       containsAllInOrder([
-        // Non-ignore fixes are alphabetical by title.
+        // Non-ignore fixes (order doesn't matter here, but this is what
+        // server produces).
         'quickfix.create.class',
-        'quickfix.create.localVariable',
         'quickfix.create.mixin',
+        'quickfix.create.localVariable',
         'quickfix.remove.unusedLocalVariable',
-        // Fixes are last and line is always sorted above file.
+        // Ignore fixes last, with line sorted above file.
         'quickfix.ignore.line',
         'quickfix.ignore.file',
       ]),
@@ -850,4 +946,14 @@ useFunction(int g(a, b)) {}
     applyDocumentChanges(contents, edit.documentChanges!);
     expect(contents[mainFilePath], equals(expectedContent));
   }
+
+  void _enableLints(List<String> lintNames) {
+    registerLintRules();
+    final lintsYaml = lintNames.map((name) => '    - $name\n').join();
+    newFile(analysisOptionsPath, '''
+linter:
+  rules:
+$lintsYaml
+''');
+  }
 }