diff --git a/pkg/analysis_server/lib/plugin/edit/fix/fix_dart.dart b/pkg/analysis_server/lib/plugin/edit/fix/fix_dart.dart index 327a6e68b53..634ae798c31 100644 --- a/pkg/analysis_server/lib/plugin/edit/fix/fix_dart.dart +++ b/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; diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all.dart index 3cbb9aa4081..0200e1bd81f 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all.dart +++ b/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); } diff --git a/pkg/analysis_server/lib/src/services/correction/bulk_fix_processor.dart b/pkg/analysis_server/lib/src/services/correction/bulk_fix_processor.dart index d8ed3d6978c..500da3934d1 100644 --- a/pkg/analysis_server/lib/src/services/correction/bulk_fix_processor.dart +++ b/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 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 _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? 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 = []; - 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> 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) { diff --git a/pkg/analysis_server/lib/src/services/correction/dart/abstract_producer.dart b/pkg/analysis_server/lib/src/services/correction/dart/abstract_producer.dart index 9cc5ee10575..bb87350ed84 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/abstract_producer.dart +++ b/pkg/analysis_server/lib/src/services/correction/dart/abstract_producer.dart @@ -48,17 +48,36 @@ abstract class CorrectionProducer /// 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, diff --git a/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_catch_clause.dart b/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_catch_clause.dart index 291b874286a..d4197ff2a1a 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_catch_clause.dart +++ b/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; diff --git a/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_catch_stack.dart b/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_catch_stack.dart index 0da46620a33..8c769ca6fc2 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_catch_stack.dart +++ b/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; diff --git a/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_parameter.dart b/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_parameter.dart index 91183f5e301..714e595a204 100644 --- a/pkg/analysis_server/lib/src/services/correction/dart/remove_unused_parameter.dart +++ b/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; diff --git a/pkg/analysis_server/lib/src/services/correction/fix.dart b/pkg/analysis_server/lib/src/services/correction/fix.dart index cd4b45ea89c..d48a5e335d8 100644 --- a/pkg/analysis_server/lib/src/services/correction/fix.dart +++ b/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> getTopLevelDeclarations( diff --git a/pkg/analysis_server/test/lsp/code_actions_source_test.dart b/pkg/analysis_server/test/lsp/code_actions_source_test.dart index 12ff654e8b4..39c75cad8f5 100644 --- a/pkg/analysis_server/test/lsp/code_actions_source_test.dart +++ b/pkg/analysis_server/test/lsp/code_actions_source_test.dart @@ -195,6 +195,58 @@ void f() { expect(edit.textDocument.version, 1); } + Future 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 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 test_unavailable_outsideAnalysisRoot() async { final otherFile = convertPath('/other/file.dart'); final content = '';