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

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

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
Collaborator

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
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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