Fix panic in rustfix on overlapping suggestions

[raushan728]

Fixes #13030

What changed?

Updated apply_suggestions in rustfix to gracefully skip non-identical overlapping suggestions instead of propagating the AlreadyReplaced error.

Added a unit test to verify the fix.

[rustbot]

r? @epage

rustbot has assigned @epage.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ehuss, @epage, @weihanglo
• @ehuss, @epage, @weihanglo expanded to ehuss, epage, weihanglo
• Random selection from ehuss, epage, weihanglo

[epage]

Our contrib guide asks for tests to be added in a prior commit, reproducing the current problem (with the tests passing). The fix would then update the tests to show the new behavior

[raushan728]

@epage done u can review it now

[epage]

Note I was just focusing on process. As there isn't an end-to-end test because this is fixing a theoretical problem and not a real problem, I don't feel qualified to review this.

[epage]

r? ehuss

Would you be up for this considering this is a fix for a theoretical problem? I don't have much context for the actual problem to be able to tell if this addresses it.

[weihanglo]

Would you be up for this considering this is a fix for a theoretical problem?

Is it possible to have the test case from a real-world diagnostic? If not, I slightly lean towards we leave it.

View changes since this review

[epage]

From #13030 (comment)

I'd leave it open. Even though these types of suggestions aren't currently supported, it probably shouldn't panic.

It requires a multi-suggestion that is Machine Applicable but because we can't support that (at the UX level), the compiler shouldn't emit it.

[raushan728]

@weihanglo @epage about the e2e test making a real-world snapshot test is pretty hard bcz we'd have to force rustc to emit multiple exclusive MachineApplicable suggestions for the exact same span, which it shouldn't really do anyway.

that's exactly y i added a unit test to mock this edge case directly in apply_suggestions. the main goal is just defensive: stop the panic and gracefully skip overlaps like ehuss suggested in the original issue

let me know what u guys think or if u want me to try a diff approach

[epage]

We do have a mock rustc for cargo's test suite in https://github.com/rust-lang/cargo/blob/master/tests/testsuite/fix_n_times.rs

[raushan728]

hi @epage
thanks for the pointer to rustc-fix-shim — exactly what I needed

added an e2e test in tests/testsuite/fix_n_times.rs (Step::TwoFixExclusive) — forces the mock compiler to emit a single diagnostic with multiple overlapping MachineApplicable suggestions on the same span.

while digging in, realized the real issue was in CodeFix::apply, not just apply_suggestions — it was propagating AlreadyReplaced for intra-suggestion conflicts. Fixed it: if a conflict occurs but applied_any is true, we already committed a solution for that diagnostic, so we now gracefully skip the conflicting ones instead of panicking.

LMK what you think!

[epage]

@weihanglo

Is it possible to have the test case from a real-world diagnostic? If not, I slightly lean towards we leave it.

A benefit to the panic that came up during office hours is that if the compiler developer tests their fix, they will see the panic and know that they shouldn't do this. If we silently ignore it, there will be no feedback look for the compiler developer.

[weihanglo]

Yes. I also want to "leave" panic there. I don't think without a real world example this is going to anywhere.

[raushan728]

@epage @weihanglo Makes total sense! I get the need for a loud feedback loop.

Quick thought before I close this: Should we catch this specific overlap and throw a targeted panic instead of the generic one?

e.g., panic!("Compiler Bug: Emitted multiple overlapping MachineApplicable suggestions for the exact same span.")

This keeps the panic but tells the compiler dev exactly what went wrong. Let me know if u want this

[epage]

I could see it being helpful to have a more specific panic or error message. In particular, Cargo has an InternalError that prints the message and tells the user to report the issue to us.