fix: treat intra-suggestion overlaps as InternalError instead of panicking

[raushan728]

Fixed #13030

Rustc sometimes gives multiple overlapping MachineApplicable suggestions for the exact same span. Before, this would either panic or quietly skip them. Now we catch this case properly and throw a clear cargo::util::internal error with a message asking the user to report it as a compiler bug.
Also added an end-to-end test in tests/testsuite/fix_n_times.rs to make sure the new error message looks right.

[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.

[raushan728]

@epage updated the PR It now returns an InternalError with the specific diagnostic message instead of skipping the overlap along with the expected output in the e2e test. Let me know if it looks good now

[epage]

Tests should be in commits before the change

[epage]

Please clean up the commits to reflect how this should be reviewed and merged and not how this was developed.