Throw an exception when an attempt is made to produce conflicting edits.

Change-Id: I826cd1771a61685f76314b3ec0125cfc0364b700
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/170800
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>

pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart

@@ -203,7 +203,24 @@ class StatementCompletionProcessor {
 
   void _addReplaceEdit(SourceRange range, String text) {
     var edit = SourceEdit(range.offset, range.length, text);
-    doSourceChange_addElementEdit(change, unitElement, edit);
+    // TODO(brianwilkerson) The commented out function call has been inlined in
+    //  order to work around a situation in which _complete_doStatement creates
+    //  a conflicting edit that happens to work because of the order in which
+    //  the edits are applied. The implementation needs to be cleaned up in
+    //  order to prevent the conflicting edit from being generated.
+    // doSourceChange_addElementEdit(change, unitElement, edit);
+    var fileEdit = change.getFileEdit(unitElement.source.fullName);
+    if (fileEdit == null) {
+      fileEdit = SourceFileEdit(file, 0);
+      change.addFileEdit(fileEdit);
+    }
+    var edits = fileEdit.edits;
+    var length = edits.length;
+    var index = 0;
+    while (index < length && edits[index].offset > edit.offset) {
+      index++;
+    }
+    edits.insert(index, edit);
   }
 
   void _appendEmptyBraces(SourceBuilder sb, [bool needsExitMark = false]) {

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

@@ -73,6 +73,7 @@ import 'package:analyzer/src/generated/java_core.dart';
 import 'package:analyzer_plugin/utilities/assist/assist.dart'
     hide AssistContributor;
 import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
+import 'package:analyzer_plugin/utilities/change_builder/conflicting_edit_exception.dart';
 
 /// The computer for Dart assists.
 class AssistProcessor extends BaseProcessor {
@@ -241,29 +242,34 @@ class AssistProcessor extends BaseProcessor {
     if (!setupSuccess) {
       return;
     }
+
+    Future<void> compute(CorrectionProducer producer) async {
+      producer.configure(context);
+      var builder = ChangeBuilder(
+          workspace: context.workspace, eol: context.utils.endOfLine);
+      try {
+        await producer.compute(builder);
+        _addAssistFromBuilder(builder, producer.assistKind,
+            args: producer.assistArguments);
+      } on ConflictingEditException {
+        // Handle the exception by not adding an assist based on the producer.
+        // TODO(brianwilkerson) Report the exception to the instrumentation
+        //  service so that we can fix the bug in the producer.
+      }
+    }
+
     for (var generator in generators) {
       var ruleNames = lintRuleMap[generator] ?? {};
       if (!_containsErrorCode(ruleNames)) {
         var producer = generator();
-        producer.configure(context);
-
-        var builder = ChangeBuilder(
-            workspace: context.workspace, eol: context.utils.endOfLine);
-        await producer.compute(builder);
-        _addAssistFromBuilder(builder, producer.assistKind,
-            args: producer.assistArguments);
+        await compute(producer);
       }
     }
     for (var multiGenerator in multiGenerators) {
       var multiProducer = multiGenerator();
       multiProducer.configure(context);
       for (var producer in multiProducer.producers) {
-        var builder = ChangeBuilder(
-            workspace: context.workspace, eol: context.utils.endOfLine);
-        producer.configure(context);
-        await producer.compute(builder);
-        _addAssistFromBuilder(builder, producer.assistKind,
-            args: producer.assistArguments);
+        await compute(producer);
       }
     }
   }

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

@@ -71,6 +71,7 @@ import 'package:analyzer/src/error/codes.dart';
 import 'package:analyzer/src/generated/engine.dart' show AnalysisEngine;
 import 'package:analyzer/src/generated/source.dart';
 import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
+import 'package:analyzer_plugin/utilities/change_builder/conflicting_edit_exception.dart';
 
 /// A fix producer that produces changes to fix multiple diagnostics.
 class BulkFixProcessor {
@@ -415,7 +416,11 @@ class BulkFixProcessor {
 
     Future<void> compute(CorrectionProducer producer) async {
       producer.configure(context);
-      await producer.compute(builder);
+      try {
+        await producer.compute(builder);
+      } on ConflictingEditException {
+        // TODO(brianwilkerson) Roll back the changes made by this producer.
+      }
     }
 
     var errorCode = diagnostic.errorCode;

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

@@ -160,6 +160,7 @@ import 'package:analyzer/src/generated/parser.dart';
 import 'package:analyzer_plugin/protocol/protocol_common.dart'
     hide AnalysisError, Element, ElementKind;
 import 'package:analyzer_plugin/utilities/change_builder/change_builder_core.dart';
+import 'package:analyzer_plugin/utilities/change_builder/conflicting_edit_exception.dart';
 import 'package:analyzer_plugin/utilities/fixes/fixes.dart' hide FixContributor;
 
 /// A function that can be executed to create a multi-correction producer.
@@ -1134,9 +1135,15 @@ class FixProcessor extends BaseProcessor {
       producer.configure(context);
       var builder = ChangeBuilder(
           workspace: context.workspace, eol: context.utils.endOfLine);
-      await producer.compute(builder);
-      _addFixFromBuilder(builder, producer.fixKind,
-          args: producer.fixArguments);
+      try {
+        await producer.compute(builder);
+        _addFixFromBuilder(builder, producer.fixKind,
+            args: producer.fixArguments);
+      } on ConflictingEditException {
+        // Handle the exception by not adding a fix based on the producer.
+        // TODO(brianwilkerson) Report the exception to the instrumentation
+        //  service so that we can fix the bug in the producer.
+      }
     }
 
     var errorCode = error.errorCode;

pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart

@@ -61,6 +61,8 @@ class StatementCompletionTest extends AbstractSingleUnitTest {
   }
 
   Future<void> _computeCompletion(int offset) async {
+    // TODO(brianwilkerson) I'm fairly confident that `result` is equivalent to
+    //  `testAnalysisResult` and that we're resolving the file twice.
     var result = await session.getResolvedUnit(testFile);
     var context = StatementCompletionContext(result, offset);
     var processor = StatementCompletionProcessor(context);

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

@@ -25,7 +25,7 @@ class ChangeTest {
   void test_addEdit() {
     var change = SourceChange('msg');
     var edit1 = SourceEdit(1, 2, 'a');
-    var edit2 = SourceEdit(1, 2, 'b');
+    var edit2 = SourceEdit(4, 2, 'b');
     expect(change.edits, hasLength(0));
     change.addEdit('/a.dart', 0, edit1);
     expect(change.edits, hasLength(1));

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

@@ -8,6 +8,7 @@ import 'dart:convert' hide JsonDecoder;
 import 'package:analyzer_plugin/protocol/protocol.dart';
 import 'package:analyzer_plugin/protocol/protocol_common.dart';
 import 'package:analyzer_plugin/protocol/protocol_generated.dart';
+import 'package:analyzer_plugin/utilities/change_builder/conflicting_edit_exception.dart';
 
 final Map<String, RefactoringKind> REQUEST_ID_REFACTORING_KINDS =
     HashMap<String, RefactoringKind>();
@@ -18,13 +19,46 @@ void addAllEditsForSource(
   edits.forEach(sourceFileEdit.add);
 }
 
-/// Adds the given [sourceEdit] to the list in [sourceFileEdit].
+/// Adds the given [sourceEdit] to the list in [sourceFileEdit] while preserving
+/// two invariants:
+/// - the list is sorted such that edits with a larger offset appear earlier in
+///   the list, and
+/// - no two edits in the list overlap each other.
+///
+/// If the invariants can't be preserved, then a [ConflictingEditException] is
+/// thrown.
 void addEditForSource(SourceFileEdit sourceFileEdit, SourceEdit sourceEdit) {
   var edits = sourceFileEdit.edits;
+  var length = edits.length;
   var index = 0;
-  while (index < edits.length && edits[index].offset > sourceEdit.offset) {
+  while (index < length && edits[index].offset > sourceEdit.offset) {
     index++;
   }
+  if (index > 0) {
+    var previousEdit = edits[index - 1];
+    // 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.offset + sourceEdit.length > previousEdit.offset) {
+      throw ConflictingEditException(
+          newEdit: sourceEdit, existingEdit: previousEdit);
+    }
+  }
+  if (index < length) {
+    var nextEdit = edits[index];
+    // The [nextEdit] has an offset that is less than or equal to the offset of
+    // the [sourceEdit]. If they're equal, then we consider it to be a conflict.
+    // Otherwise the offset of [nextEdit] is strictly less than the offset of
+    // the [sourceEdit] so we need to look at the end of the [nextEdit] to know
+    // whether they overlap.
+    if ((sourceEdit.offset == nextEdit.offset &&
+            sourceEdit.length > 0 &&
+            nextEdit.length > 0) ||
+        nextEdit.offset + nextEdit.length > sourceEdit.offset) {
+      throw ConflictingEditException(
+          newEdit: sourceEdit, existingEdit: nextEdit);
+    }
+  }
   edits.insert(index, sourceEdit);
 }
 

pkg/analyzer_plugin/lib/utilities/change_builder/conflicting_edit_exception.dart

@@ -0,0 +1,24 @@
+// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:analyzer_plugin/protocol/protocol_common.dart';
+import 'package:meta/meta.dart';
+
+/// An exception that is thrown when a change builder is asked to include an
+/// edit that conflicts with a previous edit.
+class ConflictingEditException implements Exception {
+  /// The new edit that was being added.
+  final SourceEdit newEdit;
+
+  /// The existing edit with which it conflicts.
+  final SourceEdit existingEdit;
+
+  /// Initialize a newly created exception indicating that the [newEdit].
+  ConflictingEditException(
+      {@required this.newEdit, @required this.existingEdit});
+
+  @override
+  String toString() =>
+      'ConflictingEditException: $newEdit conflicts with $existingEdit';
+}

pkg/analyzer_plugin/pubspec.yaml

@@ -10,13 +10,13 @@ environment:
 dependencies:
   analyzer: '>=0.39.12 <0.41.0'
   dart_style: '^1.2.0'
+  meta: ^1.2.3
   pub_semver: '^1.3.2'
 
 dev_dependencies:
   analysis_tool:
     path: ../analysis_tool
   html: '>=0.13.1 <0.15.0'
-  meta: '^1.1.8'
   path: '^1.4.1'
   test_reflective_loader: ^0.1.8
   test: ^1.0.0

pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_core_test.dart

@@ -6,6 +6,7 @@ import 'package:analyzer/src/generated/source.dart';
 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:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
@@ -259,6 +260,71 @@ class FileEditBuilderImplTest {
     expect(edits[0].replacement, isEmpty);
   }
 
+  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;
+    var secondLength = 5;
+    var builder = ChangeBuilderImpl();
+    await builder.addGenericFileEdit(path, (builder) {
+      builder.addDeletion(SourceRange(firstOffset, firstLength));
+      builder.addDeletion(SourceRange(secondOffset, secondLength));
+    });
+    var edits = builder.sourceChange.edits[0].edits;
+    expect(edits, hasLength(2));
+    expect(edits[0].offset, 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_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;
+    var builder = ChangeBuilderImpl();
+    await builder.addGenericFileEdit(path, (builder) {
+      builder.addDeletion(SourceRange(secondOffset, secondLength));
+      builder.addDeletion(SourceRange(firstOffset, firstLength));
+    });
+    var edits = builder.sourceChange.edits[0].edits;
+    expect(edits, hasLength(2));
+    expect(edits[0].offset, 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);
+  }
+
+  @failingTest
+  Future<void> test_addDeletion_overlapping() async {
+    // This support is not yet implemented.
+    var firstOffset = 23;
+    var firstLength = 7;
+    var secondOffset = 27;
+    var secondLength = 8;
+    var builder = ChangeBuilderImpl();
+    await builder.addGenericFileEdit(path, (builder) {
+      builder.addDeletion(SourceRange(firstOffset, firstLength));
+      builder.addDeletion(SourceRange(secondOffset, secondLength));
+    });
+    var edits = builder.sourceChange.edits[0].edits;
+    expect(edits, hasLength(1));
+    expect(edits[0].offset, firstOffset);
+    expect(edits[0].length, secondOffset + secondLength - firstOffset);
+    expect(edits[0].replacement, isEmpty);
+  }
+
   Future<void> test_addInsertion() async {
     var builder = ChangeBuilderImpl();
     await builder.addGenericFileEdit(path, (builder) {
@@ -307,6 +373,24 @@ class FileEditBuilderImplTest {
     expect(edits[0].replacement, text);
   }
 
+  Future<void> test_addSimpleInsertion_sameOffset() async {
+    var offset = 23;
+    var text = 'xyz';
+    var builder = ChangeBuilderImpl();
+    await builder.addGenericFileEdit(path, (builder) {
+      builder.addSimpleInsertion(offset, text);
+      builder.addSimpleInsertion(offset, 'abc');
+    });
+    var edits = builder.sourceChange.edits[0].edits;
+    expect(edits, hasLength(2));
+    expect(edits[0].offset, offset);
+    expect(edits[0].length, 0);
+    expect(edits[0].replacement, 'abc');
+    expect(edits[1].offset, offset);
+    expect(edits[1].length, 0);
+    expect(edits[1].replacement, text);
+  }
+
   Future<void> test_addSimpleReplacement() async {
     var offset = 23;
     var length = 7;
@@ -322,6 +406,64 @@ class FileEditBuilderImplTest {
     expect(edits[0].replacement, text);
   }
 
+  Future<void> test_addSimpleReplacement_adjacent() async {
+    var firstOffset = 23;
+    var firstLength = 7;
+    var secondOffset = firstOffset + firstLength;
+    var secondLength = 5;
+    var text = 'xyz';
+    var builder = ChangeBuilderImpl();
+    await builder.addGenericFileEdit(path, (builder) {
+      builder.addSimpleReplacement(SourceRange(firstOffset, firstLength), text);
+      builder.addSimpleReplacement(
+          SourceRange(secondOffset, secondLength), text);
+    });
+    var edits = builder.sourceChange.edits[0].edits;
+    expect(edits, hasLength(2));
+    expect(edits[0].offset, secondOffset);
+    expect(edits[0].length, secondLength);
+    expect(edits[0].replacement, text);
+    expect(edits[1].offset, firstOffset);
+    expect(edits[1].length, firstLength);
+    expect(edits[1].replacement, text);
+  }
+
+  Future<void> test_addSimpleReplacement_overlapsHead() async {
+    var offset = 23;
+    var length = 7;
+    var text = 'xyz';
+    var builder = ChangeBuilderImpl();
+    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';
+    var builder = ChangeBuilderImpl();
+    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 {
     var builder = ChangeBuilderImpl();
     await builder.addGenericFileEdit(path, (builder) {