[DAS] Fixes adding overrides first in bulk-fixes

Fixes: #61301
Change-Id: I73a97fc45089a6818c3255050880ed61a608194e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/444984
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Auto-Submit: Felipe Morschel <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

Author: FMorschel

pkg/analysis_server/integration_test/edit/bulk_fixes_test.dart

@@ -37,6 +37,31 @@ class B extends A {
     expect(result.edits, hasLength(1));
   }
 
+  Future<void> test_bulk_fix_override_first() async {
+    writeFile(sourcePath(file_paths.analysisOptionsYaml), '''
+linter:
+  rules:
+    - always_declare_return_types
+    - annotate_overrides
+''');
+    writeFile(sourcePath('test.dart'), '''
+abstract class A {
+  void foo();
+}
+
+class B extends A {
+  foo() {}
+}
+''');
+    await standardAnalysisSetup();
+    await analysisFinished;
+
+    var result = await sendEditBulkFixes([sourceDirectory.path]);
+    var edits = result.edits;
+    expect(edits, hasLength(1));
+    expect(edits.single.edits, hasLength(2));
+  }
+
   Future<void> test_bulk_fix_with_parts() async {
     writeFile(sourcePath(file_paths.analysisOptionsYaml), '''
 linter:
@@ -71,7 +96,7 @@ void a() {
     var result = await sendEditBulkFixes([sourceDirectory.path]);
     var edits = result.edits;
     expect(edits, hasLength(2));
-    expect(edits[0].edits, hasLength(1));
-    expect(edits[1].edits, hasLength(1));
+    expect(edits.first.edits, hasLength(1));
+    expect(edits.last.edits, hasLength(1));
   }
 }

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

@@ -50,6 +50,7 @@ import 'package:analyzer_plugin/protocol/protocol_common.dart'
 import 'package:analyzer_plugin/src/utilities/change_builder/change_builder_core.dart';
 import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
 import 'package:analyzer_plugin/utilities/change_builder/conflicting_edit_exception.dart';
+import 'package:linter/src/lint_codes.dart';
 import 'package:linter/src/lint_names.dart';
 import 'package:linter/src/rules/directives_ordering.dart';
 import 'package:meta/meta.dart';
@@ -545,7 +546,7 @@ class BulkFixProcessor {
     List<Diagnostic> originalDiagnostics,
   ) sync* {
     var diagnostics = originalDiagnostics.toList();
-    diagnostics.sort((a, b) => a.offset.compareTo(b.offset));
+    diagnostics.sort(_fixOrder);
     for (var diagnostic in diagnostics) {
       if (_codes != null &&
           !_codes.contains(diagnostic.diagnosticCode.name.toLowerCase())) {
@@ -700,6 +701,23 @@ class BulkFixProcessor {
     }
   }
 
+  int _fixOrder(Diagnostic a, Diagnostic b) {
+    var result = a.offset.compareTo(b.offset);
+    if (result != 0) {
+      return result;
+    }
+    // Special casing for `annotate_overrides` fix order
+    // See https://github.com/dart-lang/sdk/issues/61301
+    // Since the output for it should be before any other fixes like editing
+    // the return type
+    if (a.diagnosticCode == LinterLintCode.annotateOverrides) {
+      return -1;
+    } else if (b.diagnosticCode == LinterLintCode.annotateOverrides) {
+      return 1;
+    }
+    return 0;
+  }
+
   /// Uses the change [builder] and the [fixContext] to create a fix for the
   /// given [diagnostic] in the compilation unit associated with the analysis
   /// [unitResult].

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

@@ -67,11 +67,7 @@ class AddReturnType extends ResolvedCorrectionProducer {
     await builder.addDartFileEdit(file, (builder) {
       if (returnType is DynamicType || builder.canWriteType(returnType)) {
         builder.addInsertion(insertBeforeEntity.offset, (builder) {
-          if (returnType is DynamicType) {
-            builder.write('dynamic');
-          } else {
-            builder.writeType(returnType);
-          }
+          builder.writeType(returnType, shouldWriteDynamic: true);
           builder.write(' ');
         });
       }

pkg/analysis_server/test/lsp/commands/fix_all_in_workspace_test.dart

@@ -83,6 +83,46 @@ bool f() {
     setChangeAnnotationSupport();
   }
 
+  Future<void> test_annotatate_overrides_return_types() async {
+    newFile(analysisOptionsPath, '''
+linter:
+  rules:
+    - always_declare_return_types
+    - annotate_overrides
+    ''');
+
+    newFile(join(projectFolderPath, 'lib', 'a.dart'), '''
+abstract class A {
+  void foo();
+}
+
+class B extends A {
+  foo() {}
+}
+''');
+
+    await initialize();
+    var verifier = await verifyCommandEdits(
+      Command(command: commandId, title: 'UNUSED'),
+      '''
+>>>>>>>>>> lib/a.dart
+>>>>>>>>>>   Add '@override' annotation: line 6
+>>>>>>>>>>   Add return type: line 6
+abstract class A {
+  void foo();
+}
+
+class B extends A {
+  @override
+  void foo() {}
+}
+''',
+    );
+    verifier.edit.changeAnnotations?.values.forEach((annotation) {
+      expect(annotation.needsConfirmation, expectRequiresConfirmation);
+    });
+  }
+
   Future<void> test_documentChanges_notSupported() async {
     setDocumentChangesSupport(false);
 

pkg/analysis_server/test/src/services/correction/fix/bulk_fix_processor_test.dart

@@ -118,7 +118,7 @@ part 'a.dart';
       lints: [LintNames.unnecessary_new],
     );
 
-    newFile('$testPackageLibPath/a.dart', '''
+    newFile('$testPackageLibPath/c.dart', '''
 part 'test.dart';
 part 'b.dart';
 
@@ -128,27 +128,28 @@ var c = C();
 ''');
 
     newFile('$testPackageLibPath/b.dart', '''
-part of 'a.dart';
+part of 'c.dart';
 
-class B { }
+class B {}
 
 var b = new B();
 ''');
 
     await resolveTestCode('''
-part of 'a.dart';
+part of 'c.dart';
 
-class A { }
+class A {}
 
 var a = new A();
 ''');
 
     expect(await computeHasFixes(), isTrue);
-    expect(processor.changeMap.libraryMap.length, 1);
+    processor = await computeFixes();
+    expect(processor.changeMap.libraryMap, hasLength(2));
   }
 
-  /// https://github.com/dart-lang/sdk/issues/59572
   Future<void> test_hasFixes_in_part_and_library2() async {
+    // https://github.com/dart-lang/sdk/issues/59572
     createAnalysisOptionsFile(
       experiments: experiments,
       lints: [LintNames.empty_statements, LintNames.prefer_const_constructors],
@@ -178,10 +179,9 @@ void a() {
 ''');
 
     expect(await computeHasFixes(), isTrue);
-    expect(processor.changeMap.libraryMap.length, 1);
-    expect(processor.fixDetails.length, 1);
-    var details = processor.fixDetails;
-    expect(details.first.fixes, hasLength(1));
+    processor = await computeFixes();
+    expect(processor.changeMap.libraryMap, hasLength(2));
+    expect(processor.fixDetails.expand((d) => d.fixes), hasLength(2));
   }
 
   Future<void> test_hasFixes_stoppedAfterFirst() async {
@@ -206,9 +206,7 @@ var a = new A();
   Future<void> test_noFixes() async {
     createAnalysisOptionsFile(
       experiments: experiments,
-      lints: [
-        'avoid_catching_errors', // NOTE: not in lintProducerMap
-      ],
+      lints: [LintNames.avoid_catching_errors],
     );
 
     await resolveTestCode('''
@@ -222,6 +220,30 @@ void bad() {
 
     expect(await computeHasFixes(), isFalse);
   }
+
+  Future<void> test_override_first() async {
+    createAnalysisOptionsFile(
+      experiments: experiments,
+      lints: [
+        LintNames.annotate_overrides,
+        LintNames.always_declare_return_types,
+      ],
+    );
+    await resolveTestCode('''
+abstract class A {
+  void foo();
+}
+
+class B extends A {
+  foo() {}
+}
+''');
+
+    expect(await computeHasFixes(), isTrue);
+    processor = await computeFixes();
+    expect(processor.fixDetails.single.fixes, hasLength(2));
+    expect(processor.builder.sourceChange.edits.single.edits, hasLength(2));
+  }
 }
 
 @reflectiveTest
@@ -230,9 +252,7 @@ class NoFixTest extends BulkFixProcessorTest {
   Future<void> test_noFix() async {
     createAnalysisOptionsFile(
       experiments: experiments,
-      lints: [
-        'avoid_catching_errors', // NOTE: not in lintProducerMap
-      ],
+      lints: [LintNames.avoid_catching_errors],
     );
 
     await resolveTestCode('''