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

Confusing error message with type unification of impl Traits #63167

Closed
adamlesinski opened this issue Jul 31, 2019 · 13 comments
Closed

Confusing error message with type unification of impl Traits #63167

adamlesinski opened this issue Jul 31, 2019 · 13 comments

Comments

@adamlesinski
Copy link

@adamlesinski adamlesinski commented Jul 31, 2019

It took a colleague explaining to me that the return type of a function returning an impl Trait is unique across function definitions for me to understand the following error: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4ceda076c82b01ea279e8864518ab5d9

This becomes more important for async/await because of the automatic translation of async fn foo() -> i64 to fn foo() -> impl Future<Output = i64>: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=b109446a49d22a632e395aacaa2cb8ff

In the first case, can the error maybe call out that the opaque types are different due to how the compiler treats them? And for the second error, can this be called out in async-specific language. That error happens when you forget to call .await.

@Centril
Copy link
Contributor

@Centril Centril commented Jul 31, 2019

In the first case, can the error maybe call out that the opaque types are different due to how the compiler treats them?

How about?:

   = note: expected type `impl Foo` (opaque type)
              found type `impl Foo` (opaque type)

   = note: distinct uses of `impl Trait` result in different opaque types

And for the second error, can this be called out in async-specific language. That error happens when you forget to call .await.

In the second case I think we should suggest that the .awaits be moved into the branches tho this may be complicated to encode as a diagnostic.

cc @estebank

Loading

@adamlesinski
Copy link
Author

@adamlesinski adamlesinski commented Jul 31, 2019

In the first case, can the error maybe call out that the opaque types are different due to how the compiler treats them?

How about?:

   = note: expected type `impl Foo` (opaque type)
              found type `impl Foo` (opaque type)

   = note: distinct uses of `impl Trait` result in different opaque types

This is perfect! It's the exact hint I needed to understand the problem :)

And for the second error, can this be called out in async-specific language. That error happens when you forget to call .await.

In the second case I think we should suggest that the .awaits be moved into the branches tho this may be complicated to encode as a diagnostic.

cc @estebank

Agreed, this is definitely a bit trickier. I'll defer to you on the best way to message this error.

Loading

@Arnavion
Copy link

@Arnavion Arnavion commented Jul 31, 2019

In the first case, can the error maybe call out that the opaque types are different due to how the compiler treats them?

Doesn't it already does so?

17 | |         two()
   | |         ^^^^^ expected opaque type, found a different opaque type

Loading

@estebank
Copy link
Contributor

@estebank estebank commented Jul 31, 2019

@Centril what is the correct nomenclature for a regular, non-opaque type that we could use here? With that we could say something like "consider materializing to an explicit type in both branches from the opaque type" and "for impl std::future::Future you can .await them on both branches to unify the types". This is rough wording, but gives an idea of what I'm thinking.

Loading

@Centril
Copy link
Contributor

@Centril Centril commented Jul 31, 2019

@estebank The antonym of opaque is transparent but I'm not sure that is situationally good to use here. ISTM you'd want to say something like "use an enum with variants for both cases" (at least that's the advice I'd give in person). I'd go for a non-actionable description as a first pass at this problem and consider an improvement later.

Loading

@gilescope
Copy link
Contributor

@gilescope gilescope commented Aug 20, 2019

Are opaque types the same as anonymous types in C#/Java? If so I think 'anonymous' is clearer than opaque (pardon the pun!).

Loading

@gilescope
Copy link
Contributor

@gilescope gilescope commented Aug 20, 2019

This is a real gotcha of rust and needs a great error message. We could show the definition sites of the opaque types so it's clear which type is expected in this position:

Something like:

17 | |         two()
   | |         ^^^^^ expected type produced at: `one() -> impl Foo`
   | |                  found type produced at: `two() -> impl Foo   
   | |                  note: distinct uses of `impl Trait` in function signatures 
   | |                       result in different opaque types.

Loading

@Centril
Copy link
Contributor

@Centril Centril commented Aug 20, 2019

It doesn't matter where the impl Trait resides in. Two uses will result in two distinct types from the POV of the type system. Only by naming an impl Trait through a type alias can you refer to the same inferred type. Adding the qualification "in function signatures" does not enlighten.

As for "anonymous", the closest equivalent Java's anonymous classes, which conjure new types, in Rust are closures. Also, with Java's anonymous classes you can recover the type identity at runtime using anon.getClass() but you cannot name them. However, impl Trait in Rust can be named (using a type alias), they do not representationally create new types (although they do nominally), and they do not leak their type identities. Rather, impl Trait is about hiding the underlying type (think of expr as impl Debug as a coercion that hides the type). The impl Trait is most similar to Stanard ML's opaque matching (see section G.2 in the definition of Standard ML) All in all, opacity is therefore the most appropriate description rather than anonymity.

Loading

Centril added a commit to Centril/rust that referenced this issue Aug 22, 2019
Tweak E0308 on opaque types

```
error[E0308]: if and else have incompatible types
  --> file.rs:21:9
   |
18 | /     if true {
19 | |         thing_one()
   | |         ----------- expected because of this
20 | |     } else {
21 | |         thing_two()
   | |         ^^^^^^^^^^^ expected opaque type, found a different opaque type
22 | |     }.await
   | |_____- if and else have incompatible types
   |
   = note: expected type `impl std::future::Future` (opaque type)
              found type `impl std::future::Future` (opaque type)
   = note: distinct uses of `impl Trait` result in different opaque types
   = help: if both futures resolve to the same type, consider `await`ing on both of them
```

r? @Centril
CC rust-lang#63167
@gilescope
Copy link
Contributor

@gilescope gilescope commented Aug 26, 2019

I'll get used to opaque :-)

Only by naming an impl Trait through a type alias

You're refering here to existential types? Most of the time when someone gets this error they're assuming they can do this but don't realise they need existential types to make the two impl Traits be the same type. We could hint that there's an unstabilised feature that could help?

But if they weren't trying to bind both impl Traits to be the same type, then given both types textually look the same it would be great if the error message could point out the difference between those two types. Currently the error says:

    = note: expected type `impl Foo` (opaque type)
               found type `impl Foo` (opaque type)

As a nubie I need to see something different between the two types that I can get my teeth into.
How about:

    = note: expected type `impl Foo` (opaque type) @ file2:1257
               found type `impl Foo` (opaque type) @ file1:1234

Somehow it would be great to distinguish these two opaque types (or does the compiler at this point has no knowledge of where these two opaque types came from - only that they are not the same?).

Loading

@estebank
Copy link
Contributor

@estebank estebank commented Aug 26, 2019

Fwiw, we desire to keep jargon to minimum.

Loading

@Centril
Copy link
Contributor

@Centril Centril commented Aug 26, 2019

You're refering here to existential types? Most of the time when someone gets this error they're assuming they can do this but don't realise they need existential types to make the two impl Traits be the same type. We could hint that there's an unstabilised feature that could help?

existential type was removed from the language in #63180 in favor of just type Alias = impl Trait;. We could hint towards that on nightly.

How about:

Seems like a nice improvement.

Loading

@gilescope
Copy link
Contributor

@gilescope gilescope commented Aug 28, 2019

Thanks Centril - hadn't seen that switch. If people are ok with adding line numbers to pinpoint the different impl Traits I'll have a go at rolling a PR?

Loading

Centril added a commit to Centril/rust that referenced this issue Sep 5, 2019
…ramertj

Opaque type locations in error message for clarity.

Attempts to fix rust-lang#63167
Centril added a commit to Centril/rust that referenced this issue Sep 5, 2019
…ramertj,Centril

Opaque type locations in error message for clarity.

Attempts to fix rust-lang#63167
@gilescope
Copy link
Contributor

@gilescope gilescope commented Sep 5, 2019

With all the various improvements that we've done, is there more we could do? or shall we close this issue for now?

Loading

@bors bors closed this in afc7e0e Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants