From 91eedfe18b9be80b69ef2f19f4b618b2968ba181 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 12 Feb 2015 12:48:01 -0500 Subject: [PATCH] Report errors for type parameters that are not constrained, either by variance or an associated type. --- src/librustc/middle/ty.rs | 7 + src/librustc_typeck/check/wf.rs | 150 ++++++++++++++++-- src/librustc_typeck/collect.rs | 56 +------ .../constrained_type_params.rs | 20 +-- src/librustc_typeck/lib.rs | 1 + src/test/compile-fail/issue-17904-2.rs | 16 ++ 6 files changed, 180 insertions(+), 70 deletions(-) create mode 100644 src/test/compile-fail/issue-17904-2.rs diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index 8618bde95fe..503def14fcd 100644 --- a/src/librustc/middle/ty.rs +++ b/src/librustc/middle/ty.rs @@ -2955,6 +2955,13 @@ impl<'tcx> TyS<'tcx> { assert_eq!(r, Some(self)); walker } + + pub fn as_opt_param_ty(&self) -> Option { + match self.sty { + ty::ty_param(ref d) => Some(d.clone()), + _ => None, + } + } } pub fn walk_ty<'tcx, F>(ty_root: Ty<'tcx>, mut f: F) diff --git a/src/librustc_typeck/check/wf.rs b/src/librustc_typeck/check/wf.rs index 94670305be7..da5dbb22608 100644 --- a/src/librustc_typeck/check/wf.rs +++ b/src/librustc_typeck/check/wf.rs @@ -10,21 +10,22 @@ use astconv::AstConv; use check::{FnCtxt, Inherited, blank_fn_ctxt, vtable, regionck}; +use constrained_type_params::identify_constrained_type_params; use CrateCtxt; use middle::region; -use middle::subst; +use middle::subst::{self, TypeSpace, FnSpace, ParamSpace, SelfSpace}; use middle::traits; use middle::ty::{self, Ty}; use middle::ty::liberate_late_bound_regions; use middle::ty_fold::{TypeFolder, TypeFoldable, super_fold_ty}; -use util::ppaux::Repr; +use util::ppaux::{Repr, UserString}; use std::collections::HashSet; use syntax::ast; use syntax::ast_util::{local_def}; use syntax::attr; use syntax::codemap::Span; -use syntax::parse::token; +use syntax::parse::token::{self, special_idents}; use syntax::visit; use syntax::visit::Visitor; @@ -38,6 +39,10 @@ impl<'ccx, 'tcx> CheckTypeWellFormedVisitor<'ccx, 'tcx> { CheckTypeWellFormedVisitor { ccx: ccx, cache: HashSet::new() } } + fn tcx(&self) -> &ty::ctxt<'tcx> { + self.ccx.tcx + } + /// Checks that the field types (in a struct def'n) or argument types (in an enum def'n) are /// well-formed, meaning that they do not require any constraints not declared in the struct /// definition itself. For example, this definition would be illegal: @@ -96,23 +101,29 @@ impl<'ccx, 'tcx> CheckTypeWellFormedVisitor<'ccx, 'tcx> { ast::ItemConst(..) => { self.check_item_type(item); } - ast::ItemStruct(ref struct_def, _) => { + ast::ItemStruct(ref struct_def, ref ast_generics) => { self.check_type_defn(item, |fcx| { vec![struct_variant(fcx, &**struct_def)] }); + + self.check_variances_for_type_defn(item, ast_generics); } - ast::ItemEnum(ref enum_def, _) => { + ast::ItemEnum(ref enum_def, ref ast_generics) => { self.check_type_defn(item, |fcx| { enum_variants(fcx, enum_def) }); + + self.check_variances_for_type_defn(item, ast_generics); } - ast::ItemTrait(..) => { + ast::ItemTrait(_, ref ast_generics, _, _) => { let trait_predicates = ty::lookup_predicates(ccx.tcx, local_def(item.id)); reject_non_type_param_bounds( ccx.tcx, item.span, &trait_predicates); + self.check_variances(item, ast_generics, &trait_predicates, + self.tcx().lang_items.phantom_fn()); } _ => {} } @@ -280,6 +291,123 @@ impl<'ccx, 'tcx> CheckTypeWellFormedVisitor<'ccx, 'tcx> { } }); } + + fn check_variances_for_type_defn(&self, + item: &ast::Item, + ast_generics: &ast::Generics) + { + let item_def_id = local_def(item.id); + let predicates = ty::lookup_predicates(self.tcx(), item_def_id); + self.check_variances(item, + ast_generics, + &predicates, + self.tcx().lang_items.phantom_data()); + } + + fn check_variances(&self, + item: &ast::Item, + ast_generics: &ast::Generics, + ty_predicates: &ty::GenericPredicates<'tcx>, + suggested_marker_id: Option) + { + let variance_lang_items = &[ + self.tcx().lang_items.phantom_fn(), + self.tcx().lang_items.phantom_data(), + ]; + + let item_def_id = local_def(item.id); + let is_lang_item = variance_lang_items.iter().any(|n| *n == Some(item_def_id)); + if is_lang_item { + return; + } + + let variances = ty::item_variances(self.tcx(), item_def_id); + + let mut constrained_parameters: HashSet<_> = + variances.types + .iter_enumerated() + .filter(|&(_, _, &variance)| variance != ty::Bivariant) + .map(|(space, index, _)| self.param_ty(ast_generics, space, index)) + .collect(); + + identify_constrained_type_params(self.tcx(), + ty_predicates.predicates.as_slice(), + None, + &mut constrained_parameters); + + for (space, index, _) in variances.types.iter_enumerated() { + let param_ty = self.param_ty(ast_generics, space, index); + if constrained_parameters.contains(¶m_ty) { + continue; + } + let span = self.ty_param_span(ast_generics, item, space, index); + self.report_bivariance(span, param_ty.name, suggested_marker_id); + } + + for (space, index, &variance) in variances.regions.iter_enumerated() { + if variance != ty::Bivariant { + continue; + } + + assert_eq!(space, TypeSpace); + let span = ast_generics.lifetimes[index].lifetime.span; + let name = ast_generics.lifetimes[index].lifetime.name; + self.report_bivariance(span, name, suggested_marker_id); + } + } + + fn param_ty(&self, + ast_generics: &ast::Generics, + space: ParamSpace, + index: usize) + -> ty::ParamTy + { + let name = match space { + TypeSpace => ast_generics.ty_params[index].ident.name, + SelfSpace => special_idents::type_self.name, + FnSpace => self.tcx().sess.bug("Fn space occupied?"), + }; + + ty::ParamTy { space: space, idx: index as u32, name: name } + } + + fn ty_param_span(&self, + ast_generics: &ast::Generics, + item: &ast::Item, + space: ParamSpace, + index: usize) + -> Span + { + match space { + TypeSpace => ast_generics.ty_params[index].span, + SelfSpace => item.span, + FnSpace => self.tcx().sess.span_bug(item.span, "Fn space occupied?"), + } + } + + fn report_bivariance(&self, + span: Span, + param_name: ast::Name, + suggested_marker_id: Option) + { + self.tcx().sess.span_err( + span, + &format!("parameter `{}` is never used", + param_name.user_string(self.tcx()))[]); + + match suggested_marker_id { + Some(def_id) => { + self.tcx().sess.span_help( + span, + format!("consider removing `{}` or using a marker such as `{}`", + param_name.user_string(self.tcx()), + ty::item_path_str(self.tcx(), def_id)).as_slice()); + } + None => { + // no lang items, no help! + } + } + } } // Reject any predicates that do not involve a type parameter. @@ -347,9 +475,9 @@ impl<'ccx, 'tcx, 'v> Visitor<'v> for CheckTypeWellFormedVisitor<'ccx, 'tcx> { match fk { visit::FkFnBlock | visit::FkItemFn(..) => {} visit::FkMethod(..) => { - match ty::impl_or_trait_item(self.ccx.tcx, local_def(id)) { + match ty::impl_or_trait_item(self.tcx(), local_def(id)) { ty::ImplOrTraitItem::MethodTraitItem(ty_method) => { - reject_shadowing_type_parameters(self.ccx.tcx, span, &ty_method.generics) + reject_shadowing_type_parameters(self.tcx(), span, &ty_method.generics) } _ => {} } @@ -363,14 +491,14 @@ impl<'ccx, 'tcx, 'v> Visitor<'v> for CheckTypeWellFormedVisitor<'ccx, 'tcx> { &ast::TraitItem::ProvidedMethod(_) | &ast::TraitItem::TypeTraitItem(_) => {}, &ast::TraitItem::RequiredMethod(ref method) => { - match ty::impl_or_trait_item(self.ccx.tcx, local_def(method.id)) { + match ty::impl_or_trait_item(self.tcx(), local_def(method.id)) { ty::ImplOrTraitItem::MethodTraitItem(ty_method) => { reject_non_type_param_bounds( - self.ccx.tcx, + self.tcx(), method.span, &ty_method.predicates); reject_shadowing_type_parameters( - self.ccx.tcx, + self.tcx(), method.span, &ty_method.generics); } diff --git a/src/librustc_typeck/collect.rs b/src/librustc_typeck/collect.rs index 44e850a0738..32679c8967e 100644 --- a/src/librustc_typeck/collect.rs +++ b/src/librustc_typeck/collect.rs @@ -87,6 +87,7 @@ There are some shortcomings in this design: use astconv::{self, AstConv, ty_of_arg, ast_ty_to_ty, ast_region_to_region}; use middle::def; +use constrained_type_params::identify_constrained_type_params; use middle::lang_items::SizedTraitLangItem; use middle::region; use middle::resolve_lifetime; @@ -1960,51 +1961,15 @@ fn enforce_impl_ty_params_are_constrained<'tcx>(tcx: &ty::ctxt<'tcx>, 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 + .chain(Some(impl_scheme.ty).iter()) // Self type, always .flat_map(|t| t.walk()) - .filter_map(to_opt_param_ty) + .filter_map(|t| t.as_opt_param_ty()) .collect(); - loop { - let num_inputs = input_parameters.len(); - - let projection_predicates = - impl_predicates.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; - } - } + identify_constrained_type_params(tcx, + impl_predicates.predicates.as_slice(), + impl_trait_ref, + &mut input_parameters); for (index, ty_param) in ast_generics.ty_params.iter().enumerate() { let param_ty = ty::ParamTy { space: TypeSpace, @@ -2025,11 +1990,4 @@ fn enforce_impl_ty_params_are_constrained<'tcx>(tcx: &ty::ctxt<'tcx>, } } } - - fn to_opt_param_ty<'tcx>(ty: Ty<'tcx>) -> Option { - match ty.sty { - ty::ty_param(ref d) => Some(d.clone()), - _ => None, - } - } } diff --git a/src/librustc_typeck/constrained_type_params.rs b/src/librustc_typeck/constrained_type_params.rs index f40add8e2c0..83d7e985000 100644 --- a/src/librustc_typeck/constrained_type_params.rs +++ b/src/librustc_typeck/constrained_type_params.rs @@ -23,16 +23,16 @@ pub fn identify_constrained_type_params<'tcx>(_tcx: &ty::ctxt<'tcx>, let projection_predicates = 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, - } - }); + .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 diff --git a/src/librustc_typeck/lib.rs b/src/librustc_typeck/lib.rs index 7498dc8179d..b5dca0bd4f6 100644 --- a/src/librustc_typeck/lib.rs +++ b/src/librustc_typeck/lib.rs @@ -123,6 +123,7 @@ mod check; mod rscope; mod astconv; mod collect; +mod constrained_type_params; mod coherence; mod variance; diff --git a/src/test/compile-fail/issue-17904-2.rs b/src/test/compile-fail/issue-17904-2.rs new file mode 100644 index 00000000000..a33ec23a16a --- /dev/null +++ b/src/test/compile-fail/issue-17904-2.rs @@ -0,0 +1,16 @@ +// Copyright 2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Test that we can parse a unit struct with a where clause, even if +// it leads to a error later on since `T` is unused. + +struct Foo where T: Copy; //~ ERROR parameter `T` is never used + +fn main() {}