Skip to content
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.

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

Duplicate bounds lint #88096

Closed
5 changes: 1 addition & 4 deletions compiler/rustc_ast/src/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,10 +350,7 @@ pub fn walk_enum_def<'a, V: Visitor<'a>>(
walk_list!(visitor, visit_variant, &enum_definition.variants);
}

pub fn walk_variant<'a, V: Visitor<'a>>(visitor: &mut V, variant: &'a Variant)
where
V: Visitor<'a>,
{
pub fn walk_variant<'a, V: Visitor<'a>>(visitor: &mut V, variant: &'a Variant) {
visitor.visit_ident(variant.ident);
visitor.visit_vis(&variant.vis);
visitor.visit_variant_data(&variant.data);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_data_structures/src/transitive_relation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ impl<T: Eq + Hash> TransitiveRelation<T> {
pub fn maybe_map<F, U>(&self, mut f: F) -> Option<TransitiveRelation<U>>
where
F: FnMut(&T) -> Option<U>,
U: Clone + Debug + Eq + Hash + Clone,
U: Clone + Debug + Eq + Hash,
{
let mut result = TransitiveRelation::default();
for edge in &self.edges {
Expand Down
151 changes: 151 additions & 0 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ use rustc_trait_selection::traits::misc::can_type_implement_copy;
use crate::nonstandard_style::{method_context, MethodLateContext};

use std::fmt::Write;
use std::hash::{Hash, Hasher};
use tracing::{debug, trace};

// hardwired lints from librustc_middle
Expand Down Expand Up @@ -3140,3 +3141,153 @@ impl<'tcx> LateLintPass<'tcx> for DerefNullPtr {
}
}
}

declare_lint! {
/// ### What it does
/// Checks for cases where the same trait or lifetime bound is specified more than once.
///
/// ### Why is this bad?
/// Duplicate bounds makes the code less readable than specifing them only once.
///
/// ### Example
/// ```rust
/// fn func<T: Clone + Default>(arg: T) where T: Clone + Default {}
/// ```
///
/// could be written as:
///
/// ```rust
/// fn func<T: Clone + Default>(arg: T) {}
/// ```
/// or
///
/// ```rust
/// fn func<T>(arg: T) where T: Clone + Default {}
/// ```
pub DUPLICATE_BOUNDS,
Warn,
"Check if the same bound is specified more than once"
}

declare_lint_pass!(DuplicateBounds => [DUPLICATE_BOUNDS]);

impl<'tcx> LateLintPass<'tcx> for DuplicateBounds {
fn check_generics(&mut self, cx: &LateContext<'tcx>, gen: &'tcx hir::Generics<'_>) {
struct Bound {
kind: BoundKind,
span: Span,
}

#[derive(Hash, PartialEq, Eq)]
enum BoundKind {
Trait(Res),
Lifetime(hir::LifetimeName),
}

impl BoundKind {
fn as_str(&self) -> &'static str {
match self {
BoundKind::Trait(_) => "trait",
BoundKind::Lifetime(_) => "lifetime",
}
}
}

impl Bound {
fn from_generic(bound: &hir::GenericBound<'_>) -> Option<Self> {
match bound {
hir::GenericBound::Trait(t, _) => {
Some(Self { kind: BoundKind::Trait(t.trait_ref.path.res), span: t.span })
}
hir::GenericBound::Outlives(lifetime) => {
Some(Self { kind: BoundKind::Lifetime(lifetime.name), span: lifetime.span })
}
_ => None,
}
}
}

impl PartialEq for Bound {
fn eq(&self, other: &Self) -> bool {
self.kind == other.kind
}
}

impl Hash for Bound {
fn hash<H: Hasher>(&self, state: &mut H) {
self.kind.hash(state)
}
}

impl Eq for Bound {}

if gen.span.from_expansion() {
return;
}

let mut bounds = FxHashMap::default();
for param in gen.params {
if let hir::ParamName::Plain(ref ident) = param.name {
let mut uniq_bounds = FxHashSet::default();

for res in param.bounds.iter().filter_map(Bound::from_generic) {
let span = res.span.clone();
let kind = res.kind.as_str();
if !uniq_bounds.insert(res) {
cx.struct_span_lint(DUPLICATE_BOUNDS, span, |lint| {
lint.build(&format!("this {} bound has already been specified", kind))
.help(&format!("consider removing this {} bound", kind))
.emit()
});
}
}

bounds.insert(*ident, uniq_bounds);
}
}

for predicate in gen.where_clause.predicates {
let res = match &predicate {
hir::WherePredicate::BoundPredicate(hir::WhereBoundPredicate {
bounded_ty:
hir::Ty {
kind:
hir::TyKind::Path(hir::QPath::Resolved(_, hir::Path { segments, .. })),
..
},
bounds,
..
}) => segments.first().map(|s| (s.ident, *bounds)),
hir::WherePredicate::RegionPredicate(hir::WhereRegionPredicate {
lifetime:
hir::Lifetime {
name: hir::LifetimeName::Param(hir::ParamName::Plain(ident)),
..
},
bounds,
..
}) => Some((*ident, *bounds)),
_ => None,
};

if let Some((ident, where_predicate_bounds)) = res {
if let Some(bounds) = bounds.get_mut(&ident) {
for res in where_predicate_bounds.iter().filter_map(Bound::from_generic) {
let span = res.span.clone();
let kind = res.kind.as_str();
if !bounds.insert(res) {
cx.struct_span_lint(DUPLICATE_BOUNDS, span, |lint| {
lint.build(&format!(
"this {} bound has already been specified",
kind
))
.help(&format!("consider removing this {} bound", kind))
.emit()
});
}
}
}
}
}
}
}
1 change: 1 addition & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ macro_rules! late_lint_mod_passes {
// Depends on referenced function signatures in expressions
MutableTransmutes: MutableTransmutes,
TypeAliasBounds: TypeAliasBounds,
DuplicateBounds: DuplicateBounds,
TrivialConstraints: TrivialConstraints,
TypeLimits: TypeLimits::new(),
NonSnakeCase: NonSnakeCase,
Expand Down
1 change: 1 addition & 0 deletions library/core/src/iter/traits/iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2434,6 +2434,7 @@ pub trait Iterator {
/// assert!(result.is_err());
/// ```
#[inline]
#[cfg_attr(not(bootstrap), allow(duplicate_bounds))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the lint reported here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because Try and TryV2 are the same thing now. cc @scottmcm

Copy link
Member

Choose a reason for hiding this comment

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

Oh, good point, I never cleaned that alias up after the bootstrap compiler updated. New PR: #88223

#[unstable(feature = "try_find", reason = "new API", issue = "63178")]
fn try_find<F, R, E>(&mut self, f: F) -> Result<Option<Self::Item>, E>
where
Expand Down
1 change: 1 addition & 0 deletions library/proc_macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1233,6 +1233,7 @@ pub mod tracked_env {
/// Besides the dependency tracking this function should be equivalent to `env::var` from the
/// standard library, except that the argument must be UTF-8.
#[unstable(feature = "proc_macro_tracked_env", issue = "74690")]
#[cfg_attr(not(bootstrap), allow(duplicate_bounds))]
pub fn var<K: AsRef<OsStr> + AsRef<str>>(key: K) -> Result<String, VarError> {
Comment on lines +1236 to 1237
Copy link
Member Author

@ibraheemdev ibraheemdev Aug 16, 2021

Choose a reason for hiding this comment

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

This seems to be a false positive from the old clippy lint as well:

#![deny(clippy::pedantic)]

fn foo<T: AsRef<str>>()
where
    T: AsRef<String>,
{
}
error: this trait bound is already specified in the where clause
 --> src/lib.rs:3:11
  |
3 | fn foo<T: AsRef<str>>()
  |           ^^^^^^^^^^
  |

I guess I would have to compare Path::segments instead of Res to handle this case?

let key: &str = key.as_ref();
let value = env::var(key);
Expand Down
82 changes: 82 additions & 0 deletions src/test/ui/lint/duplicate_bounds.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#![deny(duplicate_bounds)]

trait DupDirectAndWhere {}
fn dup_direct_and_where<T: DupDirectAndWhere>(t: T)
where
T: DupDirectAndWhere,
//~^ ERROR this trait bound has already been specified
T: DupDirectAndWhere,
//~^ ERROR this trait bound has already been specified
{
unimplemented!();
}

trait DupDirect {}
fn dup_direct<T: DupDirect + DupDirect>(t: T) {
//~^ ERROR this trait bound has already been specified
unimplemented!();
}

trait DupWhere {}
fn dup_where<T>(t: T)
where
T: DupWhere + DupWhere,
//~^ ERROR this trait bound has already been specified
{
unimplemented!();
}

trait NotDup {}
fn not_dup<T: NotDup, U: NotDup>((t, u): (T, U)) {
unimplemented!();
}

fn dup_lifetimes<'a, 'b: 'a + 'a>()
//~^ ERROR this lifetime bound has already been specified
where
'b: 'a,
//~^ ERROR this lifetime bound has already been specified
{
}

fn dup_lifetimes_generic<'a, T: 'a + 'a>()
//~^ ERROR this lifetime bound has already been specified
where
T: 'a,
//~^ ERROR this lifetime bound has already been specified
{
}

trait Everything {}
fn everything<T: Everything + Everything, U: Everything + Everything>((t, u): (T, U))
//~^ ERROR this trait bound has already been specified
//~| ERROR this trait bound has already been specified
where
T: Everything + Everything + Everything,
//~^ ERROR this trait bound has already been specified
//~| ERROR this trait bound has already been specified
//~| ERROR this trait bound has already been specified
U: Everything,
//~^ ERROR this trait bound has already been specified
{
unimplemented!();
}

trait DupStructBound {}
struct DupStruct<T: DupStructBound + DupStructBound>(T)
//~^ ERROR this trait bound has already been specified
where
T: DupStructBound;
//~^ ERROR this trait bound has already been specified

impl<'a, T: 'a + DupStructBound + DupStructBound> DupStruct<T>
//~^ ERROR this trait bound has already been specified
where
T: 'a + DupStructBound,
//~^ ERROR this lifetime bound has already been specified
//~| ERROR this trait bound has already been specified
{
fn _x() {}
}

fn main() {}
Loading