rust
rust copied to clipboard
rust-lang
More distinctive pretty-printing of function item types
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]`
r? @davidtwco
(rust-highfive has picked a reviewer for you, use r? to override)
I’m wondering if there’s a way (and a desire) to (be able to) test colors of compiler output, for the highlighting logic.
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
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[^1] or that close over different generics[^2] differ. Are there any other cases where we generate a fresh type that doesn't have a user-writable name?
[^1]: `impl Sized` (opaque type at <src/lib.rs:1:12>)
[^2]: `impl Sized` (`u8`)
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.)
We have access to tcx in pretty printing, right? We should be able to check the
def_kindand special case for constructors, if necessary. Though that can be done in a follow-up.
Yeah, I thought follow-up.
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
The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Finished release [optimized] target(s) in 0.17s
Check compiletest suite=mir-opt mode=mir-opt (x86_64-unknown-linux-gnu -> i686-unknown-linux-gnu)
running 181 tests
i....F.................................i........................................i....... 88/181
........i........F.........F...F.............F..............i........................... 176/181
Some tests failed in compiletest suite=mir-opt mode=mir-opt host=x86_64-unknown-linux-gnu target=i686-unknown-linux-gnu
failures:
---- [mir-opt] src/test/mir-opt/array-index-is-temporary.rs stdout ----
---- [mir-opt] src/test/mir-opt/array-index-is-temporary.rs stdout ----
39 _5 = foo(move _6) -> bb1; // scope 4 at $DIR/array-index-is-temporary.rs:+4:21: +4:27
40 // mir::Constant
41 // + span: $DIR/array-index-is-temporary.rs:16:21: 16:24
- // + literal: Const { ty: unsafe fn(*mut usize) -> u32 {foo}, val: Value(<ZST>) }
+ // + literal: Const { ty: [fn item {foo}: unsafe fn(*mut usize) -> u32], val: Value(<ZST>) }
44
45 bb1: {
thread '[mir-opt] src/test/mir-opt/array-index-is-temporary.rs' panicked at 'Actual MIR output differs from expected MIR output /checkout/src/test/mir-opt/array_index_is_temporary.main.SimplifyCfg-elaborate-drops.after.32bit.mir', src/tools/compiletest/src/runtest.rs:3513:25
---- [mir-opt] src/test/mir-opt/inline/inline-into-box-place.rs stdout ----
---- [mir-opt] src/test/mir-opt/inline/inline-into-box-place.rs stdout ----
28 _4 = alloc::alloc::exchange_malloc(move _2, move _3) -> bb1; // scope 2 at $DIR/inline-into-box-place.rs:+1:29: +1:43
29 // mir::Constant
30 // + span: $DIR/inline-into-box-place.rs:8:29: 8:43
- // + literal: Const { ty: unsafe fn(usize, usize) -> *mut u8 {alloc::alloc::exchange_malloc}, val: Value(<ZST>) }
+ // + literal: Const { ty: [fn item {alloc::alloc::exchange_malloc}: unsafe fn(usize, usize) -> *mut u8], val: Value(<ZST>) }
33
34 bb1: {
44 // mir::Constant
44 // mir::Constant
45 - // + span: $DIR/inline-into-box-place.rs:8:33: 8:41
46 - // + user_ty: UserType(1)
- - // + literal: Const { ty: fn() -> Vec<u32> {Vec::<u32>::new}, val: Value(<ZST>) }
+ - // + literal: Const { ty: [fn item {Vec::<u32>::new}: fn() -> Vec<u32>], val: Value(<ZST>) }
49 -
50 - bb2: {
75 - _6 = alloc::alloc::box_free::<Vec<u32>, std::alloc::Global>(move (_5.0: std::ptr::Unique<std::vec::Vec<u32>>), move (_5.1: std::alloc::Global)) -> bb5; // scope 0 at $DIR/inline-into-box-place.rs:+1:42: +1:43
76 - // mir::Constant
77 - // + span: $DIR/inline-into-box-place.rs:8:42: 8:43
- - // + literal: Const { ty: unsafe fn(Unique<Vec<u32>>, std::alloc::Global) {alloc::alloc::box_free::<Vec<u32>, std::alloc::Global>}, val: Value(<ZST>) }
+ - // + literal: Const { ty: [fn item {alloc::alloc::box_free::<Vec<u32>, std::alloc::Global>}: unsafe fn(Unique<Vec<u32>>, std::alloc::Global)], val: Value(<ZST>) }
80 -
80 -
81 - bb5 (cleanup): {
thread '[mir-opt] src/test/mir-opt/inline/inline-into-box-place.rs' panicked at 'Actual MIR output differs from expected MIR output /checkout/src/test/mir-opt/inline/inline_into_box_place.main.Inline.32bit.diff', src/tools/compiletest/src/runtest.rs:3513:25
---- [mir-opt] src/test/mir-opt/issue-72181.rs stdout ----
---- [mir-opt] src/test/mir-opt/issue-72181.rs stdout ----
25 _1 = std::mem::size_of::<Foo>() -> [return: bb1, unwind: bb3]; // scope 0 at $DIR/issue-72181.rs:+1:13: +1:34
26 // mir::Constant
27 // + span: $DIR/issue-72181.rs:24:13: 24:32
- // + literal: Const { ty: fn() -> usize {std::mem::size_of::<Foo>}, val: Value(<ZST>) }
+ // + literal: Const { ty: [fn item {std::mem::size_of::<Foo>}: fn() -> usize], val: Value(<ZST>) }
30
31 bb1: {
thread '[mir-opt] src/test/mir-opt/issue-72181.rs' panicked at 'Actual MIR output differs from expected MIR output /checkout/src/test/mir-opt/issue_72181.main.mir_map.0.32bit.mir', src/tools/compiletest/src/runtest.rs:3513:25
---- [mir-opt] src/test/mir-opt/issue-73223.rs stdout ----
---- [mir-opt] src/test/mir-opt/issue-73223.rs stdout ----
135 _21 = core::panicking::assert_failed::<i32, i32>(const core::panicking::AssertKind::Eq, move _23, move _25, move _27); // scope 5 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
136 // mir::Constant
137 // + span: $SRC_DIR/core/src/macros/mod.rs:LL:COL
- // + literal: Const { ty: for<'r, 's, 't0> fn(core::panicking::AssertKind, &'r i32, &'s i32, Option<Arguments<'t0>>) -> ! {core::panicking::assert_failed::<i32, i32>}, val: Value(<ZST>) }
+ // + literal: Const { ty: [fn item {core::panicking::assert_failed::<i32, i32>}: for<'r, 's, 't0> fn(core::panicking::AssertKind, &'r i32, &'s i32, Option<Arguments<'t0>>) -> !], val: Value(<ZST>) }
139 // mir::Constant
140 // + span: $SRC_DIR/core/src/macros/mod.rs:LL:COL
141 // + literal: Const { ty: core::panicking::AssertKind, val: Value(Scalar(0x00)) }
thread '[mir-opt] src/test/mir-opt/issue-73223.rs' panicked at 'Actual MIR output differs from expected MIR output /checkout/src/test/mir-opt/issue_73223.main.SimplifyArmIdentity.32bit.diff', src/tools/compiletest/src/runtest.rs:3513:25
---- [mir-opt] src/test/mir-opt/nll/region-subtyping-basic.rs stdout ----
---- [mir-opt] src/test/mir-opt/nll/region-subtyping-basic.rs stdout ----
70 StorageLive(_8); // bb2[0]: scope 3 at $DIR/region-subtyping-basic.rs:+5:9: +5:18
71 StorageLive(_9); // bb2[1]: scope 3 at $DIR/region-subtyping-basic.rs:+5:15: +5:17
72 _9 = (*_6); // bb2[2]: scope 3 at $DIR/region-subtyping-basic.rs:+5:15: +5:17
- _8 = ConstValue(ZeroSized: fn(usize) -> bool {use_x})(move _9) -> [return: bb3, unwind: bb7]; // bb2[3]: scope 3 at $DIR/region-subtyping-basic.rs:+5:9: +5:18
+ _8 = ConstValue(ZeroSized: [fn item {use_x}: fn(usize) -> bool])(move _9) -> [return: bb3, unwind: bb7]; // bb2[3]: scope 3 at $DIR/region-subtyping-basic.rs:+5:9: +5:18
74 // mir::Constant
75 // + span: $DIR/region-subtyping-basic.rs:21:9: 21:14
- // + literal: Const { ty: fn(usize) -> bool {use_x}, val: Value(<ZST>) }
+ // + literal: Const { ty: [fn item {use_x}: fn(usize) -> bool], val: Value(<ZST>) }
78
79 bb3: {
85
85
86 bb4: {
87 StorageLive(_10); // bb4[0]: scope 3 at $DIR/region-subtyping-basic.rs:+7:9: +7:18
- _10 = ConstValue(ZeroSized: fn(usize) -> bool {use_x})(const ConstValue(Scalar(0x00000016): usize)) -> [return: bb5, unwind: bb7]; // bb4[1]: scope 3 at $DIR/region-subtyping-basic.rs:+7:9: +7:18
+ _10 = ConstValue(ZeroSized: [fn item {use_x}: fn(usize) -> bool])(const ConstValue(Scalar(0x00000016): usize)) -> [return: bb5, unwind: bb7]; // bb4[1]: scope 3 at $DIR/region-subtyping-basic.rs:+7:9: +7:18
89 // mir::Constant
90 // + span: $DIR/region-subtyping-basic.rs:23:9: 23:14
- // + literal: Const { ty: fn(usize) -> bool {use_x}, val: Value(<ZST>) }
+ // + literal: Const { ty: [fn item {use_x}: fn(usize) -> bool], val: Value(<ZST>) }
93
94 bb5: {
thread '[mir-opt] src/test/mir-opt/nll/region-subtyping-basic.rs' panicked at 'Actual MIR output differs from expected MIR output /checkout/src/test/mir-opt/nll/region_subtyping_basic.main.nll.0.32bit.mir', src/tools/compiletest/src/runtest.rs:3513:25
---- [mir-opt] src/test/mir-opt/unusual-item-types.rs stdout ----
---- [mir-opt] src/test/mir-opt/unusual-item-types.rs stdout ----
34 _3 = <Vec<i32> as Drop>::drop(move _2) -> [return: bb5, unwind: bb4]; // scope 0 at $SRC_DIR/core/src/ptr/mod.rs:+0:1: +0:56
35 // mir::Constant
36 // + span: $SRC_DIR/core/src/ptr/mod.rs:LL:COL
- // + literal: Const { ty: for<'r> fn(&'r mut Vec<i32>) {<Vec<i32> as Drop>::drop}, val: Value(<ZST>) }
+ // + literal: Const { ty: [fn item {<Vec<i32> as Drop>::drop}: for<'r> fn(&'r mut Vec<i32>)], val: Value(<ZST>) }
39 }
40
thread '[mir-opt] src/test/mir-opt/unusual-item-types.rs' panicked at 'Actual MIR output differs from expected MIR output /checkout/src/test/mir-opt/unusual_item_types.core.ptr-drop_in_place.Vec_i32_.AddMovesForPackedDrops.before.32bit.mir', src/tools/compiletest/src/runtest.rs:3513:25
failures:
[mir-opt] src/test/mir-opt/array-index-is-temporary.rs
[mir-opt] src/test/mir-opt/inline/inline-into-box-place.rs
@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
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.)
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.
The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
.....
failures:
Some tests failed in compiletest suite=mir-opt mode=mir-opt host=x86_64-unknown-linux-gnu target=i686-unknown-linux-gnu
---- [mir-opt] src/test/mir-opt/issue-73223.rs stdout ----
98 _14 = core::panicking::assert_failed::<i32, i32>(const core::panicking::AssertKind::Eq, move _15, move _17, move _19); // scope 5 at $SRC_DIR/core/src/macros/mod.rs:LL:COL
99 // mir::Constant
100 // + span: $SRC_DIR/core/src/macros/mod.rs:LL:COL
- // + literal: Const { ty: for<'r, 's, 't0> fn(core::panicking::AssertKind, &'r i32, &'s i32, Option<Arguments<'t0>>) -> ! {core::panicking::assert_failed::<i32, i32>}, val: Value(<ZST>) }
+ // + literal: Const { ty: [fn item {core::panicking::assert_failed::<i32, i32>}: for<'r, 's, 't0> fn(core::panicking::AssertKind, &'r i32, &'s i32, Option<Arguments<'t0>>) -> !], val: Value(<ZST>) }
102 // mir::Constant
103 // + span: $SRC_DIR/core/src/macros/mod.rs:LL:COL
104 // + literal: Const { ty: core::panicking::AssertKind, val: Value(Scalar(0x00)) }
thread '[mir-opt] src/test/mir-opt/issue-73223.rs' panicked at 'Actual MIR output differs from expected MIR output /checkout/src/test/mir-opt/issue_73223.main.PreCodegen.32bit.diff', src/tools/compiletest/src/runtest.rs:3513:25
failures:
[mir-opt] src/test/mir-opt/issue-73223.rs
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: 65eb3401f62e78c38e4d7edd5a1688352346c94e and cb0db7f36a56185648e067f3fab327b6db4287c0); 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.
Indeed, I just overlooked DefKind::AssocFn. I’ll push new commits soon, and then upload screenshots of the highlighting, too.
For now, I’ve used [constructor of {Foo}: fn() -> Foo]-style printing for tuple constructors (not further distinguishing between enum and struct ones). (I.e. fn item becomes constructor of.)
Here’s screenshots:
fn foo() {
fn f() {}
fn g() {}
fn g2() -> u8 {
0
}
fn g3(_: u8) {}
fn h<T>() {}
fn h2<T>(_: T) {}
struct Struct();
let mut x = f;
x = || ();
x = (|| ()) as fn();
x = (|| 0) as fn() -> u8;
x = (|_| ()) as fn(u8);
x = g;
x = g2;
x = g3;
x = Struct;
let mut y = h::<()>;
y = h::<u8>;
let mut y2 = h2::<()>;
y = h2::<u8>;
let mut z = g3;
z = (|| ()) as fn();
let mut p = (|| ()) as fn();
p = (|| 0) as fn() -> u8;
p = (|_| ()) as fn(u8);
let _: fn() -> u8 = g3;
let _: fn() -> u16 = g2;
}
fn main() {}
CI is passing
@rustbot ready
I can re-order and squash some commits before merging if that’s desired
All those "bless"/"update" commits could get squashed into their parents, since those aren't really functional changes.
All those "bless"/"update" commits could get squashed into their parents, since those aren't really functional changes.
I know. Also the multiple iterations on the highlighting logic aren’t really valuable to keep. My main conditional was not only about squashing if but (implicitly) also “only when it’s desired”, since squashing messes with reviews, especially since the first commits up to 7ce3e43ca5a6526d37a7b5fe91c63936c0959cd7 had already been reviewed.
@compiler-errors
Upon second read, what do you mean “into their parents”? I think it makes it easier to understand what’s going on if automatically-generated changes by --bless are kept separate from the changes that caused these tests to fail/change. I.e. the compiler-changes, …test-cases.rs-changes, and …test-cases.stderr-changes feel appropriate split up into 3 commits IMO.
I was mostly considering squashing the multiple iterations of test-case changes (due to the later addition of special-casing constructors; and perhaps due to upcoming changes if a somehow slightly different pretty-printing is desired) into a single set of change.
I mostly meant "bless -> bless -> bless" could get squashed, but I guess this convo doesn't really matter -- my mistake for not reading "before merging" on your original diff. I trust that you can clean up the history sufficiently once it's approved.
I don't think it's useful to separate changes to UI tests (i.e. edits to src/test/ui/**/*.rs) from their --bless invocations (edits to src/test/ui/**/*.stderr), but to each their own.
I just recalled why I remembered the highlighting logic that’s supposed to make comparing something like fn(u8) to [fn item {foo}: fn()] easier (in this case by highlighting the parentheses of the fn(…)s to pull attention to the mismatching signature in the [fn item {foo}: fn()], too) to be more nontrivial than what I perceived it like today. I.e., I want to tweak this code further. @rustbot author
Oh well… I think “fixing” the logic becomes unnecessarily complicated. For more context, I thought that a diff like
fn()
[fn item {foo}: fn(u32)]
would be nicer than
fn()
[fn item {foo}: fn(u32)]
because the function signatures don’t line up at all, whereas for something like
fn()
fn(u32)
i.e. where it does line up, it’s clearer what the highlighted u32 relates to.
But generalizing this “correctly” is too complex and not worth it, so those new complications are now removed entirely. The screenshots are outdated now…
@rustbot ready
Screenshots, updated (same tests as before… perhaps some are slightly redundant now, since those were designed to also showcase some cases of the parentheses-highlighting logic, but more tests don’t hurt):
fn foo() {
fn f() {}
fn g() {}
fn g2() -> u8 {
0
}
fn g3(_: u8) {}
fn h<T>() {}
fn h2<T>(_: T) {}
struct Struct();
let mut x = f;
x = || ();
x = (|| ()) as fn();
x = (|| 0) as fn() -> u8;
x = (|_| ()) as fn(u8);
x = g;
x = g2;
x = g3;
x = Struct;
let mut y = h::<()>;
y = h::<u8>;
let mut y2 = h2::<()>;
y = h2::<u8>;
let mut z = g3;
z = (|| ()) as fn();
let mut p = (|| ()) as fn();
p = (|| 0) as fn() -> u8;
p = (|_| ()) as fn(u8);
let _: fn() -> u8 = g3;
let _: fn() -> u16 = g2;
}
First of all, I love the work you're putting on this. I have a couple of concerns. 1) I generally worry about changing the output too drastically all at once, due to users maybe losing on the familiarity they've built already. This is fine most of the time, and I don't think it applies here, but I wanted to mention it. 2) It seems like the output for the case in line 17 is slightly easier to understand today than with the new output. I think we can improve the output by explicitly looking for cases where we have different signatures and if the signature were to be fixed, then the type error wouldn't happen because the coercion would successfully take over, and produce output that only mentions the signature (like a missing return type).
As a simultaneously more general and more bikesheddy feedback, my aesthetic preferences would change something like [fn item {my_const_fn}: fn(u8) -> u8] to [item my_const_fn: fn(u8) -> u8].
Also, I know that @BoxyUwU was working on a different formatter for the output of mismatched arguments, where spaces are introduced to make the signatures vertically align, and it'd be great if we could look into introducing that into these cases, so that we could have something like (not too happy with it, but you get the idea):
= note: expected fn item `[fn item foo::<u8>: fn(_, _) -> _]`
found fn pointer ` fn(_ ) -> _`
= help: change the expected type to be function
It might be that even fn item is redundant, it could be foo::<u8>: fn(_, _) -> _, as it shouldn't be too confusing.
You had a good point in the internals thread, about the distinction of fn item and fn pointer being too subtle and not highlighted. Maybe we should also modify that for all E0308 and highlight the parts of the item description that differs always?
It seems like the output for the case in line 17 is slightly easier to understand today than with the new output. I think we can improve the output by explicitly looking for cases where we have different signatures and if the signature were to be fixed, then the type error wouldn't happen because the coercion would successfully take over, and produce output that only mentions the signature (like a missing return type).
I don’t think line 17 is such a case. Maybe you mean lines 36 and 37?
In general, this seems to be impossible to determine in this highlighting logic alone. It’s called both in cases where coercion would be possible, and (e.g. recursively[^1]) in cases where it would not be possible. As a long-term improvement, maybe it’s possible to somehow link up highlighting and the type-checking that generated the error message in the first place, so that always either only the place that the actual error message talks about is highlighted, or such that this place is somehow highlighted stronger, perhaps with color, or maybe with an underline (which would also help in color-less environments). But I wouldn’t know how to pull that off, I haven’t looked at the relevant type-checking logic at all, so far.
[^1]: Except… maybe that’s the only case where coercion is not an option? In case that’s actually true, then an additional bool argument to cmp could track whether or not we are in a place where coercion is allowed (not a recursive call). Also last time I checked, there were actually two places in rustc source code that called the cmp function as an entry point (i.e. calls that aren’t recursive calls), I haven’t understood where the second one comes from yet, and whether or not it’s a place that supports coercion.
I suppose, if it is possible to reliably recognize cases where coercion is possible, then one could both skip highlighting of the fn item {path…} part, and perhaps even simplify it to fn item {_} if coercion is possible and the signatures don’t match, and similarly, it might make sense to not highlight the function signatures at all in cases where coercion isn’t possible. Actually… even nicer for the former would be a dedicated error message about the “fn-item-to-fn-pointer” coercion failing, and then that error message only needs to print the function signatures.
a different formatter for the output of mismatched arguments, where spaces are introduced to make the signatures vertically align, and it'd be great if we could look into introducing that into these cases, so that we could have something like (not too happy with it, but you get the idea):
I’d not a huge fan of introducing spaces like that. Vertically aligning stuff in Rust source code is super rare (because rustfmt is popular and it doesn’t align stuff), so from a familiarity standpoint I would thus consider something like
= note: expected fn pointer `fn(Result<Option<[u8; 1024]>, std::io::Error>) -> _`
found fn pointer `fn( ) -> _`
to be rather hard to read. And once the type does no longer fit into a single line, the benefits are mostly lost, anyway.
my aesthetic preferences would change something like
[fn item {my_const_fn}: fn(u8) -> u8]to[item my_const_fn: fn(u8) -> u8]
It might be that even
fn itemis redundant, it could befoo::<u8>: fn(_, _) -> _, as it shouldn't be too confusing.
For the latter, this reads perhaps a bit too much like Rust syntax, suggesting that the type of foo::<u8> is literally fn(_, _) -> _ (a function pointer type). The colon is in my approach, too, but only because I considered something like [fn item {foo::<u8>} with signature fn(_, _) -> _], that’s more explicit that this is not specifying the type but only the signature of foo::<u8>, a bit too lengthy.
For either approach, I don’t think that path::to::some_function: reads particularly well, since there’s so many colons in the path already. Also the longer the path becomes the more useful I feel the additional {} becomes as an easier-to-find visual indicator for where the end of the path is. E.g. here’s two examples from the changed test outputs:
[item <X as T>::bal: for<'r> fn(&'r X) -> usize]
vs.
[fn item {<X as T>::bal}: for<'r> fn(&'r X) -> usize]
and
<[item core::f32::<impl f32>::to_bits::ct_f32_to_u32: fn(f32) -> u32] as FnOnce<(f32,)>>::call_once
vs.
<[fn item {core::f32::<impl f32>::to_bits::ct_f32_to_u32}: fn(f32) -> u32] as FnOnce<(f32,)>>::call_once
The only potential problem with the syntax I see so far is the use of []s (though that’s somewhat consistent with other unnamable types, i.e. closures and generators) because those can look like slices, e.g. in
error[E0308]: mismatched types
--> src/main.rs:17:15
|
17 | func: &foo, //~ ERROR mismatched types
| ^^^^ expected fn pointer, found fn item
|
= note: expected reference `&fn() -> Option<isize>`
found reference `&[fn item {foo}: fn() -> Option<isize>]`
Just realized that we already show the description for the type, so we could include the fn name in it instead (which might be a bad idea for really long paths...):
= note: expected fn item `foo::<u8>` `fn(_, _) -> _ {...}`
found fn pointer `fn(_) -> _`
We still need something (like the {...} above) to differentiate items from pointers because they can appear in nested places or in messages on their own, but when they appear in the message or a span label, they usually point at the problem anyways (so it shouldn't add additional confusion).
The problematic case is
error[E0308]: mismatched types
--> src/main.rs:17:15
|
17 | func: &foo, //~ ERROR mismatched types
| ^^^^ expected fn pointer, found fn item
|
= note: expected reference `&fn() -> Option<isize> {...}`
found reference `&fn() -> Option<isize> {...}`
where now we imply we have two different functions with the same signature. Beyond this PR, we should detect such cases and emit the "you likely want an fn pointer instead" suggestion/help in these cases, but it presents a problem when eliding potentially useful info.
An option here would be to use only the name in the item signature, but the full path in the description. That would ensure a relatively short ident and give a fighting chance to users, but would still be terrible in a case where a::foo and b::foo have the same signature (but we already a similar problem for different crate versions in the dep tree).
Edit: wait, what am I saying, the formatter for messages goes through pprint, while for E0308 goes through cmp, we can make them diverge. Given the fn [1]() -> () proposal below, for pprint we could use fn [foo]() -> () to have some consistency.
All of these cases could be checked for in E0308, and use that info to emit a note:
error[E0308]: mismatched types
--> src/main.rs:17:15
|
17 | func: &foo, //~ ERROR mismatched types
| ^^^^ expected fn pointer, found fn item
|
= note: expected reference `&fn [1]() -> Option<isize>`
found reference `&fn [2]() -> Option<isize>`
= note: fn item [1]: `a::foo`
fn item [2]: `b::foo`
And having that functionality gives us for free the hook we need to mention that different functions are different even if their signature is the same, and to use fn pointers instead.
Edit: this ability to remove subparts of the type is something we'd want for really long types already, so it could even be extended to replace any of the places we usually highlight with a number, if its display length is longer than N. So in this case we might use the path as part of the type, but if the whole fn signature ends up being longer than, lets say 20 chars, we use the numbers.
I don’t think line 17 is such a case. Maybe you mean lines 36 and 37?
Yes, indeed, I got the expected/found swapped in my head.
such that this place is somehow highlighted stronger, perhaps with color, or maybe with an underline (which would also help in color-less environments).
Right now the message highlighting only supports bolding (which might be enough), but there's no reason not to extend its feature set. It'd be a smaller change than adding highlighting in the first place was.
I know I'm somewhat derailing the conversation by exploring other alternatives in the output, but I'm sure we all agree we want this to be the best it can be :)