Skip to content

Add duplicate trait bound lint #95052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Add duplicate trait bound lint #95052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
416195e
Add duplicate trait bound lint
pierwill Mar 17, 2022
f0c5c30
Bless test
pierwill Jul 20, 2022
486fb44
Update compiler/rustc_lint/src/traits.rs
pierwill Jul 20, 2022
0b346be
rename var
pierwill Jul 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_lint/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ macro_rules! late_lint_mod_passes {
UnstableFeatures: UnstableFeatures,
ArrayIntoIter: ArrayIntoIter::default(),
DropTraitConstraints: DropTraitConstraints,
DuplicateTraitBounds: DuplicateTraitBounds,
TemporaryCStringAsPtr: TemporaryCStringAsPtr,
NonPanicFmt: NonPanicFmt,
NoopMethodCall: NoopMethodCall,
Expand Down
43 changes: 43 additions & 0 deletions compiler/rustc_lint/src/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::LateContext;
use crate::LateLintPass;
use crate::LintContext;
use hir::GenericBound;
use rustc_data_structures::stable_set::FxHashSet;
use rustc_errors::fluent;
use rustc_hir as hir;
use rustc_span::symbol::sym;
Expand Down Expand Up @@ -132,3 +134,44 @@ impl<'tcx> LateLintPass<'tcx> for DropTraitConstraints {
}
}
}

declare_lint! {
pub DUP_TRAIT_BOUNDS,
Deny,
"Duplicate trait bounds are meaningless"
}

declare_lint_pass!(
DuplicateTraitBounds => [DUP_TRAIT_BOUNDS]
);

impl<'tcx> LateLintPass<'tcx> for DuplicateTraitBounds {
fn check_item(&mut self, cx: &LateContext<'_>, item: &'tcx hir::Item<'tcx>) {
let bounds: &[hir::GenericBound<'_>] = match &item.kind {
hir::ItemKind::Trait(_, _, _, bounds, _) => bounds,
hir::ItemKind::TraitAlias(_, bounds) => bounds,
_ => return,
};

let mut set = FxHashSet::default();
for bound in bounds.into_iter() {
match bound {
GenericBound::Trait(polytraitref, _) => {
let Some(did) = polytraitref.trait_ref.trait_def_id() else { continue; };
// If inserting the trait bound into the set returns `false`,
// there is a duplicate.
if !set.insert(did) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@pierwill pierwill Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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> {}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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,
    }
}

let span_of_dup = polytraitref.span;
cx.struct_span_lint(DUP_TRAIT_BOUNDS, span_of_dup, |lint| {
let msg = format!("duplicate trait bound");
lint.build(&msg)
.span_help(span_of_dup, "Remove this duplicate trait bound")
.emit();
});
};
}
_ => continue,
}
}
}
}
12 changes: 12 additions & 0 deletions src/test/ui/traits/duplicate-trait-bounds.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
pub trait DirectedGraph {}
pub trait WithStartNode {}
pub trait WithPredecessors {}
pub trait WithSuccessors {}
pub trait WithNumNodes {}

pub trait ControlFlowGraph:
DirectedGraph + WithStartNode + WithPredecessors + WithStartNode + WithSuccessors + WithNumNodes
//~ ERROR duplicate trait bound
{}

fn main() {}
15 changes: 15 additions & 0 deletions src/test/ui/traits/duplicate-trait-bounds.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error: duplicate trait bound
--> $DIR/duplicate-trait-bounds.rs:8:56
|
LL | DirectedGraph + WithStartNode + WithPredecessors + WithStartNode + WithSuccessors + WithNumNodes
| ^^^^^^^^^^^^^
|
= note: `#[deny(dup_trait_bounds)]` on by default
help: Remove this duplicate trait bound
--> $DIR/duplicate-trait-bounds.rs:8:56
|
LL | DirectedGraph + WithStartNode + WithPredecessors + WithStartNode + WithSuccessors + WithNumNodes
| ^^^^^^^^^^^^^

error: aborting due to previous error