Skip to content

Prohibit unused type parameters in impls. #20593

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

Merged
merged 4 commits into from
Jan 7, 2015
Merged
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
3 changes: 2 additions & 1 deletion src/libcollections/btree/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1417,7 +1417,7 @@ pub type MutTraversal<'a, K, V> = AbsTraversal<ElemsAndEdges<Zip<slice::Iter<'a,
/// An owning traversal over a node's entries and edges
pub type MoveTraversal<K, V> = AbsTraversal<MoveTraversalImpl<K, V>>;


#[old_impl_check]
impl<K, V, E, Impl: TraversalImpl<K, V, E>> Iterator for AbsTraversal<Impl> {
type Item = TraversalItem<K, V, E>;

Expand All @@ -1433,6 +1433,7 @@ impl<K, V, E, Impl: TraversalImpl<K, V, E>> Iterator for AbsTraversal<Impl> {
}
}

#[old_impl_check]
impl<K, V, E, Impl: TraversalImpl<K, V, E>> DoubleEndedIterator for AbsTraversal<Impl> {
fn next_back(&mut self) -> Option<TraversalItem<K, V, E>> {
let tail_is_edge = self.tail_is_edge;
Expand Down
1 change: 1 addition & 0 deletions src/libcollections/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#![feature(unsafe_destructor, slicing_syntax)]
#![feature(unboxed_closures)]
#![feature(old_orphan_check)]
#![feature(old_impl_check)]
#![feature(associated_types)]
#![no_std]

Expand Down
1 change: 1 addition & 0 deletions src/librustc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#![feature(rustc_diagnostic_macros)]
#![feature(unboxed_closures)]
#![feature(old_orphan_check)]
#![feature(old_impl_check)]
#![feature(associated_types)]

extern crate arena;
Expand Down
1 change: 1 addition & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,7 @@ impl LintPass for UnusedAttributes {

// FIXME: #19470 this shouldn't be needed forever
"old_orphan_check",
"old_impl_check",
];

static CRATE_ATTRS: &'static [&'static str] = &[
Expand Down
1 change: 1 addition & 0 deletions src/librustc/util/ppaux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1349,6 +1349,7 @@ impl<'tcx, T:Repr<'tcx>> Repr<'tcx> for ty::Binder<T> {
}
}

#[old_impl_check]
impl<'tcx, S, H, K, V> Repr<'tcx> for HashMap<K,V,H>
where K : Hash<S> + Eq + Repr<'tcx>,
V : Repr<'tcx>,
Expand Down
100 changes: 99 additions & 1 deletion src/librustc_typeck/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use middle::lang_items::SizedTraitLangItem;
use middle::region;
use middle::resolve_lifetime;
use middle::subst;
use middle::subst::{Substs};
use middle::subst::{Substs, TypeSpace};
use middle::ty::{AsPredicate, ImplContainer, ImplOrTraitItemContainer, TraitContainer};
use middle::ty::{self, RegionEscape, Ty, TypeScheme};
use middle::ty_fold::{self, TypeFolder, TypeFoldable};
Expand All @@ -47,6 +47,7 @@ use util::ppaux;
use util::ppaux::{Repr,UserString};
use write_ty_to_tcx;

use std::collections::HashSet;
use std::rc::Rc;

use syntax::abi;
Expand Down Expand Up @@ -644,6 +645,10 @@ fn convert(ccx: &CollectCtxt, it: &ast::Item) {
Some(selfty),
None);
}

enforce_impl_ty_params_are_constrained(ccx.tcx,
generics,
local_def(it.id));
},
ast::ItemTrait(_, _, _, ref trait_methods) => {
let trait_def = trait_def_of_item(ccx, it);
Expand Down Expand Up @@ -1605,3 +1610,96 @@ fn check_method_self_type<'a, 'tcx, RS:RegionScope>(
})
}
}

/// Checks that all the type parameters on an impl
fn enforce_impl_ty_params_are_constrained<'tcx>(tcx: &ty::ctxt<'tcx>,
ast_generics: &ast::Generics,
impl_def_id: ast::DefId)
{
let impl_scheme = ty::lookup_item_type(tcx, impl_def_id);
let impl_trait_ref = ty::impl_trait_ref(tcx, impl_def_id);

// The trait reference is an input, so find all type parameters
// reachable from there, to start (if this is an inherent impl,
// then just examine the self type).
let mut input_parameters: HashSet<_> =
impl_trait_ref.iter()
.flat_map(|t| t.input_types().iter()) // Types in trait ref, if any
.chain(Some(impl_scheme.ty).iter()) // Self type, always
.flat_map(|t| t.walk())
.filter_map(to_opt_param_ty)
.collect();

loop {
let num_inputs = input_parameters.len();

let mut projection_predicates =
impl_scheme.generics.predicates
.iter()
.filter_map(|predicate| {
match *predicate {
// Ignore higher-ranked binders. For the purposes
// of this check, they don't matter because they
// only affect named regions, and we're just
// concerned about type parameters here.
ty::Predicate::Projection(ref data) => Some(data.0.clone()),
_ => None,
}
});

for projection in projection_predicates {
// Special case: watch out for some kind of sneaky attempt
// to project out an associated type defined by this very trait.
if Some(projection.projection_ty.trait_ref.clone()) == impl_trait_ref {
continue;
}

let relies_only_on_inputs =
projection.projection_ty.trait_ref.input_types().iter()
.flat_map(|t| t.walk())
.filter_map(to_opt_param_ty)
.all(|t| input_parameters.contains(&t));

if relies_only_on_inputs {
input_parameters.extend(
projection.ty.walk().filter_map(to_opt_param_ty));
}
}

if input_parameters.len() == num_inputs {
break;
}
}

for (index, ty_param) in ast_generics.ty_params.iter().enumerate() {
let param_ty = ty::ParamTy { space: TypeSpace,
idx: index as u32,
name: ty_param.ident.name };
if !input_parameters.contains(&param_ty) {
if ty::has_attr(tcx, impl_def_id, "old_impl_check") {
tcx.sess.span_warn(
ty_param.span,
format!("the type parameter `{}` is not constrained by the \
impl trait, self type, or predicates",
param_ty.user_string(tcx)).as_slice());
} else {
tcx.sess.span_err(
ty_param.span,
format!("the type parameter `{}` is not constrained by the \
impl trait, self type, or predicates",
param_ty.user_string(tcx)).as_slice());
tcx.sess.span_help(
ty_param.span,
format!("you can temporarily opt out of this rule by placing \
the `#[old_impl_check]` attribute on the impl").as_slice());
}
}
}

fn to_opt_param_ty<'tcx>(ty: Ty<'tcx>) -> Option<ty::ParamTy> {
match ty.sty {
ty::ty_param(ref d) => Some(d.clone()),
_ => None,
}
}
}
4 changes: 4 additions & 0 deletions src/libserialize/collection_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ impl<
}
}

#[old_impl_check]
impl<
K: Encodable + Hash<X> + Eq,
V: Encodable,
Expand All @@ -175,6 +176,7 @@ impl<
}
}

#[old_impl_check]
impl<
K: Decodable + Hash<S> + Eq,
V: Decodable,
Expand All @@ -195,6 +197,7 @@ impl<
}
}

#[old_impl_check]
impl<
T: Encodable + Hash<X> + Eq,
X,
Expand All @@ -212,6 +215,7 @@ impl<
}
}

#[old_impl_check]
impl<
T: Decodable + Hash<S> + Eq,
S,
Expand Down
1 change: 1 addition & 0 deletions src/libserialize/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ Core encoding and decoding interfaces.
#![feature(macro_rules, default_type_params, phase, slicing_syntax, globs)]
#![feature(unboxed_closures)]
#![feature(associated_types)]
#![feature(old_impl_check)]

// test harness access
#[cfg(test)]
Expand Down
10 changes: 10 additions & 0 deletions src/libstd/collections/hash/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,7 @@ impl<K, V, M> SearchResult<K, V, M> {
}
}

#[old_impl_check]
impl<K: Eq + Hash<S>, V, S, H: Hasher<S>> HashMap<K, V, H> {
fn make_hash<X: ?Sized + Hash<S>>(&self, x: &X) -> SafeHash {
table::make_hash(&self.hasher, x)
Expand Down Expand Up @@ -517,6 +518,7 @@ impl<K: Hash + Eq, V> HashMap<K, V, RandomSipHasher> {
}
}

#[old_impl_check]
impl<K: Eq + Hash<S>, V, S, H: Hasher<S>> HashMap<K, V, H> {
/// Creates an empty hashmap which will use the given hasher to hash keys.
///
Expand Down Expand Up @@ -1191,6 +1193,7 @@ fn search_entry_hashed<'a, K, V, Q: ?Sized>(table: &'a mut RawTable<K,V>, hash:
}

#[stable]
#[old_impl_check]
impl<K: Eq + Hash<S>, V: PartialEq, S, H: Hasher<S>> PartialEq for HashMap<K, V, H> {
fn eq(&self, other: &HashMap<K, V, H>) -> bool {
if self.len() != other.len() { return false; }
Expand All @@ -1202,9 +1205,11 @@ impl<K: Eq + Hash<S>, V: PartialEq, S, H: Hasher<S>> PartialEq for HashMap<K, V,
}

#[stable]
#[old_impl_check]
impl<K: Eq + Hash<S>, V: Eq, S, H: Hasher<S>> Eq for HashMap<K, V, H> {}

#[stable]
#[old_impl_check]
impl<K: Eq + Hash<S> + Show, V: Show, S, H: Hasher<S>> Show for HashMap<K, V, H> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
try!(write!(f, "{{"));
Expand All @@ -1219,6 +1224,7 @@ impl<K: Eq + Hash<S> + Show, V: Show, S, H: Hasher<S>> Show for HashMap<K, V, H>
}

#[stable]
#[old_impl_check]
impl<K: Eq + Hash<S>, V, S, H: Hasher<S> + Default> Default for HashMap<K, V, H> {
#[stable]
fn default() -> HashMap<K, V, H> {
Expand All @@ -1227,6 +1233,7 @@ impl<K: Eq + Hash<S>, V, S, H: Hasher<S> + Default> Default for HashMap<K, V, H>
}

#[stable]
#[old_impl_check]
impl<K: Hash<S> + Eq, Q: ?Sized, V, S, H: Hasher<S>> Index<Q> for HashMap<K, V, H>
where Q: BorrowFrom<K> + Hash<S> + Eq
{
Expand All @@ -1239,6 +1246,7 @@ impl<K: Hash<S> + Eq, Q: ?Sized, V, S, H: Hasher<S>> Index<Q> for HashMap<K, V,
}

#[stable]
#[old_impl_check]
impl<K: Hash<S> + Eq, Q: ?Sized, V, S, H: Hasher<S>> IndexMut<Q> for HashMap<K, V, H>
where Q: BorrowFrom<K> + Hash<S> + Eq
{
Expand Down Expand Up @@ -1472,6 +1480,7 @@ impl<'a, Q: ?Sized + 'a + ToOwned<K>, K: 'a, V: 'a> VacantEntry<'a, Q, K, V> {
}

#[stable]
#[old_impl_check]
impl<K: Eq + Hash<S>, V, S, H: Hasher<S> + Default> FromIterator<(K, V)> for HashMap<K, V, H> {
fn from_iter<T: Iterator<Item=(K, V)>>(iter: T) -> HashMap<K, V, H> {
let lower = iter.size_hint().0;
Expand All @@ -1482,6 +1491,7 @@ impl<K: Eq + Hash<S>, V, S, H: Hasher<S> + Default> FromIterator<(K, V)> for Has
}

#[stable]
#[old_impl_check]
impl<K: Eq + Hash<S>, V, S, H: Hasher<S>> Extend<(K, V)> for HashMap<K, V, H> {
fn extend<T: Iterator<Item=(K, V)>>(&mut self, mut iter: T) {
for (k, v) in iter {
Expand Down
Loading