Skip to content

Elaborate Future::Output when printing opaque impl Future type #91021

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Nov 20, 2021

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Nov 18, 2021

I would love to see the Output = type when printing type errors involving opaque impl Future.

Test code

Before (cut relevant part of output):

note: while checking the return type of the `async fn`
 --> /home/michael/test.rs:5:19
  |
5 | async fn bar() -> usize {
  |                   ^^^^^ checked the `Output` of this `async fn`, found opaque type
  = note:     expected type `usize`
          found opaque type `impl Future`

After:

note: while checking the return type of the `async fn`
 --> /home/michael/test.rs:5:19
  |
5 | async fn bar() -> usize {
  |                   ^^^^^ checked the `Output` of this `async fn`, found opaque type
  = note:     expected type `usize`
          found opaque type `impl Future<Output = usize>`

Note the "found opaque type impl Future<Output = usize>" in the new output.


Questions:

  1. We skip printing the output type when it's a projection, since I have been seeing some types like impl Future<Output = <[static generator@/home/michael/test.rs:2:11: 2:21] as Generator<ResumeTy>>::Return> which are not particularly helpful and leak implementation detail.
    • Am I able to normalize this type within rustc_middle::ty::print::pretty? Alternatively, can we normalize it when creating the diagnostic? Otherwise, I'm fine with skipping it and falling back to the old output.
    • Should I suppress any other types? I didn't encounter anything other than this generator projection type.
  2. Not sure what the formatting of this should be. Do I include spaces in Output = ?

@rust-highfive
Copy link
Contributor

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2021
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

Had to re-bless test outputs

Comment on lines +661 to +672
ty::PredicateKind::Projection(projection_predicate) => {
let Some(future_trait) = self.tcx().lang_items().future_trait() else { continue };
let future_output_def_id =
self.tcx().associated_item_def_ids(future_trait)[0];

if projection_predicate.projection_ty.item_def_id
== future_output_def_id
{
// We don't account for multiple `Future::Output = Ty` contraints.
is_future = true;
future_output_ty = Some(projection_predicate.ty);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

A note: copied most of this from get_impl_future_output_ty in rustc_infer::infer::error_reporting. Didn't know how to factor those two together when we're already iterating thru the predicates here, so pardon the code duplication.

@estebank
Copy link
Contributor

Ideally we'd generalize this for all associated types, but I'm more than happy to special case Future, given how critical it is. I could see, for example how this would be useful for impl Iterator as well, but doing this for a handful of traits would be an exercise in futility. I want to land this as is, would you be interested in generalizing this? Something like collecting a HashMap<DefId /* of trait */, Vec<ProjectionPredicate>> and only printing everything afterwards would work well, I think, and then we could remove the special casing. Would you be interested in doing that?

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 19, 2021

📌 Commit f6392a1 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2021
@estebank
Copy link
Contributor

We skip printing the output type when it's a projection, since I have been seeing some types like impl Future<Output = <[static generator@/home/michael/test.rs:2:11: 2:21] as Generator>::Return> which are not particularly helpful and leak implementation detail.

   Am I able to normalize this type within rustc_middle::ty::print::pretty? Alternatively, can we normalize it when creating the diagnostic? Otherwise, I'm fine with skipping it and falling back to the old output.

I would try normalizing it, but wouldn't be surprised if you get ICEs from it. Also I think that they are resolve_vars_if_possible already before being passed to print::pretty where the error is being emitted, but if they aren't, that would be a safer place to do that.

   Should I suppress any other types? I didn't encounter anything other than this generator projection type.

I think it's ok to land the PR as is and leak these, at least for now. I prefer to have too much info than too little and we can improve it piecemeal.

Not sure what the formatting of this should be. Do I include spaces in Output = ?

Spaces around assoc type =s is my preferred style :)

@estebank
Copy link
Contributor

Something like collecting a HashMap<DefId /* of trait */, Vec> and only printing everything afterwards would work well, I think, and then we could remove the special casing. Would you be interested in doing that?

One more thing: if you do that, you need to make sure that the following prints correctly impl Trait<Type, Assoc = Type, Assoc = ()>.

@compiler-errors
Copy link
Member Author

I'd love to do this in general for impl Trait<Generic, Assoc = Ty>. I'll put up a draft this weekend -- I can totally see how to refactor this code to make that happen.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 20, 2021
…, r=estebank

Elaborate `Future::Output` when printing opaque `impl Future` type

I would love to see the `Output =` type when printing type errors involving opaque `impl Future`.

[Test code](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a800b481edd31575fbcaf5771a9c3678)

Before (cut relevant part of output):
```
note: while checking the return type of the `async fn`
 --> /home/michael/test.rs:5:19
  |
5 | async fn bar() -> usize {
  |                   ^^^^^ checked the `Output` of this `async fn`, found opaque type
  = note:     expected type `usize`
          found opaque type `impl Future`
```

After:
```
note: while checking the return type of the `async fn`
 --> /home/michael/test.rs:5:19
  |
5 | async fn bar() -> usize {
  |                   ^^^^^ checked the `Output` of this `async fn`, found opaque type
  = note:     expected type `usize`
          found opaque type `impl Future<Output = usize>`
```

Note the "found opaque type `impl Future<Output = usize>`" in the new output.

----

Questions:
1. We skip printing the output type when it's a projection, since I have been seeing some types like `impl Future<Output = <[static generator@/home/michael/test.rs:2:11: 2:21] as Generator<ResumeTy>>::Return>` which are not particularly helpful and leak implementation detail.
    * Am I able to normalize this type within `rustc_middle::ty::print::pretty`? Alternatively, can we normalize it when creating the diagnostic? Otherwise, I'm fine with skipping it and falling back to the old output.
    * Should I suppress any other types? I didn't encounter anything other than this generator projection type.
2. Not sure what the formatting of this should be. Do I include spaces in `Output = `?
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 20, 2021
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#90575 (Improve suggestions for compatible variants on type mismatch.)
 - rust-lang#90628 (Clarify error messages caused by re-exporting `pub(crate)` visibility to outside)
 - rust-lang#90930 (Fix `non-constant value` ICE (rust-lang#90878))
 - rust-lang#90983 (Make scrollbar in the sidebar always visible for visual consistency)
 - rust-lang#91021 (Elaborate `Future::Output` when printing opaque `impl Future` type)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3379721 into rust-lang:master Nov 20, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 20, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 24, 2021
…ait, r=estebank

Print associated types on opaque `impl Trait` types

This PR generalizes rust-lang#91021, printing associated types for all opaque `impl Trait` types instead of just special-casing for future.

before:
```
error[E0271]: type mismatch resolving `<impl Iterator as Iterator>::Item == u32`
```

after:
```
error[E0271]: type mismatch resolving `<impl Iterator<Item = usize> as Iterator>::Item == u32`
```

---

Questions:
1. I'm kinda lost in binders hell with this one. Is all of the `rebind`ing necessary?
2. Is there a map collection type that will give me a stable iteration order? Doesn't seem like TraitRef is Ord, so I can't just sort later..
3. I removed the logic that suppresses printing generator projection types. It creates outputs like this [gist](https://gist.github.com/compiler-errors/d6f12fb30079feb1ad1d5f1ab39a3a8d). Should I put that back?
4. I also added spaces between traits, `impl A+B` -> `impl A + B`. I quite like this change, but is there a good reason to keep it like that?

r? `@estebank`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 25, 2021
…ait, r=estebank

Print associated types on opaque `impl Trait` types

This PR generalizes rust-lang#91021, printing associated types for all opaque `impl Trait` types instead of just special-casing for future.

before:
```
error[E0271]: type mismatch resolving `<impl Iterator as Iterator>::Item == u32`
```

after:
```
error[E0271]: type mismatch resolving `<impl Iterator<Item = usize> as Iterator>::Item == u32`
```

---

Questions:
1. I'm kinda lost in binders hell with this one. Is all of the `rebind`ing necessary?
2. Is there a map collection type that will give me a stable iteration order? Doesn't seem like TraitRef is Ord, so I can't just sort later..
3. I removed the logic that suppresses printing generator projection types. It creates outputs like this [gist](https://gist.github.com/compiler-errors/d6f12fb30079feb1ad1d5f1ab39a3a8d). Should I put that back?
4. I also added spaces between traits, `impl A+B` -> `impl A + B`. I quite like this change, but is there a good reason to keep it like that?

r? ``@estebank``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 25, 2021
…ait, r=estebank

Print associated types on opaque `impl Trait` types

This PR generalizes rust-lang#91021, printing associated types for all opaque `impl Trait` types instead of just special-casing for future.

before:
```
error[E0271]: type mismatch resolving `<impl Iterator as Iterator>::Item == u32`
```

after:
```
error[E0271]: type mismatch resolving `<impl Iterator<Item = usize> as Iterator>::Item == u32`
```

---

Questions:
1. I'm kinda lost in binders hell with this one. Is all of the `rebind`ing necessary?
2. Is there a map collection type that will give me a stable iteration order? Doesn't seem like TraitRef is Ord, so I can't just sort later..
3. I removed the logic that suppresses printing generator projection types. It creates outputs like this [gist](https://gist.github.com/compiler-errors/d6f12fb30079feb1ad1d5f1ab39a3a8d). Should I put that back?
4. I also added spaces between traits, `impl A+B` -> `impl A + B`. I quite like this change, but is there a good reason to keep it like that?

r? ```@estebank```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 25, 2021
…ait, r=estebank

Print associated types on opaque `impl Trait` types

This PR generalizes rust-lang#91021, printing associated types for all opaque `impl Trait` types instead of just special-casing for future.

before:
```
error[E0271]: type mismatch resolving `<impl Iterator as Iterator>::Item == u32`
```

after:
```
error[E0271]: type mismatch resolving `<impl Iterator<Item = usize> as Iterator>::Item == u32`
```

---

Questions:
1. I'm kinda lost in binders hell with this one. Is all of the `rebind`ing necessary?
2. Is there a map collection type that will give me a stable iteration order? Doesn't seem like TraitRef is Ord, so I can't just sort later..
3. I removed the logic that suppresses printing generator projection types. It creates outputs like this [gist](https://gist.github.com/compiler-errors/d6f12fb30079feb1ad1d5f1ab39a3a8d). Should I put that back?
4. I also added spaces between traits, `impl A+B` -> `impl A + B`. I quite like this change, but is there a good reason to keep it like that?

r? ````@estebank````
@compiler-errors compiler-errors deleted the print_future_output branch December 3, 2021 07:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants