More distinctive pretty-printing of function item types

[steffahn]

Change the way “fn item” types are displayed: make them easier to distinguish from fn pointers.

My first point here is to figure out if CI passes, or how much more needs to be done to make it pass. (Edit: It does pass 🥳) Then there still needs to be discussion if we want such a change, and also how exactly they should best be rendered.

As you can see, this affects quite a lot of output. In particular, not only error messages but also pretty printed MIR (with types), as far as I can tell.

Context: https://internals.rust-lang.org/t/extending-implicit-coercion-of-function-items-to-function-pointers-during-trait-resolution/17093/6

My initial idea was to use something like [function item `guess`: for<'r> fn(&'r [Guess]) -> String], but fn item is more consistent with other parts of the error messages, and the backticks don’t look nice in an error message that also puts backticks around the whole type, so it became [fn item {guess}: for<'r> fn(&'r [Guess]) -> String]. I’ve also considered something like [fn-item guess: for<'r> fn(&'r [Guess]) -> String], but the colon looks suboptimal after a longer path; wrapping the path into an extra set of parentheses works nicely IMO, and it could also be seen as a reference to the previous way of rendering the type, for<'r> fn(&'r [Guess]) -> String {guess}, which used the curly braces, too.

Really, all that changes now is: the order is switched, the thing becomes more clearly grouped as one coherent thing with the []s (which are also meant to make the thing look more similar to a closure type, and more clearly different from a fn pointer type), and the thing becomes more self-describing (since it is special diagnostics-only syntax, after all) with the string “fn item” included.

As of now, there’s a minor downside that tuple-structs’ and tuple-struct-style-enum-variants’ constructors render as something like [fn item {Foo}: u8 -> Foo], too, whereas something like e.g. [tuple struct constructor {Foo}: u8 -> Foo] might be nicer. It’s also already described as “'fn item`” currently though anyway, by error messages such as

struct Foo(u8);
fn f() {
    let _: () = Foo;
}

error[E0308]: mismatched types
 --> src/lib.rs:3:17
  |
3 |     let _: () = Foo;
  |            --   ^^^ expected `()`, found fn item
  |            |
  |            expected due to this
  |
  = note: expected unit type `()`
               found fn item `fn(u8) -> Foo {Foo}`

(which would after this PR look like this instead:)

error[E0308]: mismatched types
 --> src/lib.rs:3:17
  |
3 |     let _: () = Foo;
  |            --   ^^^ expected `()`, found fn item
  |            |
  |            expected due to this
  |
  = note: expected unit type `()`
               found fn item `[fn item {Foo}: fn(u8) -> Foo]`

[rust-highfive]

r? @davidtwco

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

[steffahn]

I’m wondering if there’s a way (and a desire) to (be able to) test colors of compiler output, for the highlighting logic.

[compiler-errors]

As of now, there’s a minor downside that tuple-structs’ and tuple-struct-style-enum-variants’ constructors render as something like [...]

We have access to tcx in pretty printing, right? We should be able to check the def_kind and special case for constructors, if necessary. Though that can be done in a follow-up.

Otherwise, I do like this change, even if the output is a bit heavier.

cc @rust-lang/wg-diagnostics

[CAD97]

cc @estebank who recently complained about the closure type display; this makes fn items more like closures.

Bikeshed: why don't we just call it fn Foo(u8) -> Foo / for<'r> fn guess(&'r [Guess]) -> String? This moves the "it's not a function pointer" earlier in the type, but I suppose still doesn't offer an obvious difference to fn(u8) -> String / for<'r> fn(&'r [Guess]) -> String.

Either way, I think the ideal situation (including closure numbering) would treat fn name and fn {closure#0} similarly, if not for the fact that closures can be !Fn.

Using the "type ascription" form would allow closures to use : fn, : Fn, : FnMut, or : FnOnce to communicate the most permissive coercion they provide.

---

A proper survey would consider all unnamed types and how we display them. Fn items are fn(Args...) -> Ret {path}, closures are [closure@path:LL:CC: LL:CC]; opaque RPIT and APIT use impl Trait with a note about how types from different sites¹ or that close over different generics² differ. Are there any other cases where we generate a fresh type that doesn't have a user-writable name?

Footnotes

1. `impl Sized` (opaque type at <src/lib.rs:1:12>) ↩

2. `impl Sized` (`u8`) ↩

[CAD97]

I’m wondering if there’s a way (and a desire) to (be able to) test colors of compiler output, for the highlighting logic.

Current tests have no color output, and this is the correct default, provided by default --color=auto and not any explicit choice. It should be possible to set something like // compile-flags --color=always to force the color ANSI codes into the test output. (Of course/unfortunately, this makes reviewing it as plaintext somewhere between difficult and impossible.)

[steffahn]

We have access to tcx in pretty printing, right? We should be able to check the def_kind and special case for constructors, if necessary. Though that can be done in a follow-up.

Yeah, I thought follow-up.

[steffahn]

Anyone know how to run 32bit mir-opt test cases to bless them, while working on a 64bit machine?

Ah, I found the command in the CI output, let’s try that.

python2.7 ../x.py --stage 2 test src/test/mir-opt --host= --target=i686-unknown-linux-gnu

[compiler-errors]

@steffahn I think you've gotta run ./x.py test --target=i686-unknown-linux-gnu src/test/mir-opt

edit: lol i didnt see your comment haha

[steffahn]

Hmmm… I’m getting some errors /usr/include/limits.h:26:10: fatal error: bits/libc-header-start.h: No such file or directory with when trying either version of that.

(During Building stage2 std artifacts, in Compiling compiler_builtins v0.1.73.)

[steffahn]

Well, it’s few enough changes that I can do it manually…

Edit: Ah, I’ve missed fixing one of them.

Edit2: Oh, neat, so after that, it actually passes now.

[davidtwco]

Implementation changes look good to me. I don't have a strong opinion on the actual changes being made, I'll let this sit for a little bit to see if anyone else has opinions.

[estebank]

Can we get a screenshot to see the highlighting?

[steffahn]

I’ve started working on differentiating between constructors and non-constructors, however, it’s running into problems in the ui test that runs rustc with RUSTC_LOG=debug, i.e. src/test/ui/rustc-rust-log.rs. Some kind of infinite looping or recursion or so (i.e. it hangs); presumably because the call I’m making does some debug printing of the type, resulting in infinite recursion? Though I’ll have to investigate.

I also made some small further changes to the highlighting there… also I wouldn’t mind trying to get it to work and then integrate it into this PR already. In that case, @estebank, perhaps it makes most sense to show screenshots of the highlighting only after those changes since they change highlighting a little…

@rustbot author

---

Currently working on this branch (new commits: 65eb340 and cb0db7f); the relevant new call that’s presumably still causing problems is the one to self.tcx().def_kind(def_id).

For fixing the problem, I’m considering either to look for an alternative to the def_kind call, or to find and disable the relevant debug-information printing in its implementation. In case someone has experience with such problems, feel free to give general advice.

---

Edit: Actually… upon inspection of the error message, I think I’m perhaps simply reaching the unreachable!() statement I wrote.

[bors]

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

[steffahn]

Given the large number of affected files, merge conflicts in test outputs are inevitable, and it won't make sense to fix them before there is agreement that this PR should be merged in the first place.

[davidtwco]

r? @estebank

[estebank]

We'll have a t-compiler meeting to follow up on this.

There's a subset of this PR that I think is uncontroversial (and could be landed faster), but for the larger visible representation of the output I believe merits discussion.

[estebank]

Apologies, I failed to create a meeting to address this. I'll do so shortly.

CC #19939

[estebank]

Opened https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/More.20distinctive.20pretty-printing.20of.20function.20item.20types to try and arrive to agreement on what the output should be.

[apiraino]

I'll label this as S-blocked trying to signal that a consensus should be reached to move on.

@rustbot label S-blocked

[jackh726]

Changing this to waiting-on-team, since that's a more appropriate label than blocked (since there is no concrete item to unblock this)

[wesleywiser]

Visited during the compiler team triage meeting. There was some rough consensus that @bstrie's suggestion from Zulip might be a reasonable compromise between concerns:

trying to highlight the difference between function pointers/function items solely by making their as-written-in-the-source-code types distinct (and then trying to future-proof that syntax) might be worse than just having a note like "note: foo is a function item, which is distinct from a function pointer".

We think it would be useful to try implementing that informational note and then re-evaluate how well the diagnostics work after that.

[steffahn]

just having a note like "note: foo is a function item, which is distinct from a function pointer"

That sounds like an unrelated change, both in terms of what it achieves and how it would be implemented. It can of course be very useful improvement, too.

So if the consensus is that fn(Args) -> ReturnVal {path_to_item} should stay the preferred way the compile prints types of fn items (and tuple-struct constructors), I should maybe just close this PR, which was about changing the way the “name” of such a type is printed?

We think it would be useful to try implementing that informational note and then re-evaluate how well the diagnostics work after that.

So then the idea would not be to close this PR but “block” is on someone implementing some additional diagnostics? Note that in my mind, the ability to avoid confusion was only part of the motivation for this PR. I also just don’t see much value in the design of the current way these types are printed; besides the “legacy” value of “experienced Rust programmers may be used to this way”.

E.g. it would’t make a difference for any aspect listed in the following paragraph from the PR description, whether or not there was a useful note in relevant error messages, except partially for the point of it being “more clearly different from a fn pointer type”. (I.e. the aspect that clearly different looking types could avoid confusion would be addressed, the aspect that unnecessarily similar looking types are just bad design wouldn’t be addressed.)

(from the PR description)

the order is switched, the thing becomes more clearly grouped as one coherent thing with the []s (which are also meant to make the thing look more similar to a closure type, and more clearly different from a fn pointer type), and the thing becomes more self-describing (since it is special diagnostics-only syntax, after all) with the string “fn item” included.

To re-iterate the points: I do believe it’s a nicer design to have some way of printing the type that makes the function name come first. I do believe that there’s value in conveying in the design of the way diagnostics print them that closure types and fn items types are similar, and fn pointer types are quite different from those two. I do believe that the syntax fn(Args) -> ReturnVal {path_to_item} features too many spaces that make the syntactic elements making up this rendering of the type feel very visually disconnected, and I do think that the vibes of “this is clearly not valid Rust syntax” that the way closure types in diagnostics currently give off is valuable.

On that last note, I also think there might be value on changing all cases where unnamable types are currently looking a lot like valid Rust syntax to name that type. Another example (maybe the only other one? ¹) could be impl Trait return types (“opaque types”). Though in that case, at least the types are referred to using the same syntax as the diagnostics, once, in the source code, namely in the function return type that defines them (except when they come from the async fn sugar); and at least the syntax is quite clearly distinguished from unrelated things like dyn Trait. (Which seems at least somewhat analogous to the fn item vs fn pointer distinction.) Also, I don’t want to discuss this too deeply here, as it’s somewhat of another topic.

---

Another idea could be to just avoid printing anything for such types in more cases. E.g. I’ve just noticed that for futures, in some cases the note: expected `…`, found `…` is just completely omitted.

async fn foo() {}
async fn bar() {}

fn f() {
    let mut f = foo();
    f = bar();
}

error[[E0308]](https://doc.rust-lang.org/stable/error_codes/E0308.html): mismatched types
 --> src/lib.rs:6:9
  |
5 |     let mut f = foo();
  |                 ----- expected due to this value
6 |     f = bar();
  |         ^^^^^ expected future, found a different future
  |
  = help: consider `await`ing on both `Future`s
  = note: distinct uses of `impl Trait` result in different opaque types

For more information about this error, try `rustc --explain E0308`.

(playground)

Compared to these other cases that all do have the note:

fn f() {
    let mut f = || "";
    f = || "";
}

error[[E0308]](https://doc.rust-lang.org/stable/error_codes/E0308.html): mismatched types
 --> src/lib.rs:3:9
  |
2 |     let mut f = || "";
  |                 -- the expected closure
3 |     f = || "";
  |         ^^^^^ expected closure, found a different closure
  |
  = note: expected closure `[closure@src/lib.rs:2:17: 2:19]`
             found closure `[closure@src/lib.rs:3:9: 3:11]`
  = note: no two closures, even if identical, have the same type
  = help: consider boxing your closure and/or using it as a trait object

For more information about this error, try `rustc --explain E0308`.

(playground)

fn f() {
    let mut f = async {};
    f = async {};
}

error[[E0308]](https://doc.rust-lang.org/stable/error_codes/E0308.html): mismatched types
 --> src/lib.rs:3:9
  |
2 |     let mut f = async {};
  |                 --------
  |                 |
  |                 the expected `async` block
  |                 expected due to this value
3 |     f = async {};
  |         ^^^^^^^^ expected `async` block, found a different `async` block
  |
  = note: expected `async` block `[async block@src/lib.rs:2:17: 2:25]`
             found `async` block `[async block@src/lib.rs:3:9: 3:17]`

For more information about this error, try `rustc --explain E0308`.

(playground)

fn foo() {}
fn bar() {}

fn f() {
    let mut f = foo;
    f = bar;
}

error[[E0308]](https://doc.rust-lang.org/stable/error_codes/E0308.html): mismatched types
 --> src/lib.rs:6:9
  |
5 |     let mut f = foo;
  |                 --- expected due to this value
6 |     f = bar;
  |         ^^^ expected fn item, found a different fn item
  |
  = note: expected fn item `fn() {foo}`
             found fn item `fn() {bar}`
  = note: different fn items have unique types, even if their signatures are the same
  = help: consider casting both fn items to fn pointers using `as fn()`

For more information about this error, try `rustc --explain E0308`.

(playground)

Footnotes

1. async {} blocks also exist, but those do print a lot like closure types already. ↩

[estebank]

CC rust-lang/compiler-team#675

[apiraino]

mcp#675 has been accepted.

cc @steffahn for the next steps (also, a rebase)

@rustbot author

[Dylan-DPC]

@steffahn any updates on this? thanks

[steffahn]

Thanks for the ping. I missed that there was a call to action in the last reply. Sure, I can rebase and get this into working condition again, now with {…} instead of […].

[Dylan-DPC]

@steffahn any updates since the last check? thanks

[Dylan-DPC]

Closing this pr as it's been inactive for a long time. Feel free to reöpen if you plan on continuing otherwise preferably open a new pr and link the approved MCP.