Print associated types on opaque `impl Trait` types #91096

compiler-errors · 2021-11-20T23:33:50Z

This PR generalizes #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:

I'm kinda lost in binders hell with this one. Is all of the rebinding necessary?
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..
I removed the logic that suppresses printing generator projection types. It creates outputs like this gist. Should I put that back?
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

estebank · 2021-11-21T00:37:08Z

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?

Can we move that to a separate PR? I can approve and merge that quickly.

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.

You can use sort_by to get around that issue. Any container that does stable sorted iteration will have that problem :-/

I removed the logic that suppresses printing generator projection types. It creates outputs like this gist. Should I put that back?

Some of the output is getting quite verbose :(

The one that stands out to me in particular is for closures: multiple Fn* traits are implemented for the type, and now we show them all, instead of showing only the one that is relevant. The nested Future cases also look... unwieldy. It might be we do want to show them in some cases, but I'm slightly concerned. I need to think about what we can do here a bit more in depth.

estebank · 2021-11-21T00:37:49Z

compiler/rustc_middle/src/ty/print/pretty.rs

+                                // Append to trait_ref's projection list if it exists, otherwise
+                                // insert a new one.
+                                if let Some((_, assoc_items)) =
+                                    traits.iter_mut().find(|(t, _)| t == &trait_ref)
                                {
-                                    is_future = true;
-                                    continue;
+                                    assoc_items.push(proj_ty);
+                                } else {
+                                    traits.push((trait_ref, vec![proj_ty]));
                                }


All of this can be replaced by using the .entry() API instead.

Yeah, that's what I was using before I hit issues with iter() ordering being unstable and messing up the ui tests.

To make myself a bit more clear, I was using an HashMap here before and when I iterated over the entries during the printing phase, the test outputs were randomized across runs. I tried to fix this by then collecting back into a Vec only after iteration, but I didn't know what to sort_by since all of my keys aren't Ord.

compiler-errors · 2021-11-21T00:39:56Z

You can use sort_by to get around that issue

Sort by what though? PolyTraitRef (or TraitRef) which I would want to use as the key to this mapping is not Ord. Should I sort by the ref's DefId and its substs?

compiler-errors · 2021-11-21T00:40:05Z

Can we move that to a separate PR? I can approve and merge that quickly.

Can do.

estebank · 2021-11-21T00:40:06Z

src/test/ui/lint/opaque-ty-ffi-unsafe.stderr

@@ -1,4 +1,4 @@
-error: `extern` block uses type `impl Fn<()>`, which is not FFI-safe
+error: `extern` block uses type `impl Fn<()> + FnOnce<(), Output = ()>`, which is not FFI-safe


This is what I was mentioning. I think that we can look at the traits to see that one implies the other to show only one at a time, without special casing closures.

Yeah so this is printing because we have two predicates here Fn<()> and <Self as FnOnce<()>>::Output == (). The first one makes us print Fn<()> and the second makes us print FnOnce<(), Output = ()>.

I actually would be interested in special casing just Fn/FnMut/FnOnce and fixing it up further so it prints impl Fn() -> () instead.

You can keep track of the traits for the predicates you've seen already and see if the new predicate's trait is part of the existing ones' super-traits. Special casing sounds reasonable-ish, but I would prefer it if we could just make it work for the general case :)

Regardless, dealing with the <_ as async {}>::Return types in a nicer way (even if it is showing them as _) has priority over this.

You can keep track of the traits for the predicates you've seen already

Is there any ordering enforced for the list of predicates that we're iterating over here, though?

I would prefer it if we could just make it work for the general case :)

So I'm pretty sure this only prints supertraits if you're bounding an associated type on that supertrait, so in practice we really only see this with Fn* family of traits I think. (Because impl Fn() -> Foo desugars to impl Fn<()> + FnOnce<(), Output=Foo>, or similar, right?)

Closures is gonna be the most common thing people will see, but a crate can write their own API structure that does the same thing. If closures are special cased, any user of those (potential, as none come to mind) crates could get confused.

estebank · 2021-11-21T00:42:43Z

src/test/ui/impl-trait/issues/issue-78722.stderr

@@ -15,7 +15,7 @@ LL | pub const fn from_generator<T>(gen: T) -> impl Future<Output = T::Return>
   |                                           ------------------------------- the found opaque type
   |
   = note: expected opaque type `impl Future<Output = u8>`
-              found opaque type `impl Future`
+              found opaque type `impl Future<Output = <[static generator@$DIR/issue-78722.rs:12:26: 12:31] as Generator<ResumeTy>>::Return>`


I think though that we should special case Generator and call it something like [async@$DIR/issue-78722.rs:12:20: 12:31] so it looks like:

found opaque type `impl Future<Output = <[async@$DIR/issue-78722.rs:12:26: 12:31]>`

What do you think?

It is tricky, because I'd want to refer to the whole Future as that, but that would also be confusing :-/

Hmm.. Yeah, I could see us special-casing it. I wouldn't mind printing [async@...] for the whole impl Future type. Having impl Future<Output = [async@...]> seems to me confusing because the output type isn't the async block's type itself.

... unless there's a way of normalizing that <_ as Generator>::Return type in the code here. That would be the best.

If you make Generator::Return a lang_item and query it here to unconditionally replace it with _ or [async result], what is the diff in the tests? We might want to go that route instead, depending on how much real code is affected. I suspect that this type isn't being shown much today because it most commonly appears only as part of Future::Output, so it wouldn't be a regression to hide it.

bors · 2021-11-21T14:00:49Z

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

compiler-errors · 2021-11-22T05:58:47Z

I updated the code to address a few issues with my original PR:

Replace those noisy generator projection ty with [async output]. I would be fine with also replacing it with _.
Add logic to special-case print impl Fn(A, B, C) -> D for Fn/FnMut/FnOnce in the cases where the predicates give us enough information to do so (namely, we have both args and return ty).

And a few smaller things to support these changes:

Derive Ord for TraitRef so I could use BTreeMap instead of Vec (which stably prints error outputs now).
Added lang-item for generator_return.
Coped and simplified supertraits method from rustc_infer since we can't access that from rustc_middle, since we need it for impl Fn printing.

What do you think @estebank ?

compiler/rustc_middle/src/ty/print/pretty.rs

estebank · 2021-11-23T07:35:48Z

src/test/ui/impl-trait/issues/issue-86201.stderr

@@ -7,7 +7,7 @@ LL |
 LL | static STATIC_FN: FunType = some_fn;
   |                             ^^^^^^^ expected opaque type, found fn item
   |
-   = note: expected opaque type `impl Fn<()>`
+   = note: expected opaque type `impl Fn<()> + FnOnce<()>`


Noticed this, a bit odd because it seems you were detecting this case already

So the reason this one doesn't rewrite itself into impl Fn() -> T is because the opaque type has no return type constraint. I'll fix it to just go back to impl Fn<()> (very easy to do) or we could print -> _ if you think that would look better!

Either approach would be fine. Do whichever works best for you.

estebank · 2021-11-23T19:40:15Z

src/test/ui/entry-point/imported_main_const_fn_item_type_forbidden.stderr

+   = note: expected opaque type `impl Fn()`
                  found fn item `fn() {bar}`


(not for this PR): These cases should have an additional note explaining the difference between fn and Fn, and what you can do to deal with that.

estebank · 2021-11-23T19:44:23Z

@bors r+

bors · 2021-11-23T19:44:24Z

📌 Commit 9cc1179 has been approved by estebank

bors · 2021-11-24T08:27:36Z

⌛ Testing commit 9cc1179 with merge 26389d998e6ee0492f2c7f3eec661d37e9d6d841...

rust-log-analyzer · 2021-11-24T09:02:46Z

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

......... (50/53)
...        (53/53)


/checkout/src/test/rustdoc-gui/toggle-click-deadspace.goml toggle-click-deadspace... FAILED
[ERROR] (line 8) Error: Evaluation failed: assert didn't fail: for command `assert-attribute-false: (".impl-items .rustdoc-toggle", {"open": ""})`



command did not execute successfully: "/node-v14.4.0-linux-x64/bin/node" "/checkout/src/tools/rustdoc-gui/tester.js" "--jobs" "16" "--doc-folder" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc" "--tests-folder" "/checkout/src/test/rustdoc-gui"


Build completed unsuccessfully in 0:00:16

bors · 2021-11-24T09:03:40Z

💔 Test failed - checks-actions

oli-obk · 2021-11-24T09:11:20Z

@bors retry

…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`

…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``

…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```

…askrgr Rollup of 6 pull requests Successful merges: - rust-lang#89359 (Various fixes for const_trait_impl) - rust-lang#90499 (Link with default MACOSX_DEPLOYMENT_TARGET if not otherwise specified.) - rust-lang#91096 (Print associated types on opaque `impl Trait` types) - rust-lang#91111 (Do not visit attributes in `ItemLowerer`.) - rust-lang#91162 (explain why CTFE/Miri perform truncation on shift offset) - rust-lang#91185 (Remove `-Z force-overflow-checks`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

… r=jackh726 Restore `impl Future<Output = Type>` to async blocks I was sad when I undid some of the code I wrote in rust-lang#91096 in the PR rust-lang#95225, so I fixed it here to not print `[async output]`. This PR "manually" normalizes the associated type `<[generator] as Generator>::Return` type which appears very frequently in `impl Future` types that result from async block desugaring.

rust-highfive assigned estebank Nov 20, 2021

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

estebank reviewed Nov 21, 2021

View reviewed changes

compiler-errors force-pushed the elaborate_opaque_trait branch from e24de3e to 22f9b9b Compare November 22, 2021 05:58

compiler-errors force-pushed the elaborate_opaque_trait branch from 22f9b9b to 703d751 Compare November 22, 2021 06:11

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 22, 2021

estebank reviewed Nov 23, 2021

View reviewed changes

compiler/rustc_middle/src/ty/print/pretty.rs Outdated Show resolved Hide resolved

estebank reviewed Nov 23, 2021

View reviewed changes

oli-obk self-assigned this Nov 23, 2021

compiler-errors added 6 commits November 23, 2021 10:34

Derive Ord and PartialOrd for TraitRef

8756d07

Add supertraits method to rustc_middle

c969b1d

Elaborate trait generics and associated types

ce182d1

Add generator lang-item

b84a52c

Suppress noisy generator associated type

471334e

Update test outputs

9ae575c

compiler-errors force-pushed the elaborate_opaque_trait branch from 703d751 to 4c6194f Compare November 23, 2021 18:34

This comment has been minimized.

Sign in to view

Fix printing unit return ty, don't elaborate FnOnce unless we see it

9cc1179

compiler-errors force-pushed the elaborate_opaque_trait branch from 4c6194f to 9cc1179 Compare November 23, 2021 19:05

estebank reviewed Nov 23, 2021

View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 23, 2021

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 23, 2021

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

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 24, 2021

GuillaumeGomez mentioned this pull request Nov 24, 2021

Rollup of 8 pull requests #91198

Closed

matthiaskrgr mentioned this pull request Nov 25, 2021

Rollup of 5 pull requests #91213

Closed

matthiaskrgr mentioned this pull request Nov 25, 2021

Rollup of 5 pull requests #91214

Closed

matthiaskrgr mentioned this pull request Nov 25, 2021

Rollup of 6 pull requests #91221

Merged

bors merged commit 6970cf5 into rust-lang:master Nov 25, 2021

rustbot added this to the 1.58.0 milestone Nov 25, 2021

compiler-errors deleted the elaborate_opaque_trait branch December 3, 2021 07:09

compiler-errors mentioned this pull request Mar 24, 2022

Restore impl Future<Output = Type> to async blocks #95263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print associated types on opaque `impl Trait` types #91096

Print associated types on opaque `impl Trait` types #91096

compiler-errors commented Nov 20, 2021

estebank commented Nov 21, 2021

estebank Nov 21, 2021

compiler-errors Nov 21, 2021 •

edited

Loading

compiler-errors Nov 21, 2021

compiler-errors commented Nov 21, 2021

compiler-errors commented Nov 21, 2021

estebank Nov 21, 2021

compiler-errors Nov 21, 2021

estebank Nov 21, 2021

compiler-errors Nov 21, 2021 •

edited

Loading

estebank Nov 21, 2021

estebank Nov 21, 2021 •

edited

Loading

compiler-errors Nov 21, 2021 •

edited

Loading

estebank Nov 21, 2021

bors commented Nov 21, 2021

compiler-errors commented Nov 22, 2021 •

edited

Loading

estebank Nov 23, 2021

compiler-errors Nov 23, 2021

estebank Nov 23, 2021

This comment has been minimized.

estebank Nov 23, 2021

estebank commented Nov 23, 2021

bors commented Nov 23, 2021

bors commented Nov 24, 2021

rust-log-analyzer commented Nov 24, 2021

bors commented Nov 24, 2021

oli-obk commented Nov 24, 2021

		@@ -1,4 +1,4 @@
		error: `extern` block uses type `impl Fn<()>`, which is not FFI-safe
		error: `extern` block uses type `impl Fn<()> + FnOnce<(), Output = ()>`, which is not FFI-safe

		= note: expected opaque type `impl Fn()`
		found fn item `fn() {bar}`

Print associated types on opaque impl Trait types #91096

Print associated types on opaque impl Trait types #91096

Conversation

compiler-errors commented Nov 20, 2021

estebank commented Nov 21, 2021

Choose a reason for hiding this comment

compiler-errors Nov 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Nov 21, 2021

compiler-errors commented Nov 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Nov 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Nov 21, 2021 • edited Loading

Choose a reason for hiding this comment

compiler-errors Nov 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 21, 2021

compiler-errors commented Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

estebank commented Nov 23, 2021

bors commented Nov 23, 2021

bors commented Nov 24, 2021

rust-log-analyzer commented Nov 24, 2021

bors commented Nov 24, 2021

oli-obk commented Nov 24, 2021

Print associated types on opaque `impl Trait` types #91096

Print associated types on opaque `impl Trait` types #91096

compiler-errors Nov 21, 2021 •

edited

Loading

compiler-errors Nov 21, 2021 •

edited

Loading

estebank Nov 21, 2021 •

edited

Loading

compiler-errors Nov 21, 2021 •

edited

Loading

compiler-errors commented Nov 22, 2021 •

edited

Loading