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

[WIP] Lint duplicate trait and lifetime bounds #57909

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/libcore/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
integer constants",
issue = "27778")]

#![cfg_attr(not(stage0), allow(duplicate_bounds))]

use borrow::{Borrow, BorrowMut};
use cmp::Ordering;
use convert::TryFrom;
Expand Down
15 changes: 15 additions & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,19 @@ declare_lint! {
"outlives requirements can be inferred"
}

declare_lint! {
pub DEPRECATED_IN_FUTURE,
Allow,
"detects use of items that will be deprecated in a future version",
report_in_external_macro: true
}

declare_lint! {
pub DUPLICATE_BOUNDS,
Warn,
"detects duplicate bounds on type parameters, lifetime parameters, and projections"
}

/// Some lints that are buffered from `libsyntax`. See `syntax::early_buffered_lints`.
pub mod parser {
declare_lint! {
Expand Down Expand Up @@ -440,6 +453,8 @@ impl LintPass for HardwiredLints {
parser::ILL_FORMED_ATTRIBUTE_INPUT,
DEPRECATED_IN_FUTURE,
AMBIGUOUS_ASSOCIATED_ITEMS,
DUPLICATE_BOUNDS,
parser::QUESTION_MARK_MACRO_SEP,
)
}
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc/traits/util.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use errors::DiagnosticBuilder;
use hir;
use hir::def_id::DefId;
use traits::specialize::specialization_graph::NodeItem;
Expand Down
22 changes: 18 additions & 4 deletions src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1212,18 +1212,32 @@ impl<'tcx> PolyTraitPredicate<'tcx> {
// ok to skip binder since trait def-id does not care about regions
self.skip_binder().def_id()
}

pub fn self_ty(&self) -> Binder<Ty<'tcx>> {
self.map_bound(|p| p.self_ty())
}
}

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, RustcEncodable, RustcDecodable)]
pub struct OutlivesPredicate<A,B>(pub A, pub B); // `A: B`
pub type PolyOutlivesPredicate<A,B> = ty::Binder<OutlivesPredicate<A,B>>;
pub struct OutlivesPredicate<A, B>(pub A, pub B); // `A: B`
pub type PolyOutlivesPredicate<A, B> = ty::Binder<OutlivesPredicate<A, B>>;
pub type RegionOutlivesPredicate<'tcx> = OutlivesPredicate<ty::Region<'tcx>,
ty::Region<'tcx>>;
pub type TypeOutlivesPredicate<'tcx> = OutlivesPredicate<Ty<'tcx>,
ty::Region<'tcx>>;
pub type PolyRegionOutlivesPredicate<'tcx> = ty::Binder<RegionOutlivesPredicate<'tcx>>;
pub type PolyTypeOutlivesPredicate<'tcx> = ty::Binder<TypeOutlivesPredicate<'tcx>>;

impl<A: Copy, B: Copy> PolyOutlivesPredicate<A, B> {
pub fn var(&self) -> Binder<A> {
self.map_bound(|pred| pred.0)
}

pub fn value(&self) -> Binder<B> {
self.map_bound(|pred| pred.1)
}
}

#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug, RustcEncodable, RustcDecodable)]
pub struct SubtypePredicate<'tcx> {
pub a_is_expected: bool,
Expand Down Expand Up @@ -1265,11 +1279,11 @@ impl<'tcx> PolyProjectionPredicate<'tcx> {
// This is because here `self` has a `Binder` and so does our
// return value, so we are preserving the number of binding
// levels.
self.map_bound(|predicate| predicate.projection_ty.trait_ref(tcx))
self.map_bound(|pred| pred.projection_ty.trait_ref(tcx))
}

pub fn ty(&self) -> Binder<Ty<'tcx>> {
self.map_bound(|predicate| predicate.ty)
self.map_bound(|pred| pred.ty)
}

/// The `DefId` of the `TraitItem` for the associated type.
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ use rustc::hir::{self, GenericParamKind, PatKind};

use nonstandard_style::{MethodLateContext, method_context};

// hardwired lints from librustc
// Hardwired lints from librustc.
pub use lint::builtin::*;

declare_lint! {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
reference: "issue #57644 <https://github.com/rust-lang/rust/issues/57644>",
edition: None,
},
]);
]);

// Register renamed and removed lints.
store.register_renamed("single_use_lifetime", "single_use_lifetimes");
Expand Down
13 changes: 12 additions & 1 deletion src/librustc_typeck/astconv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ use std::collections::BTreeSet;
use std::iter;
use std::slice;

use crate::collect::{lint_duplicate_bounds, LintedBoundVar, LintedBoundValue};
use super::{check_type_alias_enum_variants_enabled};
use rustc_data_structures::fx::FxHashSet;

Expand Down Expand Up @@ -940,6 +941,7 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx> + 'o {
}

fn conv_object_ty_poly_trait_ref(&self,
node_id: ast::NodeId,
span: Span,
trait_bounds: &[hir::PolyTraitRef],
lifetime: &hir::Lifetime)
Expand Down Expand Up @@ -980,6 +982,15 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx> + 'o {
.emit();
}

// Lint duplicate bounds.
{
let bounds = trait_bounds.iter().filter_map(|b| {
let trait_did = b.trait_ref.trait_def_id();
Some((LintedBoundVar::SelfTy, LintedBoundValue::DefId(trait_did), b.span))
});
lint_duplicate_bounds(tcx, node_id, bounds);
}

// Check that there are no gross object safety violations;
// most importantly, that the supertraits don't contain `Self`,
// to avoid ICEs.
Expand Down Expand Up @@ -1767,7 +1778,7 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx> + 'o {
tcx.mk_fn_ptr(self.ty_of_fn(bf.unsafety, bf.abi, &bf.decl))
}
hir::TyKind::TraitObject(ref bounds, ref lifetime) => {
self.conv_object_ty_poly_trait_ref(ast_ty.span, bounds, lifetime)
self.conv_object_ty_poly_trait_ref(ast_ty.id, ast_ty.span, bounds, lifetime)
}
hir::TyKind::Path(hir::QPath::Resolved(ref maybe_qself, ref path)) => {
debug!("ast_ty_to_ty: maybe_qself={:?} path={:?}", maybe_qself, path);
Expand Down
103 changes: 89 additions & 14 deletions src/librustc_typeck/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@ use middle::lang_items::SizedTraitLangItem;
use middle::resolve_lifetime as rl;
use middle::weak_lang_items;
use rustc::mir::mono::Linkage;
use rustc::ty::{self, AdtKind, Binder, Predicate, ToPolyTraitRef, Ty, TyCtxt};
use rustc::ty::query::Providers;
use rustc::ty::query::queries;
use rustc::ty::subst::Substs;
use rustc::ty::util::Discr;
use rustc::ty::util::IntTypeExt;
use rustc::ty::{self, AdtKind, ToPolyTraitRef, Ty, TyCtxt};
use rustc::ty::util::{Discr, IntTypeExt};
use rustc::ty::{ReprOptions, ToPredicate};
use rustc::util::captures::Captures;
use rustc::util::nodemap::FxHashMap;
Expand All @@ -50,9 +49,65 @@ use rustc::hir::GenericParamKind;
use rustc::hir::{self, CodegenFnAttrFlags, CodegenFnAttrs, Unsafety};

use std::iter;
use std::ops::Range;

struct OnlySelfBounds(bool);

#[derive(Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum LintedBoundVar<'tcx> {
DefId(DefId),
SelfTy,
Ty(Binder<Ty<'tcx>>),
Region(Binder<ty::Region<'tcx>>),
}

#[derive(Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum LintedBoundValue<'tcx> {
DefId(DefId),
Ty(Binder<Ty<'tcx>>),
Region(Binder<ty::Region<'tcx>>),
}

pub fn lint_duplicate_bounds<'a, 'gcx, 'tcx>(
tcx: TyCtxt<'a, 'gcx, 'tcx>,
node_id: ast::NodeId,
bounds: impl IntoIterator<Item = (LintedBoundVar<'tcx>, LintedBoundValue<'tcx>, Span)>,
) {
let mut bounds: Vec<_> = bounds.into_iter().collect();
bounds.sort_unstable();

let emit_lint = |range: Range<usize>| {
let bound_name = tcx.sess.source_map()
.span_to_snippet(bounds[range.start].2).unwrap();
let mut err = tcx.struct_span_lint_node(
lint::builtin::DUPLICATE_BOUNDS,
node_id,
bounds[range.clone()].iter().map(|(_, _, sp)| *sp).collect::<Vec<_>>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bounds[range.clone()].iter().map(|(_, _, sp)| *sp).collect::<Vec<_>>(),
bounds[range.clone()].iter().map(|(.., sp)| *sp).collect::<Vec<_>>(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's clearer this way.

&format!("duplicate bound `{}` found",
bound_name));
debug!("zzz: 1: {:?} / {:?}", bounds[range.start].0, bounds[range.start].1);
err.span_label(bounds[range.start].2, "first use of bound");
for i in (range.start + 1)..range.end {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not get a subslice and iterate over that + pattern match out the parts of the tuple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's important to have the indices, as you can see from the following code.

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 .enumerate() to get the indices :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but the thing is I'm keeping track of the start index of a "run"... this sort of made most sense to me. shrug

debug!("zzz: 2: {:?} / {:?}", bounds[i].0, bounds[i].1);
err.span_label(bounds[i].2, "subsequent use of bound");
}
err.emit();
};

let mut seq_start = 0;
for i in 1..bounds.len() {
if (&bounds[i].0, &bounds[i].1) != (&bounds[i - 1].0, &bounds[i - 1].1) {
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 tuple_windows from itertools if available to express this more nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If only... introducing dependencies on itertools in the compiler is a big no no.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bummer... I would have expected it to already be used somewhere... :/

if i - seq_start > 1 {
emit_lint(seq_start..i);
}
seq_start = i;
}
}
if bounds.len() - seq_start > 1 {
emit_lint(seq_start..bounds.len());
}
}

///////////////////////////////////////////////////////////////////////////
// Main entry point

Expand Down Expand Up @@ -336,7 +391,7 @@ impl<'a, 'tcx> ItemCtxt<'a, 'tcx> {
param_id: ast::NodeId,
ty: Ty<'tcx>,
only_self_bounds: OnlySelfBounds,
) -> Vec<(ty::Predicate<'tcx>, Span)> {
) -> Vec<(Predicate<'tcx>, Span)> {
let from_ty_params = ast_generics
.params
.iter()
Expand Down Expand Up @@ -726,7 +781,7 @@ fn super_predicates_of<'a, 'tcx>(
// which will, in turn, reach indirect supertraits.
for &(pred, span) in &superbounds {
debug!("superbound: {:?}", pred);
if let ty::Predicate::Trait(bound) = pred {
if let Predicate::Trait(bound) = pred {
tcx.at(span).super_predicates_of(bound.def_id());
}
}
Expand Down Expand Up @@ -1672,8 +1727,8 @@ fn explicit_predicates_of<'a, 'tcx>(
/// Preserving the order of insertion is important here so as not to break
/// compile-fail UI tests.
struct UniquePredicates<'tcx> {
predicates: Vec<(ty::Predicate<'tcx>, Span)>,
uniques: FxHashSet<(ty::Predicate<'tcx>, Span)>,
predicates: Vec<(Predicate<'tcx>, Span)>,
uniques: FxHashSet<(Predicate<'tcx>, Span)>,
}

impl<'tcx> UniquePredicates<'tcx> {
Expand All @@ -1684,13 +1739,13 @@ fn explicit_predicates_of<'a, 'tcx>(
}
}

fn push(&mut self, value: (ty::Predicate<'tcx>, Span)) {
fn push(&mut self, value: (Predicate<'tcx>, Span)) {
if self.uniques.insert(value) {
self.predicates.push(value);
}
}

fn extend<I: IntoIterator<Item = (ty::Predicate<'tcx>, Span)>>(&mut self, iter: I) {
fn extend<I: IntoIterator<Item = (Predicate<'tcx>, Span)>>(&mut self, iter: I) {
for value in iter {
self.push(value);
}
Expand Down Expand Up @@ -1884,7 +1939,7 @@ fn explicit_predicates_of<'a, 'tcx>(
let span = bound_pred.bounded_ty.span;
let predicate = ty::OutlivesPredicate(ty, tcx.mk_region(ty::ReEmpty));
predicates.push(
(ty::Predicate::TypeOutlives(ty::Binder::dummy(predicate)), span)
(Predicate::TypeOutlives(ty::Binder::dummy(predicate)), span)
);
}
}
Expand All @@ -1910,7 +1965,7 @@ fn explicit_predicates_of<'a, 'tcx>(
&hir::GenericBound::Outlives(ref lifetime) => {
let region = AstConv::ast_region_to_region(&icx, lifetime, None);
let pred = ty::Binder::bind(ty::OutlivesPredicate(ty, region));
predicates.push((ty::Predicate::TypeOutlives(pred), lifetime.span))
predicates.push((Predicate::TypeOutlives(pred), lifetime.span))
}
}
}
Expand All @@ -1927,7 +1982,7 @@ fn explicit_predicates_of<'a, 'tcx>(
};
let pred = ty::Binder::bind(ty::OutlivesPredicate(r1, r2));

(ty::Predicate::RegionOutlives(pred), span)
(Predicate::RegionOutlives(pred), span)
}))
}

Expand Down Expand Up @@ -1983,6 +2038,26 @@ fn explicit_predicates_of<'a, 'tcx>(
);
}

// Lint duplicate bounds.
let bounds = predicates.iter().filter_map(|(pred, sp)| {
match pred {
Predicate::Trait(ref pred) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Use default match bindings?

Some((LintedBoundVar::Ty(pred.self_ty()),
LintedBoundValue::DefId(pred.def_id()), *sp)),
Predicate::RegionOutlives(ref pred) =>
Some((LintedBoundVar::Region(pred.var()),
LintedBoundValue::Region(pred.value()), *sp)),
Predicate::TypeOutlives(ref pred) =>
Some((LintedBoundVar::Ty(pred.var()),
LintedBoundValue::Region(pred.value()), *sp)),
Predicate::Projection(ref pred) =>
Some((LintedBoundVar::DefId(pred.item_def_id()),
LintedBoundValue::Ty(pred.ty()), *sp)),
_ => None,
}
});
lint_duplicate_bounds(tcx, node_id, bounds);

let result = Lrc::new(ty::GenericPredicates {
parent: generics.parent,
predicates,
Expand Down Expand Up @@ -2062,7 +2137,7 @@ fn predicates_from_bound<'tcx>(
astconv: &dyn AstConv<'tcx, 'tcx>,
param_ty: Ty<'tcx>,
bound: &hir::GenericBound,
) -> Vec<(ty::Predicate<'tcx>, Span)> {
) -> Vec<(Predicate<'tcx>, Span)> {
match *bound {
hir::GenericBound::Trait(ref tr, hir::TraitBoundModifier::None) => {
let mut projections = Vec::new();
Expand All @@ -2076,7 +2151,7 @@ fn predicates_from_bound<'tcx>(
hir::GenericBound::Outlives(ref lifetime) => {
let region = astconv.ast_region_to_region(lifetime, None);
let pred = ty::Binder::bind(ty::OutlivesPredicate(param_ty, region));
vec![(ty::Predicate::TypeOutlives(pred), lifetime.span)]
vec![(Predicate::TypeOutlives(pred), lifetime.span)]
}
hir::GenericBound::Trait(_, hir::TraitBoundModifier::Maybe) => vec![],
}
Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/lint/lint-incoherent-auto-trait-objects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ impl Foo for dyn Send {}
impl Foo for dyn Send + Send {}
//~^ ERROR conflicting implementations
//~| hard error
//~^^^ WARNING duplicate auto trait `Send` found in type parameter bounds [duplicate_auto_traits_in_bounds]

impl Foo for dyn Send + Sync {}

Expand All @@ -17,5 +18,6 @@ impl Foo for dyn Sync + Send {}
impl Foo for dyn Send + Sync + Send {}
//~^ ERROR conflicting implementations
//~| hard error
//~^^^ WARNING duplicate auto trait `Send` found in type parameter bounds [duplicate_auto_traits_in_bounds]

fn main() {}
18 changes: 18 additions & 0 deletions src/test/ui/lint/lint-incoherent-auto-trait-objects.stderr
Original file line number Diff line number Diff line change
@@ -1,3 +1,21 @@
warning: duplicate auto trait `Send` found in type parameter bounds
--> $DIR/lint-incoherent-auto-trait-objects.rs:7:18
|
LL | impl Foo for dyn Send + Send {}
| ^^^^ ^^^^ subsequent use of auto trait
| |
| first use of auto trait
|
= note: #[warn(duplicate_auto_traits_in_bounds)] on by default

warning: duplicate auto trait `Send` found in type parameter bounds
--> $DIR/lint-incoherent-auto-trait-objects.rs:18:18
|
LL | impl Foo for dyn Send + Sync + Send {}
| ^^^^ ^^^^ subsequent use of auto trait
| |
| first use of auto trait

error: conflicting implementations of trait `Foo` for type `(dyn std::marker::Send + 'static)`: (E0119)
--> $DIR/lint-incoherent-auto-trait-objects.rs:7:1
|
Expand Down
Loading