use stable sort to sort multipart diagnostics

[folkertdev]

I think a stable sort should be used to sort the different parts of a multipart selection. The current unstable sort uses the text of the suggestion as a tie-breaker. That just doesn't seem right, and the order of the input is a better choice I think, because it gives the diagnostic author more control.

This came up when I was building a suggestion where

fn foo() {}

must be turned into an unsafe function, and an attribute must be added

#[target_feature(enable = "...")]
unsafe fn foo() {}

In this example, the two suggestions occur at the same position, but the order is extremely important: unsafe must come after the attribute. But the situation changes if there is a pub/pub(crate), and if the unsafe is already present. It just out that because of the suggestion text, there is no way for me to order the suggestions correctly.

This change probably should be tested, but are there tests of the diagnostics code itself in the tests?

r? @estebank

[compiler-errors]

Doesn't this leave: [(s, "a"), (s, "b"), (s, "a"), (s, "b")] not deduplicated?

[compiler-errors]

I would rather you do something like:

let seen = FxHashSet::default();
suggestions.retain(|(span, msg)| seen.insert(span.data(), msg.clone()));

If possible...?

Using span.data() should deduplicate source-equal spans, and using the msg as the part of the key in the seen allows us to deduplicate messages still.

[folkertdev]

That seems to almost work, but I think it gets tripped up when macros are involved? when using

suggestion.sort_by_key(|(span, _)| *span);
let mut seen = crate::FxHashSet::default();
suggestion.retain(|(span, msg)| seen.insert((span.data(), msg.clone())));

I get this failure

failures:

---- [ui] tests/ui/macros/macro-span-issue-116502.rs stdout ----

when comparing to the previous solution (top/left) this new solution (bottom/right) does not deduplicate correctly:

left: [
    (/home/folkertdev/rust/rust/tests/ui/macros/macro-span-issue-116502.rs:7:13: 7:14 (#4), "U"), 
    (/home/folkertdev/rust/rust/tests/ui/macros/macro-span-issue-116502.rs:10:13: 10:23 (#0), "<U>")
]
 right: [
    (/home/folkertdev/rust/rust/tests/ui/macros/macro-span-issue-116502.rs:7:13: 7:14 (#4), "U"), 
    (/home/folkertdev/rust/rust/tests/ui/macros/macro-span-issue-116502.rs:7:13: 7:14 (#5), "U"), 
    (/home/folkertdev/rust/rust/tests/ui/macros/macro-span-issue-116502.rs:7:13: 7:14 (#6), "U"), 
    (/home/folkertdev/rust/rust/tests/ui/macros/macro-span-issue-116502.rs:10:13: 10:23 (#0), "<U>")
]

so this may need something slightly different from span.data()?

btw the sorting is needed for just 2 test cases:

failures:
    [ui] tests/ui/moves/nested-loop-moved-value-wrong-continue.rs
    [ui] tests/ui/moves/recreating-value-in-loop-condition.rs

[folkertdev]

this works

        suggestion.sort_by_key(|(span, _)| *span);
        let mut seen = crate::FxHashSet::default();
        suggestion.retain(|(span, msg)| {
            seen.insert((
                {
                    let mut data = span.data();
                    data.ctxt = rustc_span::hygiene::SyntaxContext::root();
                    data
                },
                msg.clone(),
            ))
        });

is that too hacky?

[compiler-errors]

Sorry, I guess you really should be doing something like:

let mut seen = crate::FxHashSet::default();
suggestion.retain(|(span, msg)| seen.insert((span.lo(), span.hi(), msg.clone())));

To ignore the syntax context.

[compiler-errors]

Also, you don't need to be sorting or deduplicating if you use this approach. That's the whole point of the retain call :)

[folkertdev]

@compiler-errors the sorting also matters for at which line the error is reported. In this one case, that actually caused issues. I've swapped the spans here now to preserve the current error message.

[compiler-errors]

@bors r+ rollup

[bors]

📌 Commit 38874a6 has been approved by compiler-errors

It is now in the queue for this repository.