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

[WIP] Simplify mismatched types by removing same subtypes #39906

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@estebank
Copy link
Contributor

estebank commented Feb 17, 2017

Simplify mismatched types errors by replacing subtypes that are not
different with _, and highlighting only the subtypes that are
different.

Given a file

struct X<T1, T2> {
    x: T1,
    y: T2,
}

fn foo() -> X<X<String, String>, String> {
    X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
}

provide the following output

error[E0308]: mismatched types
 --> file.rs:6:5
  |
6 |     X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `std::string::String`, found {integer}
  |
  = note: expected type `X<X<_, std::string::String>, _>`
             found type `X<X<_, {integer}>, _>`

Relates to #21025.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 17, 2017

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 17, 2017

The code is a bit of a mess right now, but wanted to put it out there to get feedback.

CC @jonathandturner, @GuillaumeGomez.

@estebank estebank force-pushed the estebank:shorter-mismatched-types branch from 066798f to 9e8b6da Feb 17, 2017

@estebank estebank force-pushed the estebank:shorter-mismatched-types branch from 9e8b6da to 617f7af Feb 17, 2017

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Feb 17, 2017

Should not check-in submodule updates.

Idea seems nice to me. I’m not sure there’s necessity to both highlight the mismatching parts and remove the mismatching ones. I would just pick one of the two.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 17, 2017

I really appreciate this, nice work! I'm just curious: is the submodules update normal?

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 17, 2017

Should not check-in submodule updates.

I'm just curious: is the submodules update normal?

The submodule updates where accidental.

I’m not sure there’s necessity to both highlight the mismatching parts and remove the mismatching ones. I would just pick one of the two.

I feel that they complement each other. The removal helps the error stay short, while the highlighting points the eyes towards the problem. Something I haven't implemented yet is composition errors, but the same options appear. Consider the following:

error[E0308]: mismatched types
 --> file.rs:6:5
  |
6 |     X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `std::option::Option<_>`, found `X<_>`
  |
  = note: expected type `Some(X<X<std::string::String, usize>, std::string::String>)`
             found type `X<X<std::string::String, {integer}>, std::string::String>`

and

error[E0308]: mismatched types
 --> file.rs:6:5
  |
6 |     X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `std::option::Option<_>`, found `X<_>`
  |
  = note: expected type `Some(X<_>)`
             found type `X<_>`

The removal works even with plain text output, but the highlighting is still useful in these cases. In the linked issue there's a pathological example of a type error where having both would be convenient:

 expected `Session<Cap<(), Rec<Offer<_, Rcv<String, Choose<Snd<_, _>, Snd<_, Offer<_, Rcv<String, Choose<Snd<(), _>, _>>>>>>>>>, (), _>`,
    found `Session<Cap<(), Rec<Offer<_, Rcv<String, Choose<Snd<_, _>, Snd<_, Offer<_, Rcv<String, Choose<Snd<String, _>, _>>>>>>>>>, (), ()>`
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));

This comment has been minimized.

@estebank

estebank Feb 17, 2017

Contributor

Is there any case where type mismatch errors need to point at the lifetimes?

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

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

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Feb 17, 2017

Do we want to use _? As it already has several other meanings.
Bikeshedding: maybe, ... which generally means something left out.

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Feb 17, 2017

_ is fine. You can literally write X<X<_, ...>> pretty much everywhere but signatures in code.


I feel that they complement each other. The removal helps the error stay short, while the highlighting points the eyes towards the problem. Something I haven't implemented yet is composition errors, but the same options appear. Consider the following:

Let me repeat myself from that other issue. When the screen is a rainbow, adding more rainbow makes the rest of the rainbow less legible. Phrased another way, I only have two eyes (no idea about the rest of y'all), and my eyes can only look at so many colourful and otherwise attention grabbing things at the same time.

pathological example of a type error where having both would be convenient

It is pretty easy to construct pathological examples to demonstrate anything you want, where highlighting is useful (as is case in your example), where highlighting has no benefit whatsoever (because nobody can parse the error anyway, or everything is high-lit) and where highlighting is plain detrimental (every second token is high-lit).


Either way, this is the last time I’ll be arguing about this. I’ll accept whatever the outcome is and once I begin finding rustc’s output unbearable, I’ll just disable colouring by default.

@Eh2406

This comment has been minimized.

Copy link
Contributor

Eh2406 commented Feb 17, 2017

Ah, my bad. If the _ is being added in such a way that the result is a valid type pattern then _ is the correct symbol. +1

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 17, 2017

@nagisa I understand your concern, and appreciate the counterpoints, but would like to reply to some of the points directly.

It is pretty easy to construct pathological examples to demonstrate anything you want, where highlighting is useful (as is case in your example)

That example is a real word example, which means that at least some subset of developers will face this at one point or another. Optimizing for that case at the expense of the other cases would be detrimental, but I'm not advocating that.

where highlighting has no benefit whatsoever (because nobody can parse the error anyway, or everything is high-lit)

That is the current situation (in the beta channel), the entirety of the type in the expected/found errors is highlighted. If anything this change removes some highlighting from the output.

where highlighting is plain detrimental (every second symbol is high-lit).

You have a very fair point there.

Either way, this is the last time I’ll be arguing about this. I’ll accept whatever the outcome is and once I begin finding rustc’s output unbearable, I’ll just disable colouring by default.

And I'll do my best so that none of my PRs pushes you to do so :)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 17, 2017

@estebank I'm over the moon to see you working on this!

I am not sure how I feel about _. It is true that it is a legal thing to type; but it's also true that it means that when I see an _ in the output, I do not know if it is something that the compiler chose to elide, or something that the compiler doesn't know. I guess that in this case it doesn't actually make any difference: either way it is not the source of my problem. In any case, while I expected to see ..., I admit that I see advantages to _ -- in particular, having both _ and ... feels like it will just lead to confusion.

As for the question of highlighting, I personally think it's good, but maybe we can strike a compromise and highlight in a more subtle way? I guess that the range of colors available to us is pretty limited. As for the pathological error, I definitely encounter errors like that on occasion, and having something pull my eye to the relevant part is appreciated.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 17, 2017

I'm over the moon to see you working on this!

blush

I am not sure how I feel about _. It is true that it is a legal thing to type; but it's also true that it means that when I see an _ in the output, I do not know if it is something that the compiler chose to elide, or something that the compiler doesn't know.

If the compiler doesn't know it will probably appear on one type and not the other (which as it stands now will highlight the output), while if it was elided, it'd be elided in both types:

  = note: expected type `X<X<std::string::String, usize>, _>`
             found type `X<X<{integer}, _>, _>`

The case where the compiler doesn't know the type in neither type could be accounted for and highlighted always (while it'd be too subtle, that's one way of differentiating the two cases).

@estebank estebank force-pushed the estebank:shorter-mismatched-types branch 2 times, most recently from a7852a5 to 87993af Feb 17, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 18, 2017

@estebank

If the compiler doesn't know it will probably appear on one type and not the other

I see. Anyway I think it seems fine any which way, honestly. I haven't looked at the code at all yet though.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 19, 2017

Showing off the composition case:

enum Bar {
    Qux(usize),
    Zar(String, usize),
}

struct Foo {
    bar: usize,
}

struct X<T1, T2> {
    x: T1,
    y: T2,
}

fn foo() -> X<X<String, String>, String> {
    X{ x: X{x: "".to_string(), y: 2 as usize}, y: 3}
}

fn zar() -> Result<Option<Foo>, Bar> {
    Some(Foo { bar: 1 })
}

fn z() -> Result<Option<Result<Option<()>, ()>>, ()> {
    Some(())
}

fn main() {}

screen shot 2017-02-19 at 12 04 54

@estebank estebank force-pushed the estebank:shorter-mismatched-types branch from 87993af to 249245f Feb 19, 2017

Highlight and simplify mismatched types
Shorten mismatched types errors by replacing subtypes that are not
different with `_`, and highlighting only the subtypes that are
different.

Given a file

```rust
struct X<T1, T2> {
    x: T1,
    y: T2,
}

fn foo() -> X<X<String, String>, String> {
    X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
}

fn bar() -> Option<String> {
    "".to_string()
}
```

provide the following output

```rust
error[E0308]: mismatched types
  --> file.rs:6:5
   |
 6 |     X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `std::string::String`, found {integer}
   |
   = note: expected type `X<X<_, std::string::String>, _>`
                                 ^^^^^^^^^^^^^^^^^^^^  // < highlighted
              found type `X<X<_, {integer}>, _>`
                                 ^^^^^^^^^             // < highlighted

error[E0308]: mismatched types
  --> file.rs:6:5
   |
10 |     "".to_string()
   |     ^^^^^^^^^^^^^^ expected struct `std::option::Option`, found `std::string::String`
   |
   = note: expected type `Option<std::string::String>`
                          ^^^^^^^                   ^  // < highlighted
              found type `std::string::String`
```

@estebank estebank force-pushed the estebank:shorter-mismatched-types branch from 249245f to 5ce2d15 Feb 19, 2017

@nikomatsakis
Copy link
Contributor

nikomatsakis left a comment

Left some comments, I know this is a WIP, just a few suggestions. The code seems pretty sensible though! Very exciting.

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));

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

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

@@ -555,6 +555,155 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
}
}

fn highlight_outer(&self,

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

meta-comment: I've been talking to @cengizIO about making this error_reporting section 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.

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

also, this fn could really use some doc comments explaining its interface.

@@ -665,26 +814,64 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
diag
}

/// Returns a string of the form "expected `{}`, found `{}`".
fn values_str(&self, values: &ValuePairs<'tcx>) -> Option<(String, String)> {
/// Returns two `Vec`s representing portions of a type with a flag on wether it should

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

Nit: s/wether/whether/

let mut values: (Vec<(String, bool)>, Vec<(String, bool)>) = (vec![], vec![]);
let name1 = self.tcx.item_path_str(def1.did.clone());
let name2 = self.tcx.item_path_str(def2.did.clone());
if name1 == name2 {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

seems strange to compare the string, rather than just comparing def1.did == def2.did

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

I think some comments here would be nice, btw... i.e., // X<...> vs X<...>

This comment has been minimized.

@estebank

estebank Feb 21, 2017

Contributor

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.

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 22, 2017

Contributor

when you say "subtype", are you referring to the X in Option<X>? If so, subtype isn't quite the right term (that usually is used to mean e.g. the relationship between String and Object in Java). I would say "type argument", I guess, if I were referring to the X in some use of Option, and "type parameter" if I am referring to the definition of Option.

{
match (&t1.sty, &t2.sty) {
(&ty::TyAdt(def1, sub1), &ty::TyAdt(def2, sub2)) => {
let mut values: (Vec<(String, bool)>, Vec<(String, bool)>) = (vec![], vec![]);

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

does the bool here mean "highlighted"? seems worth a comment

This comment has been minimized.

@estebank

estebank Feb 21, 2017

Contributor

Indeed. I'll be cleaning these up with a new enum with two variants.

}
if len > 0 && i != len - 1 {
values.0.push((format!(", "), false));
values.1.push((format!(", "), false));

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

Nit: could we avoid pushing <, >, and the type name into values, and then use join or something to add these commas automatically?

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

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

This comment has been minimized.

@estebank

estebank Feb 21, 2017

Contributor

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?

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 22, 2017

Contributor

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)

}
values
} else {
// Check for simple composition

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

what is "simple composition"? an example would be good

This comment has been minimized.

@estebank

estebank Feb 21, 2017

Contributor

The comments so far were meant for myself only, sorry for the lack of depth in them.

By simple composition I meant cases like Some(usize)/usize and String/Some(String).

This comment has been minimized.

@estebank

estebank Feb 21, 2017

Contributor

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.

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 22, 2017

Contributor

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).

// Check for simple composition
for (i, st) in sub1.types().enumerate() {
if st == t2 {
self.highlight_outer(&mut values.0, &mut values.1, name1, sub1, i, &t2);

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

again, an example of code that takes this branch would be great, I have no idea what this is doing :)

This comment has been minimized.

@estebank

estebank Feb 21, 2017

Contributor

These two for loops compare one type against the subtype of the other, to catch cases like usize/Some(usize). It doesn't yet handle the case {integer}/Some(usize).

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 22, 2017

Contributor

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.

}
}
}
for (i, st) in sub2.types().enumerate() {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 21, 2017

Contributor

these two for loops feel like they are begging to be extracted into a fn...no?

This comment has been minimized.

@estebank

estebank Feb 21, 2017

Contributor

Yeah, they are. :)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 28, 2017

☔️ The latest upstream changes (presumably #40008) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 3, 2017

@estebank I'm going to close this PR, since I think the general consensus is that this is awesome, but there hasn't been any activity of late. If you want to re-open, feel free!

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Mar 3, 2017

@nikomatsakis sorry for the radio silence, I was completely offline for the past week. Am I correct in understanding that the proposed output is what we want? If that is the case, I'll clean up the code into shape and submit for proper review.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 10, 2017

@estebank yes, I like the output as it was

TimNN added a commit to TimNN/rust that referenced this pull request Apr 12, 2017

Rollup merge of rust-lang#41205 - estebank:shorter-mismatched-types-2…
…, r=nikomatsakis

Highlight and simplify mismatched types

Shorten mismatched types errors by replacing subtypes that are not
different with `_`, and highlighting only the subtypes that are
different.

Given a file

```rust
struct X<T1, T2> {
    x: T1,
    y: T2,
}

fn foo() -> X<X<String, String>, String> {
    X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
}

fn bar() -> Option<String> {
    "".to_string()
}
```

provide the following output

```rust
error[E0308]: mismatched types
  --> file.rs:6:5
   |
 6 |     X { x: X {x: "".to_string(), y: 2}, y: "".to_string()}
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `std::string::String`, found {integer}
   |
   = note: expected type `X<X<_, std::string::String>, _>`
                                 ^^^^^^^^^^^^^^^^^^^   // < highlighted
              found type `X<X<_, {integer}>, _>`
                                 ^^^^^^^^^             // < highlighted

error[E0308]: mismatched types
  --> file.rs:6:5
   |
10 |     "".to_string()
   |     ^^^^^^^^^^^^^^ expected struct `std::option::Option`, found `std::string::String`
   |
   = note: expected type `Option<std::string::String>`
                          ^^^^^^^                   ^  // < highlighted
              found type `std::string::String`
```

Fix rust-lang#21025. Re: rust-lang#40186. Follow up to rust-lang#39906.

I'm looking to change how this output is accomplished so that it doesn't create list of strings to pass around, but rather add an elided `Ty` placeholder, and use the same string formatting for normal types. I'll be doing that soonish.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment