dart-lang/sdk
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Fixes the handling of linked edit groups in ChangeBuilder

Browse Source 
NOTE: Amost all of this code was written by AI, but has all been
reviewed by me. I did a tiny amount of cleanup that was faster to do
myself than to ask the agent to do it. That doesn't mean that I've
caught all of the AI's bugs, but I did try to ensure that test coverage
was complete enough to also catch problems.

This fixes a bug in the way the `ChangeBuilder` handled linked edit
groups. The bug resulted from the fact that the offsets of the linked
edit groups was adjusted at the time each edit location was added.
Doing that meant that they weren't re-adjusted when new non-group edits
were added at a lower offset, making it possible for the edits to have
the wrong offsets when they were sent to the client.

The solution is to not adjust them when they are created, but to
adjust them during `finalize` when other offsets are adjusted. At that
point we have full knowledge of all of the edits so we can get it right.

Doing that required adding a reference to the each edit in the file
edit builder (because only the edits in the same file can require that
an adjustment be made).

My hope is that with this change in place I can complete the arc of
work to update all of our refactors to use the `ChangeBuilder` APIs to
build the edits.

Change-Id: I59ddb0dee1f9dbb15bdcc38a84ddc07acefe6262
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/507622
Reviewed-by: Samuel Rawlins <srawlins@google.com>
This commit is contained in:
Brian Wilkerson
2026-05-29 17:04:24 -07:00
parent 8a5987013f
commit 4af407d11e
3 changed files with 177 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_core.dart
+63 -21
View File
@@ -460,22 +460,13 @@ class ChangeBuilderImpl implements ChangeBuilder {
required int offset,
required int delta,
}) {
void updatePosition(Position position) {
if (position.file == filePath &&
position.offset >= offset &&
!_lockedPositions.contains(position)) {
position.offset = position.offset + delta;
}
}
for (var group in _linkedEditGroups.values) {
for (var position in group.positions) {
updatePosition(position);
}
}
var selection = _selection;
if (selection != null) {
updatePosition(selection);
if (selection.file == filePath &&
selection.offset >= offset &&
!_lockedPositions.contains(selection)) {
selection.offset = selection.offset + delta;
}
}
// TODO(brianwilkerson): If the selection range is not in the file at the
// [filePath], it will be updated when it shouldn't be. This appears to not
@@ -560,7 +551,13 @@ class EditBuilderImpl implements EditBuilder {
if (length != 0) {
var position = Position(
fileEditBuilder.fileEdit.file,
start + fileEditBuilder._deltaToOffset(start),
start);
fileEditBuilder._pendingPositions.add(
_PendingPosition(
position: position,
editBuilder: this,
offset: start - offset,
),
);
fileEditBuilder.changeBuilder._lockedPositions.add(position);
var group = fileEditBuilder.changeBuilder.getLinkedEditGroup(groupName);
@@ -652,6 +649,13 @@ class FileEditBuilderImpl implements FileEditBuilder {
final _FileEditBuilderRevertData _revertData = _FileEditBuilderRevertData();
/// The list of pending linked positions for this file whose final offsets
/// have not yet been computed.
final List<_PendingPosition> _pendingPositions = [];
/// A map from edit builders to the actual source edits they created.
final Map<EditBuilderImpl, SourceEdit> _builderToEdit = {};
/// Initialize a newly created builder to build a source file edit within the
/// change being built by the given [changeBuilder]. The file being edited has
/// the given absolute [path] and [timeStamp].
@@ -692,13 +696,16 @@ class FileEditBuilderImpl implements FileEditBuilder {
var group = changeBuilder.getLinkedEditGroup(groupName);
var position = Position(
fileEdit.file,
range.offset + _deltaToOffset(range.offset),
range.offset,
);
group.addPosition(position, range.length);
var revertData = changeBuilder._revertData;
revertData._addedLinkedEditGroupPositions
.putIfAbsent(group, () => [])
.add(position);
_pendingPositions.add(
_PendingPosition(position: position, offset: range.offset),
);
}
@override
@@ -752,7 +759,19 @@ class FileEditBuilderImpl implements FileEditBuilder {
/// Finalize the source file edit that is being built.
void finalize() {
// Nothing to do.
for (var pending in _pendingPositions) {
var builder = pending.editBuilder;
if (builder != null) {
var edit = _builderToEdit[builder];
if (edit != null) {
pending.position.offset =
edit.offset + _deltaToEdit(edit) + pending.offset;
}
} else {
pending.position.offset =
pending.offset + _deltaToOffset(pending.offset);
}
}
}
/// Replace edits in the [range] with the given [edit].
@@ -814,6 +833,7 @@ class FileEditBuilderImpl implements FileEditBuilder {
bool insertBeforeExisting = false,
}) {
var edit = builder.sourceEdit;
_builderToEdit[builder] = edit;
_addEdit(edit, insertBeforeExisting: insertBeforeExisting);
_captureSelection(builder, edit);
}
@@ -835,11 +855,12 @@ class FileEditBuilderImpl implements FileEditBuilder {
/// edit before the applied edits will be at `offset + _deltaToOffset(offset)`
/// after the edits.
int _deltaToEdit(SourceEdit targetEdit) {
var targetIndex = fileEdit.edits.indexOf(targetEdit);
assert(targetIndex != -1);
var delta = 0;
for (var edit in fileEdit.edits) {
if (edit.offset < targetEdit.offset) {
delta += _editDelta(edit);
}
for (var i = targetIndex + 1; i < fileEdit.edits.length; i++) {
delta += _editDelta(fileEdit.edits[i]);
}
return delta;
}
@@ -963,6 +984,27 @@ class _FileEditBuilderRevertData {
final Set<SourceEdit> _addedEdits = HashSet.identity();
}
/// A representation of a linked position whose final offset has not yet been
/// computed.
class _PendingPosition {
/// The position object whose offset will be updated.
final Position position;
/// The edit builder inside which this position was created, or `null` if it
/// is a standalone linked position.
final EditBuilderImpl? editBuilder;
/// The original offset of the position in the file, or the offset of the
/// position within the replacement text of the [editBuilder].
final int offset;
_PendingPosition({
required this.position,
this.editBuilder,
required this.offset,
});
}
/// Workspace that wraps a single [AnalysisSession].
class _SingleSessionWorkspace extends ChangeWorkspace {
final AnalysisSession session;
pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
+1
View File
@@ -1949,6 +1949,7 @@ class DartFileEditBuilderImpl extends FileEditBuilderImpl
builder.writeln(header);
});
}
super.finalize();
}
@override
pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_core_test.dart
+113 -2
View File
@@ -319,6 +319,115 @@ class EditBuilderImplTest extends AbstractChangeBuilderTest {
expect(positions[0].offset, 19);
}
Future<void> test_addLinkedEdit_editBeforeGroup() async {
await builder.addGenericFileEdit(path, (builder) {
// 1. Add linked edit at larger offset
builder.addInsertion(10, (builder) {
builder.addSimpleLinkedEdit('group_name', 'content'); // length 7
});
// 2. Add edit at smaller offset (before the group)
builder.addSimpleInsertion(5, '12345'); // length 5, delta +5
// 3. Add edit at larger offset
builder.addSimpleInsertion(12, 'abc'); // length 3, delta +3
});
var sourceChange = builder.sourceChange;
var groups = sourceChange.linkedEditGroups;
expect(groups, hasLength(1));
var positions = groups[0].positions;
expect(positions, hasLength(1));
// Original linked edit is at 10.
// Edit at 5 (before 10) shifts it to 15.
// Edit at 12 (after 10) should NOT shift it.
// So the expected final offset is 15.
expect(positions[0].offset, 15);
}
Future<void> test_addLinkedEdit_multipleFiles_unordered() async {
var file1 = '/test1.txt';
var file2 = '/test2.txt';
// 1. Add linked edit in file2 at larger offset (20)
await builder.addGenericFileEdit(file2, (builder) {
builder.addInsertion(20, (builder) {
builder.addSimpleLinkedEdit('group_name', 'content');
});
});
// 2. Add linked edit in file1 at larger offset (10)
await builder.addGenericFileEdit(file1, (builder) {
builder.addInsertion(10, (builder) {
builder.addSimpleLinkedEdit('group_name', 'content');
});
});
// 3. Add edit in file2 at smaller offset (5) - after the linked edit at 20
await builder.addGenericFileEdit(file2, (builder) {
builder.addSimpleInsertion(5, '12345'); // delta +5
});
// 4. Add edit in file1 at smaller offset (2) - after the linked edit at 10
await builder.addGenericFileEdit(file1, (builder) {
builder.addSimpleInsertion(2, '123'); // delta +3
});
// 5. Add subsequent edits at larger offsets to trigger incorrect shifts
await builder.addGenericFileEdit(file2, (builder) {
builder.addSimpleInsertion(
22,
'abc',
); // delta +3 (originally after 20, but before shifted 25)
});
await builder.addGenericFileEdit(file1, (builder) {
builder.addSimpleInsertion(
11,
'xyz',
); // delta +3 (originally after 10, but before shifted 13)
});
var sourceChange = builder.sourceChange;
var groups = sourceChange.linkedEditGroups;
expect(groups, hasLength(1));
var positions = groups[0].positions;
expect(positions, hasLength(2));
for (var position in positions) {
if (position.file == file1) {
// Original 10. Shifted by edit at 2 (+3). Expected 13.
expect(position.offset, 13);
} else if (position.file == file2) {
// Original 20. Shifted by edit at 5 (+5). Expected 25.
expect(position.offset, 25);
}
}
}
Future<void> test_addLinkedEdit_multipleInsertions() async {
await builder.addGenericFileEdit(path, (builder) {
// Add first insertion with a linked edit
builder.addInsertion(5, (builder) {
builder.addSimpleLinkedEdit('group_a', 'first');
});
// Add second insertion with a linked edit
builder.addInsertion(15, (builder) {
builder.addSimpleLinkedEdit('group_b', 'second');
});
});
var sourceChange = builder.sourceChange;
var groups = sourceChange.linkedEditGroups;
expect(groups, hasLength(2));
var groupA = groups.firstWhere((g) => g.positions[0].offset < 10);
var groupB = groups.firstWhere((g) => g.positions[0].offset > 10);
expect(groupA.positions[0].offset, 5);
// First insertion at 5 inserts 'first' (length 5).
// Second insertion at 15 is shifted by 5.
// So expected offset of second insertion is 15 + 5 = 20.
expect(groupB.positions[0].offset, 20);
}
Future<void> test_addSimpleLinkedEdit() async {
var offset = 10;
var text = 'content';
@@ -533,7 +642,8 @@ class FileEditBuilderImplTest extends AbstractChangeBuilderTest {
builder.addInsertion(0, (builder) => builder.write('// ${'a' * 46}\n'));
});
var group = builder.getLinkedEditGroup(groupName);
var sourceChange = builder.sourceChange;
var group = sourceChange.linkedEditGroups[0];
var positions = group.positions;
expect(positions, hasLength(1));
var position = positions[0];
@@ -549,7 +659,8 @@ class FileEditBuilderImplTest extends AbstractChangeBuilderTest {
builder.addLinkedPosition(SourceRange(3, 6), groupName);
});
var group = builder.getLinkedEditGroup(groupName);
var sourceChange = builder.sourceChange;
var group = sourceChange.linkedEditGroups[0];
var positions = group.positions;
expect(positions, hasLength(1));
var position = positions[0];