Skip to content

fix broken type parameter indexing logic in wfcheck #36119

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 1 commit into from
Sep 4, 2016
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
9 changes: 7 additions & 2 deletions src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -717,11 +717,16 @@ pub struct RegionParameterDef<'tcx> {

impl<'tcx> RegionParameterDef<'tcx> {
pub fn to_early_bound_region(&self) -> ty::Region {
ty::ReEarlyBound(ty::EarlyBoundRegion {
ty::ReEarlyBound(self.to_early_bound_region_data())
}

pub fn to_early_bound_region_data(&self) -> ty::EarlyBoundRegion {
ty::EarlyBoundRegion {
index: self.index,
name: self.name,
})
}
}

pub fn to_bound_region(&self) -> ty::BoundRegion {
// this is an early bound region, so unaffected by #32330
ty::BoundRegion::BrNamed(self.def_id, self.name, Issue32330::WontChange)
Expand Down
31 changes: 9 additions & 22 deletions src/librustc_typeck/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,44 +457,31 @@ impl<'ccx, 'gcx> CheckTypeWellFormedVisitor<'ccx, 'gcx> {
let variances = self.tcx().item_variances(item_def_id);

let mut constrained_parameters: FnvHashSet<_> =
variances[ast_generics.lifetimes.len()..]
.iter().enumerate()
variances.iter().enumerate()
.filter(|&(_, &variance)| variance != ty::Bivariant)
.map(|(index, _)| self.param_ty(ast_generics, index))
.map(|p| Parameter::Type(p))
.map(|(index, _)| Parameter(index as u32))
.collect();
Copy link
Member

@eddyb eddyb Aug 29, 2016

Choose a reason for hiding this comment

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

Could this just reuse a clone of the variances Vec instead of having a (heavier) FnvHashSet?
Only in the case of errors do we have less constrainted_parameters than generics, so a set doesn't make sense.

Copy link
Contributor Author

@arielb1 arielb1 Aug 29, 2016

Choose a reason for hiding this comment

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

I would prefer to use a strongly-typed BitVector everywhere (also in other places).


identify_constrained_type_params(ty_predicates.predicates.as_slice(),
None,
&mut constrained_parameters);

for (index, &variance) in variances.iter().enumerate() {
let (span, name) = if index < ast_generics.lifetimes.len() {
if variance != ty::Bivariant {
continue;
}
for (index, _) in variances.iter().enumerate() {
if constrained_parameters.contains(&Parameter(index as u32)) {
continue;
}

let (span, name) = if index < ast_generics.lifetimes.len() {
(ast_generics.lifetimes[index].lifetime.span,
ast_generics.lifetimes[index].lifetime.name)
} else {
let index = index - ast_generics.lifetimes.len();
let param_ty = self.param_ty(ast_generics, index);
if constrained_parameters.contains(&Parameter::Type(param_ty)) {
continue;
}
(ast_generics.ty_params[index].span, param_ty.name)
(ast_generics.ty_params[index].span,
ast_generics.ty_params[index].name)
};
self.report_bivariance(span, name);
}
}

fn param_ty(&self, ast_generics: &hir::Generics, index: usize) -> ty::ParamTy {
ty::ParamTy {
idx: index as u32,
name: ast_generics.ty_params[index].name
}
}

fn report_bivariance(&self,
span: Span,
param_name: ast::Name)
Expand Down
24 changes: 10 additions & 14 deletions src/librustc_typeck/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2158,7 +2158,7 @@ fn enforce_impl_params_are_constrained<'a, 'tcx>(ccx: &CrateCtxt<'a, 'tcx>,
let ty_generics = generics_of_def_id(ccx, impl_def_id);
for (ty_param, param) in ty_generics.types.iter().zip(&generics.ty_params) {
let param_ty = ty::ParamTy::for_def(ty_param);
if !input_parameters.contains(&ctp::Parameter::Type(param_ty)) {
if !input_parameters.contains(&ctp::Parameter::from(param_ty)) {
Copy link
Member

Choose a reason for hiding this comment

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

Then again, typeck's use of ctp only wants something more like a BitVector.

report_unused_parameter(ccx, param.span, "type", &param_ty.to_string());
}
}
Expand Down Expand Up @@ -2189,23 +2189,19 @@ fn enforce_impl_lifetimes_are_constrained<'a, 'tcx>(ccx: &CrateCtxt<'a, 'tcx>,
ty::ConstTraitItem(..) | ty::MethodTraitItem(..) => None
})
.flat_map(|ty| ctp::parameters_for(&ty, true))
.filter_map(|p| match p {
ctp::Parameter::Type(_) => None,
ctp::Parameter::Region(r) => Some(r),
})
.collect();

for (index, lifetime_def) in ast_generics.lifetimes.iter().enumerate() {
let region = ty::EarlyBoundRegion {
index: index as u32,
name: lifetime_def.lifetime.name
};
for (ty_lifetime, lifetime) in impl_scheme.generics.regions.iter()
.zip(&ast_generics.lifetimes)
{
let param = ctp::Parameter::from(ty_lifetime.to_early_bound_region_data());

if
lifetimes_in_associated_types.contains(&region) && // (*)
!input_parameters.contains(&ctp::Parameter::Region(region))
lifetimes_in_associated_types.contains(&param) && // (*)
!input_parameters.contains(&param)
{
report_unused_parameter(ccx, lifetime_def.lifetime.span,
"lifetime", &region.name.to_string());
report_unused_parameter(ccx, lifetime.lifetime.span,
"lifetime", &lifetime.lifetime.name.to_string());
}
}

Expand Down
24 changes: 17 additions & 7 deletions src/librustc_typeck/constrained_type_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,14 @@ use rustc::ty::fold::{TypeFoldable, TypeVisitor};
use rustc::util::nodemap::FnvHashSet;

#[derive(Clone, PartialEq, Eq, Hash, Debug)]
pub enum Parameter {
Type(ty::ParamTy),
Region(ty::EarlyBoundRegion),
pub struct Parameter(pub u32);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please leave a comment explaining what this field is?

Also, I think the field should be private, and you should write fn from_index(usize) -> Parameter; it's always a shame when the public API doesn't expose usize, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But ParamTy has this as a u32.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, that's an internal conversion anyway...but I'd argue that's a problem with ParamTy. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway whatever. I don't care that much.


impl From<ty::ParamTy> for Parameter {
fn from(param: ty::ParamTy) -> Self { Parameter(param.idx) }
}

impl From<ty::EarlyBoundRegion> for Parameter {
fn from(param: ty::EarlyBoundRegion) -> Self { Parameter(param.index) }
}
Copy link
Member

Choose a reason for hiding this comment

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

Are these entirely needed? Seems to me like it's less work to just use the index from wherever it first is available.

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 had enough index mixups.


/// If `include_projections` is false, returns the list of parameters that are
Expand Down Expand Up @@ -49,8 +54,8 @@ impl<'tcx> TypeVisitor<'tcx> for ParameterCollector {
// projections are not injective
return false;
}
ty::TyParam(ref d) => {
self.parameters.push(Parameter::Type(d.clone()));
ty::TyParam(data) => {
self.parameters.push(Parameter::from(data));
}
_ => {}
}
Expand All @@ -61,7 +66,7 @@ impl<'tcx> TypeVisitor<'tcx> for ParameterCollector {
fn visit_region(&mut self, r: &'tcx ty::Region) -> bool {
match *r {
ty::ReEarlyBound(data) => {
self.parameters.push(Parameter::Region(data));
self.parameters.push(Parameter::from(data));
}
_ => {}
}
Expand Down Expand Up @@ -141,13 +146,15 @@ pub fn setup_constraining_predicates<'tcx>(predicates: &mut [ty::Predicate<'tcx>
// * <U as Iterator>::Item = T
// * T: Debug
// * U: Iterator
debug!("setup_constraining_predicates: predicates={:?} \
impl_trait_ref={:?} input_parameters={:?}",
predicates, impl_trait_ref, input_parameters);
let mut i = 0;
let mut changed = true;
while changed {
changed = false;

for j in i..predicates.len() {

if let ty::Predicate::Projection(ref poly_projection) = predicates[j] {
// Note that we can skip binder here because the impl
// trait ref never contains any late-bound regions.
Expand Down Expand Up @@ -181,5 +188,8 @@ pub fn setup_constraining_predicates<'tcx>(predicates: &mut [ty::Predicate<'tcx>
i += 1;
changed = true;
}
debug!("setup_constraining_predicates: predicates={:?} \
i={} impl_trait_ref={:?} input_parameters={:?}",
predicates, i, impl_trait_ref, input_parameters);
}
}
22 changes: 22 additions & 0 deletions src/test/run-pass/issue-36075.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2016 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

trait DeclarationParser {
type Declaration;
}

struct DeclarationListParser<'i, I, P>
where P: DeclarationParser<Declaration = I>
{
input: &'i (),
parser: P
}

fn main() {}