[analysis_server] Make Sort Members + Organize Imports fail silently if invoked automatically

Previously, Organise Imports always failed silently (since it was often run on-save). Sort Members had not been given the same treatment (but often now runs on-save).

This uses a new LSP 3.17 field to know when the command was run automatically, so both are now consistent, showing errors if invoked manually and failing silently if run automatically.

Change-Id: I48e8380fcee4e54d0f4dc3c177c9ae2362627efb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/247542
Commit-Queue: Brian Wilkerson <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>

Author: DanTup

pkg/analysis_server/lib/src/lsp/handlers/commands/organize_imports.dart

@@ -30,6 +30,7 @@ class OrganizeImportsCommandHandler extends SimpleEditCommandHandler {
     // modified since.
     final path = parameters['path'] as String;
     final docIdentifier = server.getVersionedDocumentIdentifier(path);
+    final autoTriggered = (parameters['autoTriggered'] as bool?) ?? false;
 
     final result = await requireResolvedUnit(path);
 
@@ -42,13 +43,15 @@ class OrganizeImportsCommandHandler extends SimpleEditCommandHandler {
       final unit = result.unit;
 
       if (hasScanParseErrors(result.errors)) {
-        // It's not uncommon for editors to run this command automatically on-save
-        // so if the file in in an invalid state it's better to fail silently
-        // than trigger errors (VS Code recently started showing popups when
-        // LSP requests return errors).
-        server.instrumentationService.logInfo(
-            'Unable to $commandName because the file contains parse errors');
-        return success(null);
+        if (autoTriggered) {
+          return success(null);
+        }
+        return ErrorOr.error(ResponseError(
+          code: ServerErrorCodes.FileHasErrors,
+          message:
+              'Unable to $commandName because the file contains parse errors',
+          data: path,
+        ));
       }
 
       final organizer = ImportOrganizer(code, unit, result.errors);

pkg/analysis_server/lib/src/lsp/handlers/commands/sort_members.dart

@@ -31,6 +31,7 @@ class SortMembersCommandHandler extends SimpleEditCommandHandler {
     // modified since.
     final path = parameters['path'] as String;
     final docIdentifier = server.getVersionedDocumentIdentifier(path);
+    final autoTriggered = (parameters['autoTriggered'] as bool?) ?? false;
 
     var session = await server.getAnalysisSession(path);
     final result = session?.getParsedUnit(path);
@@ -40,6 +41,9 @@ class SortMembersCommandHandler extends SimpleEditCommandHandler {
     }
 
     if (result is! ParsedUnitResult) {
+      if (autoTriggered) {
+        return success(null);
+      }
       return ErrorOr.error(ResponseError(
         code: ServerErrorCodes.FileNotAnalyzed,
         message: '$commandName is only available for analyzed files',
@@ -50,6 +54,9 @@ class SortMembersCommandHandler extends SimpleEditCommandHandler {
     final unit = result.unit;
 
     if (hasScanParseErrors(result.errors)) {
+      if (autoTriggered) {
+        return success(null);
+      }
       return ErrorOr.error(ResponseError(
         code: ServerErrorCodes.FileHasErrors,
         message:

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

@@ -161,6 +161,7 @@ class CodeActionHandler extends MessageHandler<CodeActionParams,
               offset,
               length,
               unit,
+              params.context.triggerKind,
             ),
           );
         });
@@ -344,6 +345,7 @@ class CodeActionHandler extends MessageHandler<CodeActionParams,
     int offset,
     int length,
     ResolvedUnitResult unit,
+    CodeActionTriggerKind? triggerKind,
   ) async {
     final docIdentifier = server.getVersionedDocumentIdentifier(path);
 
@@ -352,7 +354,7 @@ class CodeActionHandler extends MessageHandler<CodeActionParams,
         performance.runAsync(
           '_getSourceActions',
           (_) => _getSourceActions(shouldIncludeKind, supportsLiterals,
-              supportsWorkspaceApplyEdit, path),
+              supportsWorkspaceApplyEdit, path, triggerKind),
         ),
       // Assists go under the Refactor CodeActionKind so check that here.
       if (shouldIncludeAnyOfKind(CodeActionKind.Refactor))
@@ -644,6 +646,7 @@ class CodeActionHandler extends MessageHandler<CodeActionParams,
     bool supportsLiteralCodeActions,
     bool supportsApplyEdit,
     String path,
+    CodeActionTriggerKind? triggerKind,
   ) async {
     // The source actions supported are only valid for Dart files.
     var pathContext = server.resourceProvider.pathContext;
@@ -657,6 +660,8 @@ class CodeActionHandler extends MessageHandler<CodeActionParams,
       return const [];
     }
 
+    final autoTriggered = triggerKind == CodeActionTriggerKind.Automatic;
+
     return [
       if (shouldIncludeKind(DartCodeActionKind.SortMembers))
         _commandOrCodeAction(
@@ -666,7 +671,10 @@ class CodeActionHandler extends MessageHandler<CodeActionParams,
               title: 'Sort Members',
               command: Commands.sortMembers,
               arguments: [
-                {'path': path}
+                {
+                  'path': path,
+                  if (autoTriggered) 'autoTriggered': true,
+                }
               ]),
         ),
       if (shouldIncludeKind(CodeActionKind.SourceOrganizeImports))
@@ -677,7 +685,10 @@ class CodeActionHandler extends MessageHandler<CodeActionParams,
               title: 'Organize Imports',
               command: Commands.organizeImports,
               arguments: [
-                {'path': path}
+                {
+                  'path': path,
+                  if (autoTriggered) 'autoTriggered': true,
+                }
               ]),
         ),
       if (shouldIncludeKind(DartCodeActionKind.FixAll))
@@ -688,7 +699,10 @@ class CodeActionHandler extends MessageHandler<CodeActionParams,
             title: 'Fix All',
             command: Commands.fixAll,
             arguments: [
-              {'path': path}
+              {
+                'path': path,
+                if (autoTriggered) 'autoTriggered': true,
+              }
             ],
           ),
         ),

pkg/analysis_server/test/lsp/code_actions_source_test.dart

@@ -139,7 +139,30 @@ int minified(int x, int y) => min(x, y);
     );
   }
 
-  Future<void> test_failsSilentlyIfFileHasErrors() async {
+  Future<void> test_fileHasErrors_failsSilentlyForAutomatic() async {
+    final content = 'invalid dart code';
+    newFile(mainFilePath, content);
+    await initialize(
+        workspaceCapabilities:
+            withApplyEditSupport(emptyWorkspaceClientCapabilities));
+
+    final codeActions = await getCodeActions(
+      mainFileUri.toString(),
+      triggerKind: CodeActionTriggerKind.Automatic,
+    );
+    final codeAction = findCommand(codeActions, Commands.organizeImports)!;
+
+    final command = codeAction.map(
+      (command) => command,
+      (codeAction) => codeAction.command!,
+    );
+
+    // Expect a valid null result.
+    final response = await executeCommand(command);
+    expect(response, isNull);
+  }
+
+  Future<void> test_fileHasErrors_failsWithErrorForManual() async {
     final content = 'invalid dart code';
     newFile(mainFilePath, content);
     await initialize(
@@ -154,10 +177,10 @@ int minified(int x, int y) => min(x, y);
       (codeAction) => codeAction.command!,
     );
 
-    final commandResponse = await executeCommand(command);
-    // Invalid code returns an empty success() response to avoid triggering
-    // errors in the editor if run automatically on every save.
-    expect(commandResponse, isNull);
+    // Ensure the request returned an error (error repsonses are thrown by
+    // the test helper to make consuming success results simpler).
+    await expectLater(executeCommand(command),
+        throwsA(isResponseError(ServerErrorCodes.FileHasErrors)));
   }
 
   Future<void> test_filtersCorrectly() async {
@@ -339,7 +362,30 @@ class SortMembersSourceCodeActionsTest extends AbstractCodeActionsTest {
         throwsA(isResponseError(ServerErrorCodes.ClientFailedToApplyEdit)));
   }
 
-  Future<void> test_failsIfFileHasErrors() async {
+  Future<void> test_fileHasErrors_failsSilentlyForAutomatic() async {
+    final content = 'invalid dart code';
+    newFile(mainFilePath, content);
+    await initialize(
+        workspaceCapabilities:
+            withApplyEditSupport(emptyWorkspaceClientCapabilities));
+
+    final codeActions = await getCodeActions(
+      mainFileUri.toString(),
+      triggerKind: CodeActionTriggerKind.Automatic,
+    );
+    final codeAction = findCommand(codeActions, Commands.sortMembers)!;
+
+    final command = codeAction.map(
+      (command) => command,
+      (codeAction) => codeAction.command!,
+    );
+
+    // Expect a valid null result.
+    final response = await executeCommand(command);
+    expect(response, isNull);
+  }
+
+  Future<void> test_fileHasErrors_failsWithErrorForManual() async {
     final content = 'invalid dart code';
     newFile(mainFilePath, content);
     await initialize(

pkg/analysis_server/test/lsp/server_abstract.dart

@@ -1016,6 +1016,7 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
     Range? range,
     Position? position,
     List<CodeActionKind>? kinds,
+    CodeActionTriggerKind? triggerKind,
   }) {
     range ??= position != null
         ? Range(start: position, end: position)
@@ -1025,10 +1026,14 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
       CodeActionParams(
         textDocument: TextDocumentIdentifier(uri: fileUri),
         range: range,
-        // TODO(dantup): We may need to revise the tests/implementation when
-        // it's clear how we're supposed to handle diagnostics:
-        // https://github.com/Microsoft/language-server-protocol/issues/583
-        context: CodeActionContext(diagnostics: [], only: kinds),
+        context: CodeActionContext(
+          // TODO(dantup): We may need to revise the tests/implementation when
+          // it's clear how we're supposed to handle diagnostics:
+          // https://github.com/Microsoft/language-server-protocol/issues/583
+          diagnostics: [],
+          only: kinds,
+          triggerKind: triggerKind,
+        ),
       ),
     );
     return expectSuccessfulResponseTo(