[analysis_server] Don't remove unused parameters when running fix-all-on-save

Fixes https://github.com/Dart-Code/Dart-Code/issues/4655

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

pkg/analysis_server/lib/plugin/edit/fix/fix_dart.dart

@@ -12,6 +12,15 @@ import 'package:analyzer_plugin/utilities/change_builder/change_workspace.dart';
 ///
 /// Clients may not extend, implement or mix-in this class.
 abstract class DartFixContext implements FixContext {
+  /// Whether fixes were triggered automatically (for example by a save
+  /// operation).
+  ///
+  /// Some fixes may be excluded when running automatically. For example
+  /// removing unused imports or parameters is less acceptable while the code is
+  /// incomplete and being worked on than when manually executing fixes ready
+  /// for committing.
+  bool get autoTriggered;
+
   /// Return the instrumentation service used to report errors that prevent a
   /// fix from being composed.
   InstrumentationService get instrumentationService;

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

@@ -126,7 +126,7 @@ class _FixAllOperation extends TemporaryOverlayOperation
     );
 
     var changes = await processor.fixErrorsForFile(message.performance, path,
-        removeUnusedImports: !autoTriggered);
+        autoTriggered: autoTriggered);
     if (changes.isEmpty) {
       return success(null);
     }

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

@@ -243,7 +243,7 @@ class BulkFixProcessor {
   /// diagnostics in [file] in the given [context].
   Future<ChangeBuilder> fixErrorsForFile(OperationPerformanceImpl performance,
       AnalysisContext context, String path,
-      {bool removeUnusedImports = true}) async {
+      {required bool autoTriggered}) async {
     var pathContext = context.contextRoot.resourceProvider.pathContext;
 
     if (file_paths.isDart(pathContext, path) && !file_paths.isGenerated(path)) {
@@ -252,8 +252,7 @@ class BulkFixProcessor {
         (_) => context.currentSession.getResolvedLibrary(path),
       );
       if (!isCancelled && library is ResolvedLibraryResult) {
-        await _fixErrorsInLibrary(library,
-            removeUnusedImports: removeUnusedImports);
+        await _fixErrorsInLibrary(library, autoTriggered: autoTriggered);
       }
     }
 
@@ -298,7 +297,10 @@ class BulkFixProcessor {
       CorrectionProducerContext context) async {
     for (var generator in generators) {
       var producer = generator();
-      if (producer.canBeAppliedInBulk) {
+      var shouldFix = (context.dartFixContext?.autoTriggered ?? false)
+          ? producer.canBeAppliedAutomatically
+          : producer.canBeAppliedInBulk;
+      if (shouldFix) {
         await _generateFix(context, producer, codeName);
         if (isCancelled) {
           return;
@@ -465,25 +467,31 @@ class BulkFixProcessor {
   /// Use the change [builder] to create fixes for the diagnostics in the
   /// library associated with the analysis [result].
   Future<void> _fixErrorsInLibrary(ResolvedLibraryResult result,
-      {bool stopAfterFirst = false, bool removeUnusedImports = true}) async {
+      {bool stopAfterFirst = false, bool autoTriggered = false}) async {
     var analysisOptions = result.session.analysisContext.analysisOptions;
 
     DartFixContextImpl fixContext(
-        ResolvedUnitResult result, AnalysisError diagnostic) {
+      ResolvedUnitResult result,
+      AnalysisError diagnostic, {
+      required bool autoTriggered,
+    }) {
       return DartFixContextImpl(
         instrumentationService,
         workspace,
         result,
         diagnostic,
+        autoTriggered: autoTriggered,
       );
     }
 
     CorrectionProducerContext<ResolvedUnitResult>? correctionContext(
         ResolvedUnitResult result, AnalysisError diagnostic) {
       var overrideSet = _readOverrideSet(result);
+      var context =
+          fixContext(result, diagnostic, autoTriggered: autoTriggered);
       return CorrectionProducerContext.createResolved(
         applyingBulkFixes: true,
-        dartFixContext: fixContext(result, diagnostic),
+        dartFixContext: context,
         diagnostic: diagnostic,
         overrideSet: overrideSet,
         resolvedResult: result,
@@ -499,8 +507,9 @@ class BulkFixProcessor {
     for (var unitResult in result.units) {
       var overrideSet = _readOverrideSet(unitResult);
       for (var error in _filterErrors(analysisOptions, unitResult.errors)) {
-        await _fixSingleError(
-            fixContext(unitResult, error), unitResult, error, overrideSet);
+        var context =
+            fixContext(unitResult, error, autoTriggered: autoTriggered);
+        await _fixSingleError(context, unitResult, error, overrideSet);
         if (isCancelled || (stopAfterFirst && changeMap.hasFixes)) {
           return;
         }
@@ -513,7 +522,7 @@ class BulkFixProcessor {
     var definingUnit = result.units[0];
     AnalysisError? directivesOrderingError;
     var unusedImportErrors = <AnalysisError>[];
-    if (removeUnusedImports && !builder.hasEditsFor(definingUnit.path)) {
+    if (!autoTriggered && !builder.hasEditsFor(definingUnit.path)) {
       for (var error in _filterErrors(analysisOptions, definingUnit.errors)) {
         var errorCode = error.errorCode;
         if (errorCode is LintCode) {
@@ -834,7 +843,7 @@ class IterativeBulkFixProcessor {
   Future<List<SourceFileEdit>> fixErrorsForFile(
     OperationPerformanceImpl performance,
     String path, {
-    bool removeUnusedImports = true,
+    required bool autoTriggered,
   }) async {
     return performance.runAsync('IterativeBulkFixProcessor.fixErrorsForFile',
         (performance) async {
@@ -850,7 +859,7 @@ class IterativeBulkFixProcessor {
           'BulkFixProcessor.fixErrorsForFile pass $i',
           (performance) => processor.fixErrorsForFile(
               performance, context, path,
-              removeUnusedImports: removeUnusedImports),
+              autoTriggered: autoTriggered),
         );
 
         if (isCancelled) {

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

@@ -48,17 +48,36 @@ abstract class CorrectionProducer<T extends ParsedUnitResult>
   /// if this producer doesn't support assists.
   AssistKind? get assistKind => null;
 
+  /// Return `true` if fixes from this producer are acceptable to run
+  /// automatically (such as during a save operation) when code could be
+  /// incomplete.
+  ///
+  /// By default this value matches [canBeAppliedInBulk] but may return `false`
+  /// for fixes that perform actions like removing unused code, which could be
+  /// unused only because the code is still being worked on.
+  bool get canBeAppliedAutomatically => canBeAppliedInBulk;
+
   /// Return `true` if this producer can be used to fix diagnostics across
-  /// multiple files. Cases where this will return `false` include fixes for
-  /// which
+  /// multiple files and/or at the same time as applying fixes from other
+  /// producers.
+  ///
+  /// This flag is used when the user has chosen to apply fixes but may not have
+  /// chosen to apply a specific fix (such as running `dart fix`).
+  ///
+  /// Cases where this will return `false` include fixes for which
   /// - the modified regions can overlap, and
   /// - fixes that have not been tested to ensure that they can be used this
   ///   way.
   bool get canBeAppliedInBulk => false;
 
   /// Return `true` if this producer can be used to fix multiple diagnostics in
-  /// the same file. Cases where this will return `false` include fixes for
-  /// which
+  /// the same file.
+  ///
+  /// Unlike [canBeAppliedInBulk], this flag is used to provide the option for
+  /// a user to fix a specific diagnostic across a file (such as a quick-fix to
+  /// "fix all x in this file").
+  ///
+  /// Cases where this will return `false` include fixes for which
   /// - the modified regions can overlap,
   /// - the fix for one diagnostic would fix all diagnostics with the same code,
   ///   and,

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

@@ -10,6 +10,10 @@ import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
 import 'package:analyzer_plugin/utilities/range_factory.dart';
 
 class RemoveUnusedCatchClause extends ResolvedCorrectionProducer {
+  @override
+  // May not be appropriate while actively coding.
+  bool get canBeAppliedAutomatically => false;
+
   @override
   bool get canBeAppliedInBulk => true;
 

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

@@ -10,6 +10,10 @@ import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
 import 'package:analyzer_plugin/utilities/range_factory.dart';
 
 class RemoveUnusedCatchStack extends ResolvedCorrectionProducer {
+  @override
+  // May not be appropriate while actively coding.
+  bool get canBeAppliedAutomatically => false;
+
   @override
   bool get canBeAppliedInBulk => true;
 

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

@@ -11,6 +11,9 @@ import 'package:analyzer_plugin/utilities/fixes/fixes.dart';
 import 'package:analyzer_plugin/utilities/range_factory.dart';
 
 class RemoveUnusedParameter extends ResolvedCorrectionProducer {
+  @override
+  bool get canBeAppliedAutomatically => false;
+
   @override
   bool get canBeAppliedInBulk => true;
 

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

@@ -67,8 +67,12 @@ class DartFixContextImpl implements DartFixContext {
   @override
   final AnalysisError error;
 
+  @override
+  final bool autoTriggered;
+
   DartFixContextImpl(this.instrumentationService, this.workspace,
-      this.resolveResult, this.error);
+      this.resolveResult, this.error,
+      {this.autoTriggered = false});
 
   @override
   Future<Map<LibraryElement, Element>> getTopLevelDeclarations(

pkg/analysis_server/test/lsp/code_actions_source_test.dart

@@ -195,6 +195,58 @@ void f() {
     expect(edit.textDocument.version, 1);
   }
 
+  Future<void> test_privateUnusedParameters_notRemovedIfSave() async {
+    const content = '''
+class _MyClass {
+  int? _param;
+  _MyClass({
+    this._param,
+  });
+}
+''';
+
+    final codeAction = await expectAction(
+      content,
+      command: Commands.fixAll,
+      triggerKind: CodeActionTriggerKind.Automatic,
+    );
+    final command = codeAction.command!;
+
+    // We should not get an applyEdit call during the command execution because
+    // no edits should be produced.
+    final applyEditSubscription = requestsFromServer
+        .where((n) => n.method == Method.workspace_applyEdit)
+        .listen((_) => throw 'workspace/applyEdit was unexpectedly called');
+    final commandResponse = await executeCommand(command);
+    expect(commandResponse, isNull);
+
+    await pumpEventQueue();
+    await applyEditSubscription.cancel();
+  }
+
+  Future<void> test_privateUnusedParameters_removedByDefault() async {
+    const content = '''
+class _MyClass {
+  int? param;
+  _MyClass({
+    this.param,
+  });
+}
+''';
+    const expectedContent = '''
+class _MyClass {
+  int? param;
+  _MyClass();
+}
+''';
+
+    await verifyActionEdits(
+      content,
+      expectedContent,
+      command: Commands.fixAll,
+    );
+  }
+
   Future<void> test_unavailable_outsideAnalysisRoot() async {
     final otherFile = convertPath('/other/file.dart');
     final content = '';