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.

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

Change the way type variables are printed in diagnostics #18264

Merged
merged 7 commits into from Oct 31, 2014
Merged

Change the way type variables are printed in diagnostics #18264

merged 7 commits into from Oct 31, 2014

Conversation

ghost
Copy link

@ghost ghost commented Oct 23, 2014

This PR aims to improve the readability of diagnostic messages that involve unresolved type variables. Currently, messages like the following:

mismatched types: expected `core::result::Result<uint,()>`, found `core::option::Option<<generic #1>>`
<anon>:6     let a: Result<uint, ()> = None;
                                       ^~~~
mismatched types: expected `&mut <generic #2>`, found `uint`
<anon>:7     f(42u);
               ^~~

tend to appear unapproachable to new users. [0] While specific type var IDs are valuable in
diagnostics that deal with more than one such variable, in practice many messages
only mention one. In those cases, leaving out the specific number makes the messages
slightly less terrifying.

mismatched types: expected `core::result::Result<uint, ()>`, found `core::option::Option<_>`
<anon>:6     let a: Result<uint, ()> = None;
                                       ^~~~
mismatched types: expected `&mut _`, found `uint`
<anon>:7     f(42u);
               ^~~

As you can see, I also tweaked the aesthetics slightly by changing type variables to use the type hole syntax _. For integer variables, the syntax used is:

mismatched types: expected `core::result::Result<uint, ()>`, found `core::option::Option<_#1i>`
<anon>:6     let a: Result<uint, ()> = Some(1);

and float variables:

mismatched types: expected `core::result::Result<uint, ()>`, found `core::option::Option<_#1f>`
<anon>:6     let a: Result<uint, ()> = Some(0.5);

[0] https://twitter.com/coda/status/517713085465772032

Closes #2632.
Closes #3404.
Closes #18426.

@ghost ghost changed the title Only include variable IDs in diagnostics when there's more than one Change the way type variables are printed in diagnostics Oct 25, 2014
@ghost
Copy link
Author

ghost commented Oct 25, 2014

cc @eddyb

.iter()
.map(|elem| ty_to_string_with_var_ids(cx, *elem, print_var_ids))
.collect::<Vec<_>>();
format!("({})", strs.connect(", "))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you sneak in a fix for the issue where (int,) is formatted like (int)? I was just about to make a fix for this myself but I saw that this PR changes quite a bit of that code anyway.

Copy link
Author

Choose a reason for hiding this comment

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

@P1start Done!

@ghost
Copy link
Author

ghost commented Oct 28, 2014

cc @nikomatsakis I figure you may have opinions on this.

@nikomatsakis
Copy link
Contributor

So I think this is great work, though my feeling is that it may be better to just always print _ unless -Z verbose is passed. Have you encountered conditions where knowing the individual identity of the variables was important? I suspect it never comes up; after all, the problem is usually that the variable is never resolved, or else that the known parts cannot be unified (e.g., Option<_> / Result<_,_>).

@nikomatsakis
Copy link
Contributor

One other thing that might be nice and seems like a small tweak on what you have done: in general if the expected/found has the same kind on both sides, or at least it is yielding the same string representation, we could add the message "a different". I'm envisioning something like "found a type parameter, expected a different type parameter". Obviously including the array lengths is even better. Anyway, I'm told this is what the Racket folks do.

@ghost
Copy link
Author

ghost commented Oct 29, 2014

@nikomatsakis

Have you encountered conditions where knowing the individual identity of the variables was important?

I was mostly worried about conditions in which the same type variable would appear twice in a diagnostic, like in the example below:

fn foo<T>() -> T { unreachable!() }
pub struct X<T>(T);
pub struct Y<T>(T);

fn main() {
    let mut x = X(foo());
    x = Y(x);
}
<anon>:7:9: 7:13 error: mismatched types: expected `X<<generic #2>>` but found `Y<X<<generic #2>>>` (expected struct X but found struct Y)
<anon>:7     x = Y(x);

And I figured that therefore it could be useful for the user to know that those types are indeed the same variable. Dropping the indices removes that information. I do share the skepticism, though, but would rather lean on the safe side here for now unless you really think we can always drop the IDs?

@nikomatsakis
Copy link
Contributor

@jakub- well, in that example, it's not particularly important that the two variables are the same, is it?

@ghost
Copy link
Author

ghost commented Oct 29, 2014

@nikomatsakis Fair enough! I pushed an update that always drops IDs from type vars unless -Z verbose is passed in.

@reem
Copy link
Contributor

reem commented Oct 29, 2014

I'm concerned about cases where you'd get a message like expected Option<_>, found Option<_> (found _ but expected _) Perhaps if there is more than one _ in the message, it could be #N or _#N?

@ghost
Copy link
Author

ghost commented Oct 29, 2014

@reem That is indeed what the PR was implementing and what @nikomatsakis and I were discussing, see above.

In general, it seems that can never happen. The expected/found messages always result from type constraints not being satisfied and it is impossible for two type variables not to satisfy a constraint without at least one of them being partially constrained.

Jakub Bukaj added 4 commits October 29, 2014 23:55
…ariables

Diagnostics such as the following

```
mismatched types: expected `core::result::Result<uint,()>`, found `core::option::Option<<generic #1>>`
<anon>:6     let a: Result<uint, ()> = None;
                                       ^~~~
mismatched types: expected `&mut <generic #2>`, found `uint`
<anon>:7     f(42u);
               ^~~
```

tend to be fairly unappealing to new users. While specific type var IDs are valuable in
diagnostics that deal with more than one such variable, in practice many messages
only mention one. In those cases, leaving out the specific number makes the messages
slightly less terrifying.

In addition, type variables have been changed to use the type hole syntax `_` in diagnostics.
With a variable ID, they're printed as `_#id` (e.g. `_#1`). In cases where the ID is left out,
it's simply `_`. Integer and float variables have an additional suffix after the number, e.g.
`_#1i` or `_#3f`.
@nikomatsakis
Copy link
Contributor

I think there was consensus that we should not differentiate integral/floating-point/other type variables in the output, as the distinction is not particularly meaningful (though I'd like to see a test where the failure to unify is due to one variable being integral and one being float, e.g.:

let mut x = 1;
x = 2.0;

There was some debate on IRC over _ vs ?. I favor ? at this point, but it's a minor point. I'd like to see the distinction between _#i etc elided, though.

@ghost
Copy link
Author

ghost commented Oct 30, 2014

@nikomatsakis I added the tests for (expected ..., found a different ...) as well as integer/float type variable mismatch. r?

@nikomatsakis
Copy link
Contributor

I saw @steveklabnik took your side on irc ;) ok, r+!

bors added a commit that referenced this pull request Oct 31, 2014
…atsakis

This PR aims to improve the readability of diagnostic messages that involve unresolved type variables. Currently, messages like the following:

```rust
mismatched types: expected `core::result::Result<uint,()>`, found `core::option::Option<<generic #1>>`
<anon>:6     let a: Result<uint, ()> = None;
                                       ^~~~
mismatched types: expected `&mut <generic #2>`, found `uint`
<anon>:7     f(42u);
               ^~~
```

tend to appear unapproachable to new users. [0] While specific type var IDs are valuable in
diagnostics that deal with more than one such variable, in practice many messages
only mention one. In those cases, leaving out the specific number makes the messages
slightly less terrifying.

```rust
mismatched types: expected `core::result::Result<uint, ()>`, found `core::option::Option<_>`
<anon>:6     let a: Result<uint, ()> = None;
                                       ^~~~
mismatched types: expected `&mut _`, found `uint`
<anon>:7     f(42u);
               ^~~
```

As you can see, I also tweaked the aesthetics slightly by changing type variables to use the type hole syntax _. For integer variables, the syntax used is:

```rust
mismatched types: expected `core::result::Result<uint, ()>`, found `core::option::Option<_#1i>`
<anon>:6     let a: Result<uint, ()> = Some(1);
```

and float variables:

```rust
mismatched types: expected `core::result::Result<uint, ()>`, found `core::option::Option<_#1f>`
<anon>:6     let a: Result<uint, ()> = Some(0.5);
```

[0] https://twitter.com/coda/status/517713085465772032

Closes #2632.
Closes #3404.
Closes #18426.
@bors bors closed this Oct 31, 2014
@bors bors merged commit a2624fc into rust-lang:master Oct 31, 2014
@ghost ghost deleted the var-ids-in-error-messages branch November 7, 2014 22:03
lnicola pushed a commit to lnicola/rust that referenced this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants