[WIP] Simplify mismatched types by removing same subtypes #39906
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -555,6 +555,155 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |
| } | ||
| } | ||
|
|
||
| fn highlight_outer(&self, | ||
| value: &mut Vec<(String, bool)>, | ||
| other_value: &mut Vec<(String, bool)>, | ||
| name: String, | ||
| sub: &ty::subst::Substs<'tcx>, | ||
| pos: usize, | ||
| other_ty: &ty::Ty<'tcx>) { | ||
| value.push((name, true)); | ||
| let len = sub.len(); | ||
| if len > 0 { | ||
| value.push(("<".to_string(), true)); | ||
| } | ||
|
|
||
| let sts = sub.regions(); | ||
| for (i, st) in sts.enumerate() { | ||
| value.push((format!("{}", st), false)); | ||
|
|
||
| if len > 0 && i != len - 1 { | ||
| value.push((format!(", "), false)); | ||
| } | ||
| } | ||
|
|
||
| let sts = sub.types(); | ||
| for (i, st) in sts.enumerate() { | ||
| if i == pos { | ||
| let (v, o_v) = self.cmp(st, other_ty); | ||
| value.extend(v); | ||
| other_value.extend(o_v); | ||
| } else { | ||
| value.push((format!("{}", st), true)); | ||
| } | ||
|
|
||
| if len > 0 && i != len - 1 { | ||
| value.push((format!(", "), true)); | ||
| } | ||
| } | ||
| if len > 0 { | ||
| value.push((">".to_string(), true)); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| fn cmp(&self, t1: ty::Ty<'tcx>, t2: ty::Ty<'tcx>) | ||
| -> (Vec<(String, bool)>, Vec<(String, bool)>) | ||
| { | ||
| match (&t1.sty, &t2.sty) { | ||
| (&ty::TyAdt(def1, sub1), &ty::TyAdt(def2, sub2)) => { | ||
| let mut values: (Vec<(String, bool)>, Vec<(String, bool)>) = (vec![], vec![]); | ||
|
There was a problem hiding this comment. does the bool here mean "highlighted"? seems worth a comment There was a problem hiding this comment. Indeed. I'll be cleaning these up with a new enum with two variants. |
||
| let name1 = self.tcx.item_path_str(def1.did.clone()); | ||
| let name2 = self.tcx.item_path_str(def2.did.clone()); | ||
| if name1 == name2 { | ||
|
There was a problem hiding this comment. seems strange to compare the string, rather than just comparing There was a problem hiding this comment. I think some comments here would be nice, btw... i.e., There was a problem hiding this comment. Yes, there's a dearth of documentation in this code so far. I believe at the time I was writing this I wasn't sure if dids for the same definition with different subtypes were the same (while intuitively would be) and I already had a need for the name string for presentation, so I did it like that. There was a problem hiding this comment. when you say "subtype", are you referring to the |
||
| // Easy case, replace same types with `_` to shorten the output | ||
| // and highlight only the differing types. | ||
| values.0.push((name1.to_string(), false)); | ||
| values.1.push((name2.to_string(), false)); | ||
|
|
||
| let len = sub1.len(); | ||
| if len > 0 { | ||
| values.0.push(("<".to_string(), false)); | ||
| values.1.push(("<".to_string(), false)); | ||
| } | ||
|
|
||
| let sts1 = sub1.regions(); | ||
| let sts2 = sub2.regions(); | ||
| let x = sts1.zip(sts2); | ||
| for (i, (st1, st2)) in x.enumerate() { | ||
| values.0.push((format!("{}", st1), st1 != st2)); | ||
| values.1.push((format!("{}", st2), st1 != st2)); | ||
|
There was a problem hiding this comment. Is there any case where type mismatch errors need to point at the lifetimes? There was a problem hiding this comment. I am working with @cengizio (slowly, slowly) on refactoring lifetime mismatch errors, but I thnk ideally they wil not go through this generic path, as they deserve a more focused explanation |
||
|
|
||
| if len > 0 && i != len - 1 { | ||
| values.0.push((format!(", "), false)); | ||
| values.1.push((format!(", "), false)); | ||
| } | ||
| } | ||
|
|
||
| let sts1 = sub1.types(); | ||
| let sts2 = sub2.types(); | ||
| let x = sts1.zip(sts2); | ||
| for (i, (st1, st2)) in x.enumerate() { | ||
| if st1 == st2 { | ||
| values.0.push(("_".to_string(), false)); | ||
| values.1.push(("_".to_string(), false)); | ||
| } else { | ||
| let (x1, x2) = self.cmp(st1, st2); | ||
| values.0.extend(x1); | ||
| values.1.extend(x2); | ||
| } | ||
| if len > 0 && i != len - 1 { | ||
| values.0.push((format!(", "), false)); | ||
| values.1.push((format!(", "), false)); | ||
|
There was a problem hiding this comment. Nit: could we avoid pushing There was a problem hiding this comment. I was thinking that maybe instead of building up strings, it'd be nice to build up a tree, and then have one routine that 'flattens' the tree into (string, bool) pairs at the end There was a problem hiding this comment. I like the suggestion. I'll look into it. Would that tree be used for anything else? One thing that worries me about my hacky approach so far is that there're now two places where a type's presentation string is created, instead of one. If we create a presentation tree, then we can compare tree against tree easily, and the printing is always performed the same way. That way the only custom code for this would be the setting nodes to highlight. Thoughts? There was a problem hiding this comment. I was wondering about the repeated code as well. I love the idea of creating a "presentation tree" for a type, actually. It might also help with things like presenting the error message "local" to a particular module (i.e., I'd like to stop using absolute path strings if we can) |
||
| } | ||
| } | ||
| if len > 0 { | ||
| values.0.push((">".to_string(), false)); | ||
| values.1.push((">".to_string(), false)); | ||
| } | ||
| values | ||
| } else { | ||
| // Check for simple composition | ||
|
There was a problem hiding this comment. what is "simple composition"? an example would be good There was a problem hiding this comment. The comments so far were meant for myself only, sorry for the lack of depth in them. By simple composition I meant cases like There was a problem hiding this comment. The output for this can be seen on the second and third diagnostic errors of my last comment. I'd appreciate feedback on it to know wether I should aim at a different type of output. The third error is purposely weird, to get the worst possible cases likely to appear in the wild, but I foresee the second one to be more representative. There was a problem hiding this comment. I see. The highlighting in those examples makes sense to me, I think it looks pretty good as is. It did take me a second to notice and interpret the second case (i.e., when one type starts out highlighted, seems a bit surprising, but it makes sense, ultimately). |
||
| for (i, st) in sub1.types().enumerate() { | ||
| if st == t2 { | ||
| self.highlight_outer(&mut values.0, &mut values.1, name1, sub1, i, &t2); | ||
|
There was a problem hiding this comment. again, an example of code that takes this branch would be great, I have no idea what this is doing :) There was a problem hiding this comment. These two for loops compare one type against the subtype of the other, to catch cases like There was a problem hiding this comment. OK. I figured it was something like that. Main thing is that I think that in code like this it's super helpful to walk through various examples of types to show what each path is handling. |
||
| return values; | ||
| } | ||
| if let &ty::TyAdt(def, _) = &st.sty { | ||
| let name = self.tcx.item_path_str(def.did.clone()); | ||
| if name == name2 { | ||
| self.highlight_outer(&mut values.0, | ||
| &mut values.1, | ||
| name1, | ||
| sub1, | ||
| i, | ||
| &t2); | ||
| return values; | ||
| } | ||
| } | ||
| } | ||
| for (i, st) in sub2.types().enumerate() { | ||
|
There was a problem hiding this comment. these two for loops feel like they are begging to be extracted into a fn...no? There was a problem hiding this comment. Yeah, they are. :) |
||
| if st == t1 { | ||
| self.highlight_outer(&mut values.1, &mut values.0, name2, sub2, i, &t1); | ||
| return values; | ||
| } | ||
| if let &ty::TyAdt(def, _) = &st.sty { | ||
| let name = self.tcx.item_path_str(def.did.clone()); | ||
| if name == name1 { | ||
| self.highlight_outer(&mut values.1, | ||
| &mut values.0, | ||
| name2, | ||
| sub2, | ||
| i, | ||
| &t1); | ||
| return values; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| (vec![(format!("{}", t1), true)], vec![(format!("{}", t2), true)]) | ||
| } | ||
| } | ||
| _ => { | ||
| if t1 == t2 { | ||
| (vec![("_".to_string(), false)], vec![("_".to_string(), false)]) | ||
| } else { | ||
| (vec![(format!("{}", t1), true)], vec![(format!("{}", t2), true)]) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn note_type_err(&self, | ||
| diag: &mut DiagnosticBuilder<'tcx>, | ||
| cause: &ObligationCause<'tcx>, | ||
|
|
@@ -665,26 +814,64 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |
| diag | ||
| } | ||
|
|
||
| /// Returns two `Vec`s representing portions of a type with a flag on wether it should | ||
|
There was a problem hiding this comment. Nit: s/wether/whether/ |
||
| /// be highlighted in the output. | ||
| /// | ||
| /// For given types `X<String, usize>` and `X<usize, usize>`, this method would return | ||
| /// | ||
| /// ```nocode | ||
| /// Some((vec![ | ||
| /// ("X", false), | ||
| /// ("<", false), | ||
| /// ("String", true), | ||
| /// (",", false), | ||
| /// ("_", false), | ||
| /// (">", false) | ||
| /// ], vec![ | ||
| /// ("X", false), | ||
| /// ("<", false), | ||
| /// ("usize", true), | ||
| /// (",", false), | ||
| /// ("_", false), | ||
| /// (">", false) | ||
| /// ])) | ||
| /// ``` | ||
| fn values_str(&self, values: &ValuePairs<'tcx>) | ||
| -> Option<(Vec<(String, bool)>, Vec<(String, bool)>)> | ||
| { | ||
| match *values { | ||
| infer::Types(ref exp_found) => self.expected_found_str_ty(exp_found), | ||
| infer::TraitRefs(ref exp_found) => self.expected_found_str(exp_found), | ||
| infer::PolyTraitRefs(ref exp_found) => self.expected_found_str(exp_found), | ||
| } | ||
| } | ||
|
|
||
| fn expected_found_str_ty( | ||
| &self, | ||
| exp_found: &ty::error::ExpectedFound<ty::Ty<'tcx>>) | ||
| -> Option<(Vec<(String, bool)>, Vec<(String, bool)>)> | ||
| { | ||
| let exp_found = self.resolve_type_vars_if_possible(exp_found); | ||
| if exp_found.references_error() { | ||
| return None; | ||
| } | ||
|
|
||
| Some(self.cmp(exp_found.expected, exp_found.found)) | ||
| } | ||
|
|
||
|
|
||
| fn expected_found_str<T: fmt::Display + TypeFoldable<'tcx>>( | ||
| &self, | ||
| exp_found: &ty::error::ExpectedFound<T>) | ||
| -> Option<(Vec<(String, bool)>, Vec<(String, bool)>)> | ||
| { | ||
| let exp_found = self.resolve_type_vars_if_possible(exp_found); | ||
| if exp_found.references_error() { | ||
| return None; | ||
| } | ||
|
|
||
| Some((vec![(format!("{}", exp_found.expected), true)], | ||
| vec![(format!("{}", exp_found.found), true)])) | ||
| } | ||
|
|
||
| fn report_generic_bound_failure(&self, | ||
|
|
@@ -1140,6 +1327,8 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { | |
| match *origin { | ||
| infer::Subtype(ref trace) => { | ||
| if let Some((expected, found)) = self.values_str(&trace.values) { | ||
| let expected = expected.iter().map(|x| x.0.to_owned()).collect::<String>(); | ||
| let found = found.iter().map(|x| x.0.to_owned()).collect::<String>(); | ||
| // FIXME: do we want a "the" here? | ||
| err.span_note( | ||
| trace.cause.span, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| // Copyright 2017 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. | ||
|
|
||
| enum Bar { | ||
| Qux, | ||
| Zar, | ||
| } | ||
|
|
||
| struct Foo { | ||
| bar: usize, | ||
| } | ||
|
|
||
| struct X<T1, T2> { | ||
| x: T1, | ||
| y: T2, | ||
| } | ||
|
|
||
| fn a() -> Foo { | ||
| Some(Foo { bar: 1 }) | ||
| } | ||
|
|
||
| fn b() -> Option<Foo> { | ||
| Foo { bar: 1 } | ||
| } | ||
|
|
||
| fn c() -> Result<Foo, Bar> { | ||
| Foo { bar: 1 } | ||
| } | ||
|
|
||
| fn d() -> X<X<String, String>, String> { | ||
| X { | ||
| x: X { | ||
| x: "".to_string(), | ||
| y: 2, | ||
| }, | ||
| y: 3, | ||
| } | ||
| } | ||
|
|
||
| fn e() -> X<X<String, String>, String> { | ||
| X { | ||
| x: X { | ||
| x: "".to_string(), | ||
| y: 2, | ||
| }, | ||
| y: "".to_string(), | ||
| } | ||
| } | ||
|
|
||
| fn main() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meta-comment: I've been talking to @cengizio about making this
error_reportingsection into a proper module, as I think it's starting to have a lot of code. It feels like this type-diffing stuff really wants to be its own module.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, this fn could really use some doc comments explaining its interface.