Add duplicate trait bound lint

[pierwill]

Do not allow duplicate trait bounds.

Closes #90108.

[rust-highfive]

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

[pierwill]

@rustbot author

[pierwill]

Two things here:

1. Maybe we can use a pop or remove to check for duplicates in a loop like this?
2. What's the best way to do the equality check given a PolyTraitRef? Not a lot of Eq implementations on the types here...

[estebank]

I believe we can dig all the way in to the corresponding DefId of the trait and use that, so we skip the ones that aren't trait bounds (like : 'a and <X as Y>::Z: A) and ignore the others. It'd be nice to also find redundant bounds for those, but we can start with only trait bounds.

[estebank]

Can you add a test exercising this?

[estebank]

When adding a test file you need to 1) annotate the line with an error with //~ ERROR message to match and 2) run x.py test src/test/ui/path/to/file --blessto generate the corresponding.stderr` file that checks against visual changes.

[pierwill]

@rustbot ready

[pierwill]

@rustbot author

[pierwill]

Looks like we need to inspect the generics as well...

[estebank]

Instead of using a set, you should use a FxHashMap so that you can keep the Span around and use that Span in the lint to point at the previous trait bound, to minimize confusion.

[pierwill]

Thanks for this!

I'm not sure which methods to use here. Using this rough implementation

let mut map = FxHashMap::default();
        for bound in bounds.into_iter() {
            match bound {
                GenericBound::Trait(polytraitref, _) => {
                    let Some(did) = polytraitref.trait_ref.trait_def_id() else { continue; };
                    let span = polytraitref.span;
                    if map.insert(&did, &span).is_none() {
                        let span_of_dup_bound = span;
                        let span_of_prev_bound = map.get(&did).unwrap();
                        cx.struct_span_lint(DUP_TRAIT_BOUNDS, span_of_dup_bound, |lint| {
                            let msg = format!("duplicate trait bound");
                            lint.build(&msg)
                                .span_note(span_of_prev_bound, "trait bound previously used here")
                                .span_help(span_of_dup_bound, "remove this duplicate trait bound")
                                .emit();
                        });
                    };
                }
                _ => continue,
            }
        }
    }

I'm getting

   --> compiler/rustc_lint/src/traits.rs:169:44
    |
169 | ...                   .span_help(span_of_prev_bound, "trait bound previously used here")
    |                        --------- ^^^^^^^^^^^^^^^^^^ the trait `From<&&rustc_span::Span>` is not implemented for `MultiSpan`
    |                        |
    |                        required by a bound introduced by this call
    |
    = help: the following other types implement trait `From<T>`:
              <MultiSpan as From<Vec<rustc_span::Span>>>
              <MultiSpan as From<rustc_span::Span>>
    = note: required because of the requirements on the impl of `Into<MultiSpan>` for `&&rustc_span::Span`
note: required by a bound in `DiagnosticBuilder::<'a, G>::span_help`
   --> /Users/will/repos/rust/compiler/rustc_errors/src/diagnostic_builder.rs:457:18
    |
457 |         sp: impl Into<MultiSpan>,
    |                  ^^^^^^^^^^^^^^^ required by this bound in `DiagnosticBuilder::<'a, G>::span_help`

I don't yet understand how to work with MultiSpan...

Also, for right now I'm thinking of this span message as a nice-to-have. I'm more worried about this test case and handling the generic parameters:

rust/src/test/ui/higher-rank-trait-bounds/normalize-under-binder/issue-81809.rs

Line 15 in 0b346be

trait StoreIndex: Indexer<u8> + Indexer<u16> {}

[fee1-dead]

That is because you have a &&Span and you need to dereference it to work.

[estebank]

That is because you have a &&Span and you need to dereference it to work.

Fun case, because then we should be suggesting the derefecence in the diagnostic, and we're not :)

[estebank]

You can use try_insert:

let mut map = FxHashMap::default();
for bound in bounds.into_iter() {
    match bound {
        GenericBound::Trait(polytraitref, _) => {
            let Some(did) = polytraitref.trait_ref.trait_def_id() else { continue; };
            let span = polytraitref.span;
            if let Err(occupied) = map.try_insert(did, span) {
                cx.struct_span_lint(DUP_TRAIT_BOUNDS, span, |lint| {
                    let msg = format!("duplicate trait bound");
                    lint.build(&msg)
                        .span_label(*occupied.value, "trait bound previously used here")
                        .span_label(span, "duplicate trait bound")
                        .emit();
                });
            };
        }
        _ => continue,
    }
}

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[Dylan-DPC]

@pierwill any updates on the test failure?

[bors]

☔ The latest upstream changes (presumably #104863) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

Closing this as inactive.