dart fix applies fixes in part files mutliple times

[denniskaselow]

Dart SDK version: 3.7.0-157.0.dev (dev) (Sat Nov 16 12:03:16 2024 -0800) on "windows_x64"

Running dart fix --apply on this code will apply the fixes in b.dart multiple times:

// a.dart
part 'b.dart';

void a() {
  // need to trigger a lint in a.dart for the bug to happen
  ;
  b();
}

// b.dart
part of 'a.dart';

Stream<String> b() {
  // dart fix should only add a single const
  return Stream.empty();
}

# analysis_options.yaml
linter:
  rules:
    - empty_statements
    - prefer_const_constructors

The fix adds const 3 times in b.dart:

part of 'a.dart';

Stream<String> b() {
  // dart fix should only add a single const
  return const const const Stream.empty();
}

Output of dart fix --apply:

Applying fixes...

lib\a.dart
  empty_statements • 1 fix

lib\b.dart
  prefer_const_constructors • 3 fixes

It's not limited to these lints. For example the fix for require_trailing_commas would add multiple commas.

[pq]

/fyi @bwilkerson

[bwilkerson]

Is this a recent regression?

[benthillerkus]

from the issue i had opened today:

• This bug is now in the latest Flutter release
• Repro https://github.com/benthillerkus/dart_fix_repro

[pq]

Working on reproducing this in https://dart-review.googlesource.com/c/sdk/+/401180, I'm not seeing it at the bulk_fix level in the server but it does reproduce in the fix command. My thinking is there's something funny going on in how we're detecting when another fix iteration is required?

/fyi @bwilkerson

[pq]

Hmmmm, or maybe not quite. Reducing maxPasses to 1, I still see the fix doubly-applied:

00:00 +0 ~13 -1: fix perform --apply --code=(multiple) [part file] [E]

  Expected: (a string containing 'Applying fixes...', 'lib/main.dart', '  empty_statements • 1 fix', 'lib/part.dart', '  prefer_const_constructors • 1 fix', '2 fixes made in 2 files.' in order or a string containing 'Applying fixes...', 'lib/main.dart', '  empty_statements - 1 fix', 'lib/part.dart', '  prefer_const_constructors - 1 fix', '2 fixes made in 2 files.' in order)
    Actual: 'Computing fixes in myapp...\n'
              'Applying fixes...\n'
              '\n'
              'lib/main.dart\n'
              '  empty_statements - 1 fix\n'
              '\n'
              'lib/part.dart\n'
              '  prefer_const_constructors - 2 fixes\n'
              '\n'
              '3 fixes made in 2 files.\n'
              ''

Interestingly, with the default of 4 maxPasses we're only seeing one more over-application:

00:01 +0 ~13 -1: fix perform --apply --code=(multiple) [part file] [E]

  Expected: (a string containing 'Applying fixes...', 'lib/main.dart', '  empty_statements • 1 fix', 'lib/part.dart', '  prefer_const_constructors • 1 fix', '2 fixes made in 2 files.' in order or a string containing 'Applying fixes...', 'lib/main.dart', '  empty_statements - 1 fix', 'lib/part.dart', '  prefer_const_constructors - 1 fix', '2 fixes made in 2 files.' in order)
    Actual: 'Computing fixes in myapp...\n'
              'Applying fixes...\n'
              '\n'
              'lib/main.dart\n'
              '  empty_statements - 1 fix\n'
              '\n'
              'lib/part.dart\n'
              '  prefer_const_constructors - 3 fixes\n'
              '\n'
              '4 fixes made in 2 files.\n'
              ''

[FMorschel]

My guess (I've not looked into anything) is that this has something to do with the new element model/enhanced parts changes. Maybe some of the new logic with that was already on that version for the analysis done here?

[pq]

@FMorschel: totally and that's my thinking too but I haven't found what level the problem is occurring in. (Unfortunately, the only place I have it reproduced, in the dart fix tests is hard to debug since the tests spawn a process and can't just be stepped through.) I'll keep digging though. Thanks!

[pq]

UPDATE: will return to this tomorrow but it may take some bissecting.

In particular, I'll be curious to see if the current error reproduces before 73adec7 (which does the initial migration) and is in 3.7.0-157.0.dev (where the OP initially reported the issue).

[FMorschel]

@pq an update for you: I've tested downloading the SDK from https://gsdview.appspot.com/dart-archive/channels/main/raw/hash/5c904c17f79f2dc2374b4619dad354bd2a293ea0/sdk/ (parent commit from the one you mentioned). The error is already there.

[FMorschel]

After some testing, still no results but I have a starting theory. When it does the first analysis for the part file, I think it is counting:

• The fix for main + the fix for the const - 2 errors - (tested this theory by adding one more ; to main which ends in 5 errors because it'll add three const to part that will stop the error from showing)
• When you run the next pass, it finds only 1 fix for the missing const (see the screenshots below).

I'm unsure why would it do such a thing, but it's a theory. I think something is not working right for calculating the fixes for the part file.

[denniskaselow]

In particular, I'll be curious to see if the current error reproduces before 73adec7 (which does the initial migration) and is in 3.7.0-157.0.dev (where the OP initially reported the issue).

Just to make sure the duplicate issue didn't get drowned out, but the bug already exists in the currently released stable version of Dart 3.6.0.

And as a further data point (and maybe more head scratching), if you introduce lints for annotate_overrides and require_trailing_commas, those "only" get applied twice.

Additionally, if you modify a.dart so that instead of

;
b();`

you write (error instead of lint)

idontexist();

the fix for prefer_const_constructors is applied 4 times, but it'll introduce an extra fix for duplicated_modifier still ending up with 3 const (annotate_overrides and require_trailing_commas still only get applied twice).

Applying fixes...

lib\b.dart
  annotate_overrides • 2 fixes
  duplicated_modifier • 1 fix
  prefer_const_constructors • 4 fixes
  require_trailing_commas • 2 fixes

9 fixes made in 1 file.

Further testing also shows, that if you switch the content of a.dart and b.dart the fixes will still be applied multiple times, so it's not limited to part files. But both files need to be there (and it's really weird what triggers the bug and what doesn't)

For example:

// a.dart
part 'b.dart';

Stream<String> b() {
  // dart fix should only add a single const
  return Stream.empty();
}

// b.dart
part of 'a.dart';

// triggers the 4 const + 1 duplicated_modifier fix
idontexist
// triggers the 3 const fix
void a {
  ;
}
// these don't trigger the bug
idontexist;
idontexist()
idontexist();

[FMorschel]

if you introduce lints for annotate_overrides and require_trailing_commas, those "only" get applied twice.

That is because when you have them applied, they don't re-trigger the diagnostic (see the screenshots in my comment above).

---

it'll introduce an extra fix for duplicated_modifier

I've never heard of it before and I couldn't find any docs besides the one for Rust. But the weird thing is that "fixing" it does nothing (because it is not there and should not be showing up so maybe a separate issue).

---

if you switch the content of a.dart and b.dart the fixes will still be applied multiple times

You may be onto something here. Probably what I suggested for the diagnostic count being off.

---

Edit

@pq commented

and is in 3.7.0-157.0.dev (where the OP initially reported the issue).

@denniskaselow

Just to make sure the duplicate issue didn't get drowned out, but the bug already exists in the currently released stable version of Dart 3.6.0.

Yeah, I'm not sure where the cut is for the 3.6 stable version (which commit) but I don't think it is on 3.7.0...dev. The suggested commit above may have nothing to do (as shown by my testing) with this issue. Maybe something farther here, @pq.

[denniskaselow]

I've never heard of it before and I couldn't find any docs besides the one for Rust. But the weird thing is that "fixing" it does nothing (because it is not there and should not be showing up so maybe a separate issue).

It doesn't exist as a lint, but it exists:

sdk/pkg/front_end/messages.yaml

Lines 1259 to 1261 in e8dafed

	problemMessage: "The modifier '#lexeme' was already specified."
	correctionMessage: "Try removing all but one occurrence of the modifier."
	analyzerCode: ParserErrorCode.DUPLICATED_MODIFIER

And it does remove one const if you have at least 4 in this case (maybe the other errors are more important/triggered first so it doesn't show up sooner).

[FMorschel]

I see, in this case, we may have a bug in the parser for this with only two const there. It should be able to detect it and trigger it.

Also, we might need to add docs to duplicated_modifier as well. I'll open an issue for it. Thanks!

---

Edit

Here it is #59738, @denniskaselow.

[FMorschel]

@denniskaselow for the duplicated_modifier trigger, I think I figured it out.

Since it now still has an error on main.dart, it'll look for all the errors in the library (any or both files) and show this fix (for duplicated_modifier) should happen. If you only leave the ones below a the same behaviour happens. But more errors won't do anything now.

[pq]

Thanks all for the detective work!

[FMorschel]

I think I figured it out @pq. Asked on Discord for more help on how to debug, but the problem is in one of the tests you made on the CL.

Commenting here too for future reference:

In pkg\analysis_server\test\src\services\correction\fix\bulk_fix_processor_test.dart you added HasFixesTest.test_hasFixes_in_part_and_library2. But the problem is: that if you run dart fix for each file by itself, you'll see the expected number of fixes. But dart fix runs for both the main file and part file, the request for bulk fixes. And since the main file already returned the errors for the part file too, they get duplicated. One or the other should not be happening.

I'd expect that when you resolve for test code (on the mentioned test) you only get the results for that file, and not part too. Or if this returned fixes for two files, for dart fix to ignore all others and only consider the ones for the requested path. I can be wrong but that seems to be the problem at least.

[pq]

Thanks for looking!

With this clue I was able to get the issue reproduced in BulkFixesTest.

(It turns out, I wasn't looking deeply enough into the edit results.)

These new expectations do it:

    var result = await sendEditBulkFixes([sourceDirectory.path]);
    var edits = result.edits;
    expect(edits, hasLength(2));
    expect(edits[0].edits, hasLength(1));
    expect(edits[1].edits, hasLength(1));

00:02 +1 -1: BulkFixesTest | test_bulk_fix_with_parts [E]

  Expected: an object with length of <1>
    Actual: [
              SourceEdit:{"offset":110,"length":0,"replacement":"const ","description":"Add 'const' modifier"},
              SourceEdit:{"offset":110,"length":0,"replacement":"const ","description":"Add 'const' modifier"}
            ]
     Which: has length of <2>

This will be far easier to debug from.

Thanks!

[FMorschel]

@pq, I'm looking at this as well, but feel free to do so, you're probably more familiar with all of this than me. I've pinged you on a Discord thread related to this.

Hopefully, we can find a good solution soon.

[FMorschel]

@pq I think I've made a small change that fixes it. Inside BulkFixProcessor._fixErrorsInLibrary if you pass it the current path and only fix the error for the unitResult that matches it.

  /// Uses the change [builder] to create fixes for the diagnostics in the
  /// library associated with the analysis [libraryResult].
  Future<void> _fixErrorsInLibraryAt(
    ResolvedLibraryResult libraryResult, {
    required String path,
    bool stopAfterFirst = false,
    bool autoTriggered = false,
  }) async {
    for (var unitResult in libraryResult.units) {
      if (unitResult.path != path) {
        continue;
      }
      await _fixErrorsInLibraryUnit(
        libraryResult,
        unitResult,
        stopAfterFirst: stopAfterFirst,
        autoTriggered: autoTriggered,
      );
    }
  }

[FMorschel]

Not quite, under bulk_fix_processor_test.dart I broke the test for #55520 but I don't think the current test reflects what we expected to fix in that issue.

Do you want me to make a CL myself or do you want to complete this, @pq?

---

Fyi @bwilkerson I think this issue mentioned above (#55520) is the one that created this issue so about 6 months old.

[pq]

Ah! This is fantastic. I'm more than happy to let you land your fix, @FMorschel. Thanks for all of your help!

[pq]

@FMorschel feel free to modify the new tests in 6456180 as suits you. I hope they're useful.

[FMorschel]

@pq can I get a review for https://dart-review.googlesource.com/c/sdk/+/401865? Thanks!

[FMorschel]

For everyone following this issue, we will open a cherry-pick (CP) issue for this change. Once we have the new issue I'll post the link here too.

Thanks, everyone for filing the issues and helping us find the source of the problem!

[FMorschel]

Here is the CP link #59853

[FMorschel]

Dart 3.6.1 is out with a fix!

[benthillerkus]

Unfortunately it's still broken for trailing commas

❯ flutter --version
Flutter 3.27.2 • channel stable • https://github.com/flutter/flutter.git
Framework • revision 68415ad1d9 (2 days ago) • 2025-01-13 10:22:03 -0800
Engine • revision e672b006cb
Tools • Dart 3.6.1 • DevTools 2.40.2

// from our CI job

Run flutter --version
Flutter 3.2[7](https://github.com/RevotionGmbH/revotion/actions/runs/12792187402/job/35662039406#step:4:8).2 • channel stable • https://github.com/flutter/flutter.git
Framework • revision 6[8](https://github.com/RevotionGmbH/revotion/actions/runs/12792187402/job/35662039406#step:4:9)415ad1d9 (2 days ago) • 2025-01-13 10:22:03 -0800
Engine • revision e672b006cb
Tools • Dart 3.6.1 • DevTools 2.40.2

ERROR: Could not format because the source could not be parsed:
line 32, column 19 of apps/app/lib/shared/models/nodes/node_data/node_data_ambient.dart: Expected an identifier.
   ╷
32 │       timer: null,,);
   │                   ^
   ╵
line 49, column 18 of apps/app/lib/shared/models/nodes/node_data/node_data_ambient.dart: Expected an identifier.
   ╷
49 │       this.timer,,});
   │                  ^
   ╵
Formatted 467 files (10 changed) in 2.94 seconds.

[benthillerkus]

Wait, are you sure this shipped in 3.6.1?
Running dart fix --apply on https://github.com/benthillerkus/dart_fix_repro looks like this

[FMorschel]

It should @benthillerkus. It is the last point in the changelog https://github.com/dart-lang/sdk/blob/stable/CHANGELOG.md

I'll take a look at your repro and see what I can find.

[FMorschel]

@benthillerkus I've tested it locally and I do see that this seems to have not been fixed.

I'm asking at the other issue (the one for CP) for what happened here. If you want to follow this go there and subscribe to it.

Sorry for the false alarm everyone. I'll update here again only when I see the fix landed and tested by me (with the stable build).

[FMorschel]

Alright, everyone. I've just tested the changes at 42f3fc1c648bc66e56c822a95e6139bb116020c3 which is the commit used to build the latest (3.6.2) version of Dart (see here https://dart.dev/get-dart/archive) and it is working now!

The change I made to stable was pending for too long and got merged just after the 3.6.1 commit for building the SDK. Now, we still have this entry to the changelog mentioning it on 3.6.1 but it actually only got released on 3.6.2.

The people who handle this part of the flow for stable releases are already fully aware of what happened but the Changelog itself will only get fixed on the next patch or on the next full version release (see here).

And then this will get [...] to stable when the 3.7 release is made. Until then, the inaccuracy in the changelog on stable will remain. If we do another hotfix on stable we can pick my changelog fix into it.

[FMorschel]

This will probably be the last update here. Flutter 3.27.4 is out now with Dart 3.6.2, so if you're running a Flutter project, you can update your SDK to get the fix!

[pq]

Thanks a million for pushing this through @FMorschel!