`dart fix` produces invalid code with overlapping `add_override` and `add_return_type`

[oprypin]

How to observe the problem:

$ cat analysis_options.yaml
linter:
  rules:
    - always_declare_return_types
    - annotate_overrides
 
$ cat lib/example_lib.dart 
class A {
  foo() {
    return 5;
  }
}  
 
class B extends A {
  foo() {
    return 6;
  }
}
 
void main() {
}
 
$ dart fix --apply lib/example_lib.dart
 
example_lib.dart
  always_declare_return_types • 3 fixes
  annotate_overrides • 1 fix
 
4 fixes made in 1 file.
 
$ cat lib/example_lib.dart
class A {
  int foo() {
    return 5;
  }
}  
 
class B extends A {
  int @override
  int foo() {
    return 6;
  }
}
 
void main() {
}

The problem is that foo() {

• got turned into

  int @override
  foo() {
  

• but should've been

  @override
  int foo() {
  

And then due to the repeated re-run behavior of dart fix, yet another int got inserted because the previous one wasn't acknowledged.

I showed how this can be tried out with dart fix, but the problem is not specific to it, the bug already applies to the edit.bulkFixes handler itself.

The way this manifests is that the handler returns both of these fixes at the same offset, in this order:

• offset 123, length 0, replacement '@override\n '
• offset 123, length 0, replacement 'int '

And they get applied as is in this order.

Dart SDK version: 2.19.0-361.0.dev and also latest main

Here are some pieces of code that I suspect. I tried to fix this by changing these, but eventually gave up.

• Maybe changing the priority would be part of the fix?
  

  sdk/pkg/analysis_server/lib/src/services/correction/fix.dart

  Lines 231 to 235 in ea9b19c

  	static const ADD_OVERRIDE_MULTI = FixKind(
  	'dart.fix.add.override.multi',
  	DartFixKindPriority.IN_FILE,
  	"Add '@override' annotations everywhere in file",
  	);

• Maybe switching to stable sort would be part of the fix?
  

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

  Line 290 in ea9b19c

  errors.sort((a, b) => a.offset.compareTo(b.offset));

• Maybe it's something about how 0-length edits are special-cased?
  

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

  Lines 48 to 54 in ea9b19c

  	// If it's an insert and it should be inserted before existing edits, also
  	// skip over any with the same offset.
  	if (insertBeforeExisting && sourceEdit.length == 0) {
  	while (index < length && edits[index].offset >= sourceEdit.offset) {
  	index++;
  	}
  	}

[bwilkerson]

Unfortunately, I think the issue is more fundamental than that. The correction producers are all independent of each other, and when running a bulk fix there's no way for one producer to know what other changes have been made (and I wouldn't want the combinatorial explosion of checking that would need to be added to each producer in order to make that approach work). The same problem can happen with any pair of edits that apply to the same region of code.

I think the only reasonable solution here is to expand the notion of "overlapping" to include any pair of insertions at the same offset, or any insertion and replacement at the same offset. (We already handle the case of a pair of replacements where the offset of one is within the range of the other.)

IIRC, the current definition was implemented before we supported the current multi-pass approach, so the consequence was too severe. But I suspect that the behavior of the system would now be reasonable even with the proposed restriction.