Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CP] Fixes multiple fixes for insertions in multi-file library #59853

Open
FMorschel opened this issue Jan 7, 2025 · 9 comments
Open

[CP] Fixes multiple fixes for insertions in multi-file library #59853

FMorschel opened this issue Jan 7, 2025 · 9 comments
Assignees
@vsmenon @itsjustkevin @mit-mit @athomas
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. cherry-pick-review Issue that need cherry pick triage to approve triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@FMorschel
Copy link
Contributor

Commit(s) to merge

https://dart-review.googlesource.com/c/sdk/+/401865

Target

Stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/403380

Issue Description

All platforms are affected.

The dart fix command applies insertions for all files in the library after #55520. This means that in a library with two files if we have errors on both and one of them has an insertion fix (say adding const ), that insertion is added twice.

The desired behaviour is to only do so for the current file since it runs for every file anyway. The initial issue was about detecting the fixes needed for part files but it didn't consider only fixing for a single file at a time, as it was doing previously.

What is the fix

The fix removes the loop for all units in the library and only runs for the unit with the current path.

Why cherry-pick

It creates errors in code fixes.

Risk

The risk of merging is low.

Issue link(s)

#59572

Extra Info

CC @pq
@FMorschel FMorschel added the cherry-pick-review Issue that need cherry pick triage to approve label Jan 7, 2025
@FMorschel FMorschel mentioned this issue Jan 7, 2025
Closed
@dart-github-bot
Copy link
Collaborator

Summary: dart fix incorrectly applied insertions multiple times in multi-file libraries. The fix restricts insertions to the current file, resolving duplicate additions.

@dart-github-bot dart-github-bot added area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Jan 7, 2025
@FMorschel
Copy link
Contributor Author

I'm unsure if I should create yet another CP for beta in this case (my first CP). Thanks for your time!

@athomas
Copy link
Member

athomas commented Jan 7, 2025

@bwilkerson WDYT?

@bwilkerson
Copy link
Member

@FMorschel The prepared changelist for beta/stable (https://dart-review.googlesource.com/c/sdk/+/403380) does not appear to have the actual bug fix in it. (The path isn't being passed in and used.)

@athomas Once the CL is correct, I think the change has significant value to developers working in libraries that have parts (though I don't know how many developers fall into that category) and very low risk. Most of the changes are in test code; the meaningful changes amount to three lines of code.

@FMorschel
Copy link
Contributor Author

Thanks @bwilkerson! Fixed that and rebased it.

@FMorschel
Copy link
Contributor Author

I'll do it for beta later then if you want me to. Thanks!

@FMorschel
Copy link
Contributor Author

As I was a bit confused by the issue template and the docs, I've opened https://dart-review.googlesource.com/c/sdk/+/403482 which adds the fix but is based on beta instead. If I should open a new issue, please let me know so I can do it properly.

@bwilkerson
Copy link
Member

Both CLs look right to me.

copybara-service bot pushed a commit that referenced this issue Jan 7, 2025
@FMorschel
[stable] Fixes multiple fixes for insertions in multi-file library
49d7d8f 
Fixes multiple insertions in multi-file library when using `dart fix`.

Merges the test in https://dart-review.googlesource.com/c/sdk/+/401840
that is related to https://dart-review.googlesource.com/c/sdk/+/401180,
but in the LSP handler where it can be more easily debugged.

Bug #59572

Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/401865
Cherry-pick-request: #59853
Change-Id: Ic0c42c4cacb5a270b1d92198819afa6813c83b17
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/403380
Reviewed-by: Phil Quitslund <pquitslund@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Auto-Submit: Felipe Morschel <git@fmorschel.dev>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@FMorschel
Copy link
Contributor Author

How did the stable CL merge (and not the beta one which is weirder IMO) if this is still unapproved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vsmenon vsmenon

@itsjustkevin itsjustkevin

@mit-mit mit-mit

@athomas athomas

Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. cherry-pick-review Issue that need cherry pick triage to approve triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@vsmenon @bwilkerson @itsjustkevin @mit-mit @athomas @FMorschel @dart-github-bot