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.

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

rustfix: replace special-case duplicate handling with error #14782

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

saites
Copy link
Contributor

@saites saites commented Nov 5, 2024

What does this PR try to resolve?

This PR changes how conflicts are handled in rustfix.

By design, cargo fix repeatedly compiles the code, collects suggested changes, and applies changes that don't conflict with one another. Often, conflicts arise because the same suggestion is reported multiple times e.g., due to a macro expansion. There have been previous changes to address that specific case, but following the PR to add transactional semantics, it makes sense to change how this is handled.

Specifically, this PR adds details to Error::AlreadyReplaced to indicate whether the specific replacement is identical to the one it conflicts with, allowing callers to handle this case, rather than special-casing it within replace.rs itself. To that point, this PR changes fix.rs and lib.rs to handle identical replacements by skipping the conflicting suggestion. This is not exactly identical to their previous behavior: before, it skipped a suggestion if any solution had been applied, whereas this skips it if any replacement within a solution has been applied. While more expansive, this is very much the spirit of the goal described above.

How should we test and review this PR?

The existing tests have been updated to account for this change.

Additional information

See: #13027

@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2024
@saites
Copy link
Contributor Author

saites commented Nov 5, 2024

r? @weihanglo might be the better choice of reviewer, since you've seen most of these changes already.

@rustbot rustbot assigned weihanglo and unassigned ehuss Nov 5, 2024
@weihanglo
Copy link
Member

Looks great. Thanks for the update!

@bors r+

Note to self: need to update rustfix in rust-lang/rust for compiletest once [email protected] gets published. Should be on 1.84.

@bors
Copy link
Contributor

bors commented Nov 5, 2024

📌 Commit 7d6889b has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2024
@bors
Copy link
Contributor

bors commented Nov 5, 2024

⌛ Testing commit 7d6889b with merge 765a6f1...

@bors
Copy link
Contributor

bors commented Nov 5, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 765a6f1 to master...

@bors bors merged commit 765a6f1 into rust-lang:master Nov 5, 2024
22 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2024
Update cargo

16 commits in 0310497822a7a673a330a5dd068b7aaa579a265e..4a2d8dc636445b276288543882e076f254b3ae95
2024-11-01 19:27:56 +0000 to 2024-11-09 19:10:33 +0000
- test: adjust `cargo_test_env` to unblock rust submodule update (rust-lang/cargo#14803)
- feat(warnings): add build.warnings option (rust-lang/cargo#14388)
- Revert "feat: Add `CARGO_RUSTC_CURRENT_DIR`" (rust-lang/cargo#14799)
- CI: make the `lint-docs` job required (rust-lang/cargo#14797)
- Switch CI from bors to merge queue (rust-lang/cargo#14718)
- docs(test):  Document Execs assertions based on port effort (rust-lang/cargo#14793)
- fix(test): Make redactions consistent with snapbox (rust-lang/cargo#14790)
- test(gc): Update remaining unordered tests to snapbox (rust-lang/cargo#14785)
- Normalize the `target` paths (rust-lang/cargo#14497)
- rustfix: replace special-case duplicate handling with error (rust-lang/cargo#14782)
- test: Update some emaining unordered tests to snapbox (rust-lang/cargo#14781)
- Change config paths to only check CARGO_HOME for cargo-script (rust-lang/cargo#14749)
- Enable transfer feature in triagebot (rust-lang/cargo#14777)
- Add transactional semantics to `rustfix` (rust-lang/cargo#14747)
- doc: fix `GlobalContext` reference (rust-lang/cargo#14773)
- chore: update handlebars to v6, fix build error (rust-lang/cargo#14772)
@rustbot rustbot added this to the 1.84.0 milestone Nov 10, 2024
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
Update cargo

16 commits in 0310497822a7a673a330a5dd068b7aaa579a265e..4a2d8dc636445b276288543882e076f254b3ae95
2024-11-01 19:27:56 +0000 to 2024-11-09 19:10:33 +0000
- test: adjust `cargo_test_env` to unblock rust submodule update (rust-lang/cargo#14803)
- feat(warnings): add build.warnings option (rust-lang/cargo#14388)
- Revert "feat: Add `CARGO_RUSTC_CURRENT_DIR`" (rust-lang/cargo#14799)
- CI: make the `lint-docs` job required (rust-lang/cargo#14797)
- Switch CI from bors to merge queue (rust-lang/cargo#14718)
- docs(test):  Document Execs assertions based on port effort (rust-lang/cargo#14793)
- fix(test): Make redactions consistent with snapbox (rust-lang/cargo#14790)
- test(gc): Update remaining unordered tests to snapbox (rust-lang/cargo#14785)
- Normalize the `target` paths (rust-lang/cargo#14497)
- rustfix: replace special-case duplicate handling with error (rust-lang/cargo#14782)
- test: Update some emaining unordered tests to snapbox (rust-lang/cargo#14781)
- Change config paths to only check CARGO_HOME for cargo-script (rust-lang/cargo#14749)
- Enable transfer feature in triagebot (rust-lang/cargo#14777)
- Add transactional semantics to `rustfix` (rust-lang/cargo#14747)
- doc: fix `GlobalContext` reference (rust-lang/cargo#14773)
- chore: update handlebars to v6, fix build error (rust-lang/cargo#14772)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command-fix S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants