[analysis_server] Fix handling of edits that require sequential processing

This moves the check for whether we need to apply edits sequentially to above some of the validation checks (because they are only applicable if not applying sequentially) and tweaks the logic slightly.

It also removes a third copy of this function in analysis_server that was only used in one test, so we're down to two copies now (analyzer_plugin and analysis_server_client).

Fixes #61501
Fixes #61488

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

Author: DanTup

pkg/analysis_server/lib/src/protocol/protocol_internal.dart

@@ -20,46 +20,6 @@ export 'package:analyzer_plugin/src/protocol/protocol_internal.dart'
 final Map<String, RefactoringKind> REQUEST_ID_REFACTORING_KINDS =
     HashMap<String, RefactoringKind>();
 
-/// Get the result of applying a set of [edits] to the given [code].  Edits
-/// are applied in the order they appear in [edits].  Access via
-/// SourceEdit.applySequence().
-String applySequenceOfEdits(String code, List<SourceEdit> edits) {
-  var buffer = StringBuffer();
-  var start = 0;
-  for (var i = edits.length - 1; i >= 0; i--) {
-    var edit = edits[i];
-    var offset = edit.offset;
-    var length = edit.length;
-    if (length < 0) {
-      throw RangeError('The edit length is negative.');
-    }
-    if (offset + length > code.length) {
-      throw RangeError('The edit extends past the end of the code.');
-    }
-    if (start > offset) {
-      // One of the edits overlaps with another, requiring that they be applied
-      // from largest offset to smallest. This should only be possible in code
-      // that creates source edits without using the `ChangeBuilder` to do so.
-      //
-      // We should consider fixing the places where overlapping edits are
-      // produced so that this branch can be removed. One such place is
-      // exhibited by `_DoCompletionTest.test_noBody`.
-      for (var edit in edits) {
-        code = edit.apply(code);
-      }
-      return code;
-    } else if (start < offset) {
-      buffer.write(code.substring(start, offset));
-    }
-    buffer.write(edit.replacement);
-    start = offset + length;
-  }
-  if (start < code.length) {
-    buffer.write(code.substring(start));
-  }
-  return buffer.toString();
-}
-
 /// Compare the lists [listA] and [listB], using [itemEqual] to compare
 /// list elements.
 bool listEqual<T>(

pkg/analysis_server/test/lsp/document_changes_test.dart

@@ -5,7 +5,6 @@
 import 'dart:async';
 
 import 'package:analysis_server/lsp_protocol/protocol.dart';
-import 'package:analysis_server/src/protocol/protocol_internal.dart';
 import 'package:analyzer_plugin/protocol/protocol_common.dart' hide Position;
 import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
@@ -60,7 +59,7 @@ class Bar {
             as ChangeContentOverlay;
 
     expect(
-      applySequenceOfEdits(content, notifiedChanges.edits),
+      SourceEdit.applySequence(content, notifiedChanges.edits),
       contentAfterUpdate,
     );
   }

pkg/analysis_server/test/services/correction/change_test.dart

@@ -132,7 +132,9 @@ class ChangeTest {
 
 @reflectiveTest
 class EditTest {
-  void test_applySequence() {
+  /// There is no ambiguity in edits sorted last-to-first and the implementation
+  /// may optimize this case.
+  void test_applySequence_lastToFirst() {
     var edit1 = SourceEdit(5, 2, 'abc');
     var edit2 = SourceEdit(1, 0, '!');
     expect(
@@ -141,6 +143,124 @@ class EditTest {
     );
   }
 
+  void test_applySequence_lastToFirst_invalidLength_negative() {
+    var edit1 = SourceEdit(1, 0, 'a');
+    var edit2 = SourceEdit(0, -1, '');
+    expect(
+      () => SourceEdit.applySequence('', [edit1, edit2]),
+      throwsRangeError('The edit length is negative.'),
+    );
+  }
+
+  void test_applySequence_lastToFirst_invalidLength_pastEndOfString() {
+    var edit1 = SourceEdit(1, 100, 'a');
+    var edit2 = SourceEdit(0, 0, '');
+    expect(
+      () => SourceEdit.applySequence('aa', [edit1, edit2]),
+      throwsRangeError('The edit extends past the end of the code.'),
+    );
+  }
+
+  void test_applySequence_lastToFirst_invalidOffset_negative() {
+    var edit1 = SourceEdit(1, 0, 'a');
+    var edit2 = SourceEdit(-1, 0, '');
+    expect(
+      () => SourceEdit.applySequence('', [edit1, edit2]),
+      throwsRangeError('The edit offset is negative.'),
+    );
+  }
+
+  void test_applySequence_lastToFirst_invalidOffset_pastEndOfString() {
+    var edit1 = SourceEdit(1, 0, 'a');
+    var edit2 = SourceEdit(0, 0, 'b');
+    expect(
+      () => SourceEdit.applySequence('', [edit1, edit2]),
+      throwsRangeError('The edit starts past the end of the code.'),
+    );
+  }
+
+  /// Last-to-first offsets may have overlaps if edit `n+1` replaces text
+  /// inserted by edit `n`. The result should be as if they were applied
+  /// sequentially.
+  ///
+  /// Although sorted last-to-first, this case will fall back to sequential
+  /// processing.
+  void test_applySequence_lastToFirstOffsets_overlap() {
+    var edit1 = SourceEdit(3, 0, '1111');
+    var edit2 = SourceEdit(1, 4, '2222'); // replaces aa11
+    expect(SourceEdit.applySequence('aaaa', [edit1, edit2]), 'a222211a');
+  }
+
+  /// Last-to-first offsets may have overlaps if edit `n+1` replaces text
+  /// inserted by edit `n`. The result should be as if they were applied
+  /// sequentially.
+  ///
+  /// Although sorted last-to-first, this case will fall back to sequential
+  /// processing.
+  void test_applySequence_lastToFirstOffsets_touching() {
+    var edit1 = SourceEdit(3, 0, '1111');
+    var edit2 = SourceEdit(1, 2, '2222'); // replaces aa
+    expect(SourceEdit.applySequence('aaaa', [edit1, edit2]), 'a22221111a');
+  }
+
+  /// Edits are described sequentially, so the offsets in edit `n` assume edit
+  /// `n-1` has been applied.
+  void test_applySequence_sequential() {
+    var edit1 = SourceEdit(0, 0, '1111');
+    var edit2 = SourceEdit(2, 0, '2222');
+    expect(SourceEdit.applySequence('', [edit1, edit2]), '11222211');
+  }
+
+  void test_applySequence_sequential_invalidLength_negative() {
+    var edit1 = SourceEdit(0, -1, '');
+    var edit2 = SourceEdit(0, 0, '');
+    expect(
+      () => SourceEdit.applySequence('', [edit1, edit2]),
+      throwsRangeError('The edit length is negative.'),
+    );
+  }
+
+  void test_applySequence_sequential_invalidLength_pastEndOfString() {
+    var edit1 = SourceEdit(0, 0, '');
+    var edit2 = SourceEdit(0, 100, '');
+    expect(
+      () => SourceEdit.applySequence('', [edit1, edit2]),
+      throwsRangeError('The edit extends past the end of the code.'),
+    );
+  }
+
+  void test_applySequence_sequential_invalidOffset_negative() {
+    var edit1 = SourceEdit(-1, 0, '');
+    var edit2 = SourceEdit(0, 0, '');
+    expect(
+      () => SourceEdit.applySequence('', [edit1, edit2]),
+      throwsRangeError('The edit offset is negative.'),
+    );
+  }
+
+  void test_applySequence_sequential_invalidOffset_pastEndOfString() {
+    var edit1 = SourceEdit(0, 0, '');
+    var edit2 = SourceEdit(100, 0, '');
+    expect(
+      () => SourceEdit.applySequence('', [edit1, edit2]),
+      throwsRangeError('The edit starts past the end of the code.'),
+    );
+  }
+
+  /// Edits are described sequentially, so repeated offsets in inserts
+  /// result in the second insert ending up in front of the first.
+  void test_applySequence_sequential_sameOffsets() {
+    var edit1 = SourceEdit(0, 0, '1111');
+    var edit2 = SourceEdit(0, 0, '2222');
+    expect(SourceEdit.applySequence('', [edit1, edit2]), '22221111');
+  }
+
+  void test_applySequence_sequential_validLength_assumesPriorEdit() {
+    var edit1 = SourceEdit(0, 0, '1111');
+    var edit2 = SourceEdit(0, 4, ''); // Valid because it deletes 1111.
+    expect(SourceEdit.applySequence('', [edit1, edit2]), '');
+  }
+
   void test_editFromRange() {
     var range = SourceRange(1, 2);
     var edit = newSourceEdit_range(range, 'foo');
@@ -177,6 +297,14 @@ class EditTest {
     var expectedJson = {OFFSET: 1, LENGTH: 2, REPLACEMENT: 'foo'};
     expect(edit.toJson(), expectedJson);
   }
+
+  Matcher throwsRangeError(String message) => throwsA(
+    const TypeMatcher<RangeError>().having(
+      (e) => e.message,
+      'message',
+      message,
+    ),
+  );
 }
 
 @reflectiveTest

pkg/analysis_server_client/lib/src/protocol/protocol_internal.dart

@@ -69,44 +69,41 @@ void addEditToSourceChange(
   fileEdit.add(edit, insertBeforeExisting: insertBeforeExisting);
 }
 
-/// Get the result of applying the edit to the given [code]. Access via
-/// SourceEdit.apply().
+/// Get the result of applying the edit to the given [code].
+///
+/// Access via SourceEdit.apply().
 String applyEdit(String code, SourceEdit edit) {
-  if (edit.length < 0) {
-    throw RangeError('length is negative');
-  }
+  validateEdit(code, edit);
+
   return code.replaceRange(edit.offset, edit.end, edit.replacement);
 }
 
-/// Get the result of applying a set of [edits] to the given [code]. Edits
-/// are applied in the order they appear in [edits]. Access via
-/// SourceEdit.applySequence().
+/// Get the result of applying a set of [edits] to the given [code].
+///
+/// The edits are applied in the order in which they occur in the list. This
+/// means that the offset of each edit must be correct under the assumption
+/// that all previous edits have been applied.
+///
+/// Access via SourceEdit.applySequence().
 String applySequenceOfEdits(String code, List<SourceEdit> edits) {
+  // This function exists in both analysis_server_client and analyzer_plugin!
+
   var buffer = StringBuffer();
   var start = 0;
   for (var i = edits.length - 1; i >= 0; i--) {
     var edit = edits[i];
     var offset = edit.offset;
     var length = edit.length;
-    if (length < 0) {
-      throw RangeError('The edit length is negative.');
-    }
-    if (offset + length > code.length) {
-      throw RangeError('The edit extends past the end of the code.');
+
+    // If this edit overlaps or is not before the next (previous in this
+    // backwards loop) one in the sequence, fall back to sequential application.
+    if (i > 0 && offset + length >= edits[i - 1].offset) {
+      return edits.fold(code, (code, edit) => edit.apply(code));
     }
-    if (start > offset) {
-      // One of the edits overlaps with another, requiring that they be applied
-      // from largest offset to smallest. This should only be possible in code
-      // that creates source edits without using the `ChangeBuilder` to do so.
-      //
-      // We should consider fixing the places where overlapping edits are
-      // produced so that this branch can be removed. One such place is
-      // exhibited by `_DoCompletionTest.test_noBody`.
-      for (var edit in edits) {
-        code = edit.apply(code);
-      }
-      return code;
-    } else if (start < offset) {
+
+    validateEdit(code, edit);
+
+    if (start < offset) {
       buffer.write(code.substring(start, offset));
     }
     buffer.write(edit.replacement);
@@ -267,6 +264,24 @@ RefactoringOptions? refactoringOptionsFromJson(JsonDecoder jsonDecoder,
   return null;
 }
 
+/// Validates whether [edit] can be applied to [code].
+///
+/// Throws [RangeError] if the edit contains an invalid offset/length.
+void validateEdit(String code, SourceEdit edit) {
+  if (edit.offset < 0) {
+    throw RangeError('The edit offset is negative.');
+  }
+  if (edit.length < 0) {
+    throw RangeError('The edit length is negative.');
+  }
+  if (edit.offset > code.length) {
+    throw RangeError('The edit starts past the end of the code.');
+  }
+  if (edit.offset + edit.length > code.length) {
+    throw RangeError('The edit extends past the end of the code.');
+  }
+}
+
 /// Type of callbacks used to decode parts of JSON objects. [jsonPath] is a
 /// string describing the part of the JSON object being decoded, and [value] is
 /// the part to decode.