Skip to content

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

This assumes that if any of the machine applicable fixes (solution)
in a diagnostic is a duplicate, then cargo fix should only apply
the entire suggestion once.

How should we test and review this PR?

The first commit demonstrates the case in rust-lang/rust#123304.

One caveat is that cargo fix would show Fixed .. (2 fixes) for duplicate suggestions,
but actually it only applies the fix once and for all.

Additional information

r? ehuss

@rustbot rustbot added Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2024
Comment on lines 1936 to 1939
assert_eq!(
p.read_file("src/lib.rs").matches("unsafe").count(),
4,
"2 in lint name, 1 from original unsafe fn, 1 from newly-applied unsafe blocks"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the message include the adjusted src/lib.rs? That would removes one step in the debugging process

@weihanglo
Copy link
Member Author

I am thinking of moving this logic into CodeFix since others might also need this deduplication (e.g. compiletest in rust-lang/rust)

This assumes that if any of the machine applicable fixes in
a diagnostic suggestion is a duplicate, we should see the
entire suggestion as a duplicate.
@weihanglo weihanglo marked this pull request as ready for review April 10, 2024 15:58
let mut fix = CodeFix::new(code);
for suggestion in suggestions.iter().rev() {
// This assumes that if any of the machine applicable fixes in
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to duplicate the work in also rustfix. I didn't do it in CodeFix as it might require a longer lifetime bound than it was.

@epage
Copy link
Contributor

epage commented Apr 10, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 10, 2024

📌 Commit 95edc06 has been approved by epage

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 Apr 10, 2024
@bors
Copy link
Contributor

bors commented Apr 10, 2024

⌛ Testing commit 95edc06 with merge 40ce8ac...

@bors
Copy link
Contributor

bors commented Apr 10, 2024

☀️ Test successful - checks-actions
Approved by: epage
Pushing 40ce8ac to master...

@bors bors merged commit 40ce8ac into rust-lang:master Apr 10, 2024
@weihanglo weihanglo deleted the dedup-suggestion branch April 10, 2024 17:52
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2024
Update cargo

11 commits in 28e7b2bc0a812f90126be30f48a00a4ada990eaa..74fd5bc730b828dbc956335b229ac34ba47f7ef7
2024-04-05 19:31:01 +0000 to 2024-04-10 18:40:49 +0000
- chore: downgrade to openssl v1.1.1 (again) (rust-lang/cargo#13731)
- fix(cargo-fix): dont apply same suggestion twice (rust-lang/cargo#13728)
- refactor: make `resolve_with_previous` clearer (rust-lang/cargo#13727)
- fix(package): Normalize paths in `Cargo.toml` (rust-lang/cargo#13729)
- refactor: Track when MSRV is explicitly set, either way (rust-lang/cargo#13732)
- [fix]:Build script not rerun when target rustflags change (rust-lang/cargo#13560)
- feat(add): Stabilize MSRV-aware version req selection (rust-lang/cargo#13608)
- Fix github fast path redirect. (rust-lang/cargo#13718)
- Add release notes for 1.77.1 (rust-lang/cargo#13717)
- doc(semver): remove mention of deprecated tool rust-semverver (rust-lang/cargo#13715)
- chore: fix some typos (rust-lang/cargo#13714)

r? ghost
@rustbot rustbot added this to the 1.79.0 milestone Apr 10, 2024
weihanglo added a commit to weihanglo/cargo that referenced this pull request Nov 1, 2024
This case also emits a insert-only suggestions (span start == end),
adn doesn't rely on any lint behavior.

See also

* rust-lang#13728
* rust-lang#13027
weihanglo added a commit to weihanglo/cargo that referenced this pull request Nov 1, 2024
This case also emits an insert-only suggestions (span start == end),
adn doesn't rely on any lint behavior.

See also

* rust-lang#13728
* rust-lang#13027
weihanglo added a commit to weihanglo/cargo that referenced this pull request Nov 1, 2024
This case also emits an insert-only suggestions (span start == end),
and doesn't rely on any lint behavior.

See also

* rust-lang#13728
* rust-lang#13027
weihanglo added a commit to weihanglo/cargo that referenced this pull request Nov 1, 2024
This case also emits an insert-only suggestions (span start == end),
and doesn't rely on any lint behavior.

See also

* rust-lang#13728
* rust-lang#13027
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