Revert "Support overlapping deletions in quick fixes"
This reverts commit ff34b811741db5ce5a55bb0ed00d6e026ef9e09b.
Reason for revert: This causes a failing test when trying to roll the engine into flutter/flutter. See https://github.com/flutter/flutter/issues/92181 for more context.
Original change's description:
> Support overlapping deletions in quick fixes
>
> I believe that the tests cover the added behavior (and some existing
> behavior), but they aren't complete. I'll add more tests over time, but
> for now the test coverage is strictly better than it used to be.
>
> Change-Id: I8dd228cb2b3c477c28e6d20c6da4a549a30d1afb
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/217280
> Reviewed-by: Phil Quitslund <pquitslund@google.com>
> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
> Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
# Not skipping CQ checks because original CL landed > 1 day ago.
Change-Id: Ib50418544a2fd30c255ffc118b42c45aded3ae1c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/217760
Reviewed-by: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
diff --git a/pkg/analyzer_plugin/lib/src/protocol/protocol_internal.dart b/pkg/analyzer_plugin/lib/src/protocol/protocol_internal.dart
index 459974c..777ccda 100644
--- a/pkg/analyzer_plugin/lib/src/protocol/protocol_internal.dart
+++ b/pkg/analyzer_plugin/lib/src/protocol/protocol_internal.dart
@@ -4,7 +4,6 @@
import 'dart:collection';
import 'dart:convert' hide JsonDecoder;
-import 'dart:math' as math;
import 'package:analyzer_plugin/protocol/protocol.dart';
import 'package:analyzer_plugin/protocol/protocol_common.dart';
@@ -29,17 +28,6 @@
/// If the invariants can't be preserved, then a [ConflictingEditException] is
/// thrown.
void addEditForSource(SourceFileEdit sourceFileEdit, SourceEdit sourceEdit) {
- /// If the [leftEdit] and the [rightEdit] can be merged, then merge them.
- SourceEdit? _merge(SourceEdit leftEdit, SourceEdit rightEdit) {
- assert(leftEdit.offset <= rightEdit.offset);
- if (leftEdit.isDeletion && rightEdit.isDeletion) {
- var offset = leftEdit.offset;
- var end = math.max(leftEdit.end, rightEdit.end);
- return SourceEdit(offset, end - offset, '');
- }
- return null;
- }
-
var edits = sourceFileEdit.edits;
var length = edits.length;
var index = 0;
@@ -51,12 +39,7 @@
// The [previousEdit] has an offset that is strictly greater than the offset
// of the [sourceEdit] so we only need to look at the end of the
// [sourceEdit] to know whether they overlap.
- if (sourceEdit.end > previousEdit.offset) {
- var mergedEdit = _merge(sourceEdit, previousEdit);
- if (mergedEdit != null) {
- edits[index - 1] = mergedEdit;
- return;
- }
+ if (sourceEdit.offset + sourceEdit.length > previousEdit.offset) {
throw ConflictingEditException(
newEdit: sourceEdit, existingEdit: previousEdit);
}
@@ -71,12 +54,7 @@
if ((sourceEdit.offset == nextEdit.offset &&
sourceEdit.length > 0 &&
nextEdit.length > 0) ||
- nextEdit.end > sourceEdit.offset) {
- var mergedEdit = _merge(nextEdit, sourceEdit);
- if (mergedEdit != null) {
- edits[index] = mergedEdit;
- return;
- }
+ nextEdit.offset + nextEdit.length > sourceEdit.offset) {
throw ConflictingEditException(
newEdit: sourceEdit, existingEdit: nextEdit);
}
@@ -490,11 +468,3 @@
/// the given [id], where the request was received at the given [requestTime].
Response toResponse(String id, int requestTime);
}
-
-extension SourceEditExtensions on SourceEdit {
- /// Return `true` if this source edit represents a deletion.
- bool get isDeletion => replacement.isEmpty;
-
- /// Return `true` if this source edit represents an insertion.
- bool get isInsertion => length == 0;
-}
diff --git a/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_core_test.dart b/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_core_test.dart
index 6ed22f9..a571f04 100644
--- a/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_core_test.dart
+++ b/pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_core_test.dart
@@ -17,7 +17,6 @@
defineReflectiveSuite(() {
defineReflectiveTests(ChangeBuilderImplTest);
defineReflectiveTests(EditBuilderImplTest);
- defineReflectiveTests(FileEditBuilderImpl_ConflictingTest);
defineReflectiveTests(FileEditBuilderImplTest);
defineReflectiveTests(LinkedEditBuilderImplTest);
});
@@ -298,33 +297,27 @@
}
}
-/// Tests that are specifically targeted at the handling of conflicting edits.
@reflectiveTest
-class FileEditBuilderImpl_ConflictingTest extends AbstractChangeBuilderTest {
+class FileEditBuilderImplTest extends AbstractChangeBuilderTest {
String path = '/test.dart';
- Matcher get hasConflict => throwsA(isA<ConflictingEditException>());
-
- Future<void> test_deletion_deletion_adjacent_left() async {
- var firstOffset = 30;
- var firstLength = 5;
- var secondOffset = 23;
- var secondLength = 7;
+ Future<void> test_addDeletion() async {
+ var offset = 23;
+ var length = 7;
await builder.addGenericFileEdit(path, (builder) {
- builder.addDeletion(SourceRange(firstOffset, firstLength));
- builder.addDeletion(SourceRange(secondOffset, secondLength));
+ builder.addDeletion(SourceRange(offset, length));
});
var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(2));
- expect(edits[0].offset, firstOffset);
- expect(edits[0].length, firstLength);
+ expect(edits, hasLength(1));
+ expect(edits[0].offset, offset);
+ expect(edits[0].length, length);
expect(edits[0].replacement, isEmpty);
- expect(edits[1].offset, secondOffset);
- expect(edits[1].length, secondLength);
- expect(edits[1].replacement, isEmpty);
}
- Future<void> test_deletion_deletion_adjacent_right() async {
+ Future<void> test_addDeletion_adjacent_lowerOffsetFirst() async {
+ // TODO(brianwilkerson) This should also merge the deletions, but is written
+ // to ensure that existing uses of FileEditBuilder continue to work even
+ // without that change.
var firstOffset = 23;
var firstLength = 7;
var secondOffset = 30;
@@ -343,23 +336,31 @@
expect(edits[1].replacement, isEmpty);
}
- Future<void> test_deletion_deletion_overlap_left() async {
- var firstOffset = 27;
- var firstLength = 8;
- var secondOffset = 23;
- var secondLength = 7;
+ Future<void> test_addDeletion_adjacent_lowerOffsetSecond() async {
+ // TODO(brianwilkerson) This should also merge the deletions, but is written
+ // to ensure that existing uses of FileEditBuilder continue to work even
+ // without that change.
+ var firstOffset = 23;
+ var firstLength = 7;
+ var secondOffset = 30;
+ var secondLength = 5;
await builder.addGenericFileEdit(path, (builder) {
- builder.addDeletion(SourceRange(firstOffset, firstLength));
builder.addDeletion(SourceRange(secondOffset, secondLength));
+ builder.addDeletion(SourceRange(firstOffset, firstLength));
});
var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(1));
+ expect(edits, hasLength(2));
expect(edits[0].offset, secondOffset);
- expect(edits[0].length, firstOffset + firstLength - secondOffset);
+ expect(edits[0].length, secondLength);
expect(edits[0].replacement, isEmpty);
+ expect(edits[1].offset, firstOffset);
+ expect(edits[1].length, firstLength);
+ expect(edits[1].replacement, isEmpty);
}
- Future<void> test_deletion_deletion_overlap_right() async {
+ @failingTest
+ Future<void> test_addDeletion_overlapping() async {
+ // This support is not yet implemented.
var firstOffset = 23;
var firstLength = 7;
var secondOffset = 27;
@@ -375,169 +376,6 @@
expect(edits[0].replacement, isEmpty);
}
- Future<void> test_deletion_insertion_adjacent_left() async {
- var deletionOffset = 23;
- var deletionLength = 7;
- var insertionOffset = 23;
- var insertionText = 'x';
- await builder.addGenericFileEdit(path, (builder) {
- builder.addDeletion(SourceRange(deletionOffset, deletionLength));
- expect(() {
- builder.addSimpleInsertion(insertionOffset, insertionText);
- }, hasConflict);
- });
- var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(1));
- expect(edits[0].offset, deletionOffset);
- expect(edits[0].length, deletionLength);
- expect(edits[0].replacement, '');
- }
-
- Future<void> test_deletion_insertion_adjacent_right() async {
- var deletionOffset = 23;
- var deletionLength = 7;
- var insertionOffset = 30;
- var insertionText = 'x';
- await builder.addGenericFileEdit(path, (builder) {
- builder.addDeletion(SourceRange(deletionOffset, deletionLength));
- builder.addSimpleInsertion(insertionOffset, insertionText);
- });
- var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(2));
- expect(edits[0].offset, insertionOffset);
- expect(edits[0].length, 0);
- expect(edits[0].replacement, insertionText);
- expect(edits[1].offset, deletionOffset);
- expect(edits[1].length, deletionLength);
- expect(edits[1].replacement, isEmpty);
- }
-
- Future<void> test_deletion_insertion_overlap() async {
- var deletionOffset = 23;
- var deletionLength = 7;
- var insertionOffset = 26;
- var insertionText = 'x';
- await builder.addGenericFileEdit(path, (builder) {
- builder.addDeletion(SourceRange(deletionOffset, deletionLength));
- expect(() {
- builder.addSimpleInsertion(insertionOffset, insertionText);
- }, hasConflict);
- });
- var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(1));
- expect(edits[0].offset, deletionOffset);
- expect(edits[0].length, deletionLength);
- expect(edits[0].replacement, '');
- }
-
- Future<void> test_insertion_deletion_adjacent_left() async {
- var deletionOffset = 23;
- var deletionLength = 7;
- var insertionOffset = 23;
- var insertionText = 'x';
- await builder.addGenericFileEdit(path, (builder) {
- builder.addSimpleInsertion(insertionOffset, insertionText);
- builder.addDeletion(SourceRange(deletionOffset, deletionLength));
- });
- var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(2));
- expect(edits[0].offset, deletionOffset);
- expect(edits[0].length, deletionLength);
- expect(edits[0].replacement, isEmpty);
- expect(edits[1].offset, insertionOffset);
- expect(edits[1].length, 0);
- expect(edits[1].replacement, insertionText);
- }
-
- Future<void> test_insertion_deletion_adjacent_right() async {
- var deletionOffset = 23;
- var deletionLength = 7;
- var insertionOffset = 30;
- var insertionText = 'x';
- await builder.addGenericFileEdit(path, (builder) {
- builder.addSimpleInsertion(insertionOffset, insertionText);
- builder.addDeletion(SourceRange(deletionOffset, deletionLength));
- });
- var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(2));
- expect(edits[0].offset, insertionOffset);
- expect(edits[0].length, 0);
- expect(edits[0].replacement, insertionText);
- expect(edits[1].offset, deletionOffset);
- expect(edits[1].length, deletionLength);
- expect(edits[1].replacement, isEmpty);
- }
-
- Future<void> test_insertion_deletion_overlap() async {
- var deletionOffset = 23;
- var deletionLength = 7;
- var insertionOffset = 26;
- var insertionText = 'x';
- await builder.addGenericFileEdit(path, (builder) {
- builder.addSimpleInsertion(insertionOffset, insertionText);
- expect(() {
- builder.addDeletion(SourceRange(deletionOffset, deletionLength));
- }, hasConflict);
- });
- var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(1));
- expect(edits[0].offset, insertionOffset);
- expect(edits[0].length, 0);
- expect(edits[0].replacement, insertionText);
- }
-
- Future<void> test_replacement_replacement_overlap_left() async {
- var offset = 23;
- var length = 7;
- var text = 'x';
- await builder.addGenericFileEdit(path, (builder) {
- builder.addSimpleReplacement(SourceRange(offset, length), text);
- expect(() {
- builder.addSimpleReplacement(SourceRange(offset - 2, length), text);
- }, hasConflict);
- });
- var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(1));
- expect(edits[0].offset, offset);
- expect(edits[0].length, length);
- expect(edits[0].replacement, text);
- }
-
- Future<void> test_replacement_replacement_overlap_right() async {
- var offset = 23;
- var length = 7;
- var text = 'x';
- await builder.addGenericFileEdit(path, (builder) {
- builder.addSimpleReplacement(SourceRange(offset, length), text);
- expect(() {
- builder.addSimpleReplacement(SourceRange(offset + 2, length), text);
- }, hasConflict);
- });
- var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(1));
- expect(edits[0].offset, offset);
- expect(edits[0].length, length);
- expect(edits[0].replacement, text);
- }
-}
-
-@reflectiveTest
-class FileEditBuilderImplTest extends AbstractChangeBuilderTest {
- String path = '/test.dart';
-
- Future<void> test_addDeletion() async {
- var offset = 23;
- var length = 7;
- await builder.addGenericFileEdit(path, (builder) {
- builder.addDeletion(SourceRange(offset, length));
- });
- var edits = builder.sourceChange.edits[0].edits;
- expect(edits, hasLength(1));
- expect(edits[0].offset, offset);
- expect(edits[0].length, length);
- expect(edits[0].replacement, isEmpty);
- }
-
Future<void> test_addInsertion() async {
await builder.addGenericFileEdit(path, (builder) {
builder.addInsertion(10, (builder) {
@@ -634,6 +472,40 @@
expect(edits[1].replacement, text);
}
+ Future<void> test_addSimpleReplacement_overlapsHead() async {
+ var offset = 23;
+ var length = 7;
+ var text = 'xyz';
+ await builder.addGenericFileEdit(path, (builder) {
+ builder.addSimpleReplacement(SourceRange(offset, length), text);
+ expect(() {
+ builder.addSimpleReplacement(SourceRange(offset - 2, length), text);
+ }, throwsA(isA<ConflictingEditException>()));
+ });
+ var edits = builder.sourceChange.edits[0].edits;
+ expect(edits, hasLength(1));
+ expect(edits[0].offset, offset);
+ expect(edits[0].length, length);
+ expect(edits[0].replacement, text);
+ }
+
+ Future<void> test_addSimpleReplacement_overlapsTail() async {
+ var offset = 23;
+ var length = 7;
+ var text = 'xyz';
+ await builder.addGenericFileEdit(path, (builder) {
+ builder.addSimpleReplacement(SourceRange(offset, length), text);
+ expect(() {
+ builder.addSimpleReplacement(SourceRange(offset + 2, length), text);
+ }, throwsA(isA<ConflictingEditException>()));
+ });
+ var edits = builder.sourceChange.edits[0].edits;
+ expect(edits, hasLength(1));
+ expect(edits[0].offset, offset);
+ expect(edits[0].length, length);
+ expect(edits[0].replacement, text);
+ }
+
Future<void> test_createEditBuilder() async {
await builder.addGenericFileEdit(path, (builder) {
var offset = 4;
