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.

Sign up for GitHub

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

Jump to bottom

Replace the obligation forest with a graph #33491

Merged
merged 5 commits into from
May 17, 2016
Merged

Replace the obligation forest with a graph #33491

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
957500b
rewrite obligation forest. cycles still handled incorrectly.
May 5, 2016
5c39a2a
add cycle-reporting logic
arielb1 May 7, 2016
f0f5ef5
address review comments
arielb1 May 12, 2016
5458d8b
rewrite fuzzy `on_unimplemented` matching to avoid ICEs
arielb1 May 14, 2016
65ad935
change on_unimplented logic
arielb1 May 16, 2016
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
214 changes: 33 additions & 181 deletions src/librustc/traits/error_reporting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ use super::{

use fmt_macros::{Parser, Piece, Position};
use hir::def_id::DefId;
use infer::{InferCtxt, TypeOrigin};
use ty::{self, ToPredicate, ToPolyTraitRef, TraitRef, Ty, TyCtxt, TypeFoldable, TypeVariants};
use infer::{InferCtxt};
use ty::{self, ToPredicate, ToPolyTraitRef, Ty, TyCtxt, TypeFoldable};
use ty::fast_reject;
use ty::fold::TypeFolder;
use ty::subst::{self, ParamSpace, Subst};
use ty::subst::{self, Subst};
use util::nodemap::{FnvHashMap, FnvHashSet};

use std::cmp;
Expand Down Expand Up @@ -61,128 +61,6 @@ impl<'a, 'gcx, 'tcx> TraitErrorKey<'tcx> {
}
}

// Enum used to differentiate the "big" and "little" weights.
enum Weight {
Coarse,
Precise,
}

trait AssociatedWeight {
fn get_weight(&self) -> (u32, u32);
}

impl<'a> AssociatedWeight for TypeVariants<'a> {
// Left number is for "global"/"big" weight and right number is for better precision.
fn get_weight(&self) -> (u32, u32) {
match *self {
TypeVariants::TyBool => (1, 1),
TypeVariants::TyChar => (1, 2),
TypeVariants::TyStr => (1, 3),

TypeVariants::TyInt(_) => (2, 1),
TypeVariants::TyUint(_) => (2, 2),
TypeVariants::TyFloat(_) => (2, 3),
TypeVariants::TyRawPtr(_) => (2, 4),

TypeVariants::TyEnum(_, _) => (3, 1),
TypeVariants::TyStruct(_, _) => (3, 2),
TypeVariants::TyBox(_) => (3, 3),
TypeVariants::TyTuple(_) => (3, 4),

TypeVariants::TyArray(_, _) => (4, 1),
TypeVariants::TySlice(_) => (4, 2),

TypeVariants::TyRef(_, _) => (5, 1),
TypeVariants::TyFnDef(_, _, _) => (5, 2),
TypeVariants::TyFnPtr(_) => (5, 3),

TypeVariants::TyTrait(_) => (6, 1),

TypeVariants::TyClosure(_, _) => (7, 1),

TypeVariants::TyProjection(_) => (8, 1),
TypeVariants::TyParam(_) => (8, 2),
TypeVariants::TyInfer(_) => (8, 3),

TypeVariants::TyError => (9, 1),
}
}
}

// The "closer" the types are, the lesser the weight.
fn get_weight_diff(a: &ty::TypeVariants, b: &TypeVariants, weight: Weight) -> u32 {
let (w1, w2) = match weight {
Weight::Coarse => (a.get_weight().0, b.get_weight().0),
Weight::Precise => (a.get_weight().1, b.get_weight().1),
};
if w1 < w2 {
w2 - w1
} else {
w1 - w2
}
}

// Once we have "globally matching" types, we need to run another filter on them.
//
// In the function `get_best_matching_type`, we got the types which might fit the
// most to the type we're looking for. This second filter now intends to get (if
// possible) the type which fits the most.
//
// For example, the trait expects an `usize` and here you have `u32` and `i32`.
// Obviously, the "correct" one is `u32`.
fn filter_matching_types<'tcx>(weights: &[(usize, u32)],
imps: &[(DefId, subst::Substs<'tcx>)],
trait_types: &[ty::Ty<'tcx>])
-> usize {
let matching_weight = weights[0].1;
let iter = weights.iter().filter(|&&(_, weight)| weight == matching_weight);
let mut filtered_weights = vec!();

for &(pos, _) in iter {
let mut weight = 0;
for (type_to_compare, original_type) in imps[pos].1
.types
.get_slice(ParamSpace::TypeSpace)
.iter()
.zip(trait_types.iter()) {
weight += get_weight_diff(&type_to_compare.sty, &original_type.sty, Weight::Precise);
}
filtered_weights.push((pos, weight));
}
filtered_weights.sort_by(|a, b| a.1.cmp(&b.1));
filtered_weights[0].0
}

// Here, we run the "big" filter. Little example:
//
// We receive a `String`, an `u32` and an `i32`.
// The trait expected an `usize`.
// From human point of view, it's easy to determine that `String` doesn't correspond to
// the expected type at all whereas `u32` and `i32` could.
//
// This first filter intends to only keep the types which match the most.
fn get_best_matching_type<'tcx>(imps: &[(DefId, subst::Substs<'tcx>)],
trait_types: &[ty::Ty<'tcx>]) -> usize {
let mut weights = vec!();
for (pos, imp) in imps.iter().enumerate() {
let mut weight = 0;
for (type_to_compare, original_type) in imp.1
.types
.get_slice(ParamSpace::TypeSpace)
.iter()
.zip(trait_types.iter()) {
weight += get_weight_diff(&type_to_compare.sty, &original_type.sty, Weight::Coarse);
}
weights.push((pos, weight));
}
weights.sort_by(|a, b| a.1.cmp(&b.1));
if weights[0].1 == weights[1].1 {
filter_matching_types(&weights, &imps, trait_types)
} else {
weights[0].0
}
}

impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
pub fn report_fulfillment_errors(&self, errors: &Vec<FulfillmentError<'tcx>>) {
for error in errors {
Expand Down Expand Up @@ -272,72 +150,46 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
substs
}

fn get_current_failing_impl(&self,
trait_ref: &TraitRef<'tcx>,
obligation: &PredicateObligation<'tcx>)
-> Option<(DefId, subst::Substs<'tcx>)> {
let simp = fast_reject::simplify_type(self.tcx,
trait_ref.self_ty(),
true);
let trait_def = self.tcx.lookup_trait_def(trait_ref.def_id);

match simp {
Some(_) => {
let mut matching_impls = Vec::new();
trait_def.for_each_impl(self.tcx, |def_id| {
let imp = self.tcx.impl_trait_ref(def_id).unwrap();
let substs = self.impl_substs(def_id, obligation.clone());
let imp = imp.subst(self.tcx, &substs);

if self.eq_types(true,
TypeOrigin::Misc(obligation.cause.span),
trait_ref.self_ty(),
imp.self_ty()).is_ok() {
matching_impls.push((def_id, imp.substs.clone()));
}
});
if matching_impls.len() == 0 {
None
} else if matching_impls.len() == 1 {
Some(matching_impls[0].clone())
} else {
let end = trait_ref.input_types().len() - 1;
// we need to determine which type is the good one!
Some(matching_impls[get_best_matching_type(&matching_impls,
&trait_ref.input_types()[0..end])]
.clone())
fn impl_with_self_type_of(&self,
trait_ref: ty::PolyTraitRef<'tcx>,
obligation: &PredicateObligation<'tcx>)
-> Option<DefId>
{
let tcx = self.tcx;
let mut result = None;
let mut ambiguous = false;

let trait_self_ty = tcx.erase_late_bound_regions(&trait_ref).self_ty();

self.tcx.lookup_trait_def(trait_ref.def_id())
.for_each_relevant_impl(self.tcx, trait_self_ty, |def_id| {
Copy link
Contributor

Choose a reason for hiding this comment

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

so, while I do prefer this simplified approach, it seems like it would mean that vec[3_i32] will fail to get the "helpful message" we were shooting for? (I could be confused; I'll kick off a local build and check it out)

Copy link
Contributor Author

@arielb1 arielb1 May 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure how the earlier approach worked for that. EDIT: that never worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both approaches do not report a helpful error message when a Vec is involved. Unless we are willing to add a "useless" impl Index<usize> for Vec, I don't see how this can be solved.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I just mean slice, then. There is this test: check_on_unimplemented_on_slice.rs which fails.

The fact that this doesn't work for vec (if indeed true) suggests that we may want to tie the hint to the use of the [] syntax rather than having a more generic mechanism, honestly.

I tend to think that the "on unimplemented" mechanism may need some work in any case. For example, I think the hint would be a bit confusing when you have X: Index<u32> and you supply a slice. (I have often found the "on unimplemented" messages from other traits confusing for this reason.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's just my logic being stupid. I have a fix to that.

let impl_self_ty = tcx
.impl_trait_ref(def_id)
.unwrap()
.self_ty()
.subst(tcx, &self.impl_substs(def_id, obligation.clone()));

if let Ok(..) = self.can_equate(&trait_self_ty, &impl_self_ty) {
ambiguous = result.is_some();
result = Some(def_id);
}
},
None => None,
}
}
});

fn find_attr(&self,
def_id: DefId,
attr_name: &str)
-> Option<ast::Attribute> {
for item in self.tcx.get_attrs(def_id).iter() {
if item.check_name(attr_name) {
return Some(item.clone());
match result {
Some(def_id) if !ambiguous && tcx.has_attr(def_id, "rustc_on_unimplemented") => {
result
}
_ => None
}
None
}

fn on_unimplemented_note(&self,
trait_ref: ty::PolyTraitRef<'tcx>,
obligation: &PredicateObligation<'tcx>) -> Option<String> {
let def_id = self.impl_with_self_type_of(trait_ref, obligation)
.unwrap_or(trait_ref.def_id());
let trait_ref = trait_ref.skip_binder();
let def_id = match self.get_current_failing_impl(trait_ref, obligation) {
Some((def_id, _)) => {
if let Some(_) = self.find_attr(def_id, "rustc_on_unimplemented") {
def_id
} else {
trait_ref.def_id
}
},
None => trait_ref.def_id,
};

let span = obligation.cause.span;
let mut report = None;
for item in self.tcx.get_attrs(def_id).iter() {
Expand Down
64 changes: 0 additions & 64 deletions src/test/compile-fail/on_unimplemented.rs
View file
Open in desktop

This file was deleted.