Modify primary span label for E0308 #106399

estebank · 2023-01-03T05:05:58Z

Looking at the reactions to https://hachyderm.io/@ekuber/109622160673605438, a lot of people seem to have trouble understanding the current output, where the primary span label on type errors talks about the specific types that diverged, but these can be deeply nested type parameters. Because of that we could see "expected i32, found u32" in the label while the note said "expected Vec, found Vec". This understandably confuses people. I believe that once people learn to read these errors it starts to make more sense, but this PR changes the output to be more in line with what people might expect, without sacrificing terseness.

Fix #68220.

rustbot · 2023-01-03T05:06:04Z

r? @nagisa

(rustbot has picked a reviewer for you, use r? to override)

jyn514

I haven't read the code in detail, but the changes to the diagnostics look really nice; I especially appreciate showing the generic parameters to types and removing the struct/enum/reference prefix at the same time, it makes it feel more clear and less noisy at the same time :)

rustbot · 2023-01-04T04:54:43Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler/rustc_hir_analysis/src/check/check.rs

nagisa · 2023-01-05T23:37:31Z

src/test/ui/issues/issue-69306.stderr

@@ -89,7 +89,7 @@ error[E0308]: mismatched types
 LL | impl<T> S1<T, u8> {
   |      - this type parameter
 LL |     const C: S1<u8, u8> = Self(0, 1);
-   |                           ^^^^^^^^^^ expected `u8`, found type parameter `T`
+   |                           ^^^^^^^^^^ expected `S1<u8, u8>`, found `S1<T, u8>`


Hm, I think this runs a risk of being unreadable when the type has many big generics, or a small mismatching generic in the between multiple large ones (as could be the case with e.g. futures code, as we've have found out many times before.)

I think we should continue using the convention of _ out substs that already match and don't provide much value (at least after some threshold of type complexity.)

The label is using the "long types" machinery which will cap the width to at most 15 chars (or revert to the current behavior if that would cause the label to lose meaning). The "hiding matching generics with _" logic is not geared towards getting a text output, not can be directly joined with the long types one. I could see if I can make it easy to turn DiagnosticString into String, and then try them all in order and use whichever is shorter, but I do feel like the current approach "is enough" before bolting more logic onto a single PR (to make it easier to review each step independently).

nagisa · 2023-01-05T23:48:21Z

src/test/ui/issues/issue-24322.stderr

@@ -2,7 +2,7 @@ error[E0308]: mismatched types
  --> $DIR/issue-24322.rs:8:29
   |
 LL |     let x: &fn(&B) -> u32 = &B::func;
-   |            --------------   ^^^^^^^^ expected fn pointer, found fn item
+   |            --------------   ^^^^^^^^ expected `&for<'a> fn(&'a B) -> u32`, found `&for<'a> fn(&'a B) -> u32 {B::func}`


Hm, I think we should somehow try to not include the for<_>s here, if at all possible. This is not a syntax that is commonly encountered, and it is routinely a source of confusion or questions, so not revealing it unless truly necessary I think is a worthwhile improvement.

I can do that.

I went further and also removed the lifetime names in all trimmed paths, which might be contentious. I did it in two different commits so that we can drop the second one if you disagree with it.

nagisa · 2023-01-05T23:56:47Z

src/test/ui/diagnostic-width/long-E0308.stderr

@@ -16,7 +16,7 @@ LL |  |         Ok(Ok(Ok(Ok(Ok(Ok(Ok(Ok(Ok(Ok(Ok(Ok(Ok(Ok...
 LL |  |             Ok("")
 LL |  |         ))))))))))))))))))))))))))))))
 LL |  |     ))))))))))))))))))))))))))))));
-   |  |__________________________________^ expected struct `Atype`, found enum `Result`
+   |  |__________________________________^ expected `Atype<Btype<..., ...>, ...>`, found `Result<Result<..., ...>, ...>`


Is there a reason we're using ... instead of _?

Yeah, the long types machinery is using .... When that was merged I had changed it to _ but was asked to get it back to what you see now. I don't have a strong position on the matter.

nagisa · 2023-01-06T00:03:37Z

src/test/ui/argument-suggestions/two-mismatch-notes.stderr

@@ -4,14 +4,14 @@ error[E0308]: arguments to this function are incorrect
 LL |     foo(f, w);
   |     ^^^
   |
-note: expected `i32`, found `u32`
+note: expected fn pointer, found fn item


Hm, I think this note isn’t particularly helpful. Even harmful perhaps. “Compatible” fn items can always coerce to fn pointers, but the note may impress upon the reader that this isn’t always the case for some reason. So there is definitely something more the compiler could say. I think the 2nd note is pretty good and could potentially stay as the only note in this error.

Yeah, this is supposed to be "expected fn(i32), found fn(u32) {f}", but I didn't modify the labels for the special cases (like this one) to keep the diff much shorter. I can go all out and apply the changes in more places to fix this one case, but it will dramatically increase the size of the PR.

I think it should be ok to land this here, provided we later also have a structured suggestion for functions. I'm pretty sure this is not the only place where we are already having this unfortunate output and where we should be explicit with a note (which I thought we had) about that behavior.

nagisa · 2023-01-06T00:23:11Z

compiler/rustc_middle/src/ty/error.rs

-impl<'tcx> fmt::Display for TypeError<'tcx> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+impl<'tcx> TypeError<'tcx> {
+    pub fn to_string(self, tcx: TyCtxt<'tcx>) -> String {


Given that this is always in the error path, allocating a String is probably fine, though I think it would probably not be too onerous to retain the more optimal fmt approach by having a structure that combines TyCtxt with TypeError, somewhat like this:

impl<'tcx> TypeError<'tcx> { fn with(self, tcx: TyCtxt<'tcx>) -> TypeErrorWithTyCtxt<'tcx> { todo!() } } impl<'tcx> std::fmt::Display for TypeErrorWithTyCtxt<'tcx> { ... }

It would probably be terser, and I think we use this kind of pattern elsewhere in the codebase already, so it isn’t that unconventional.

Maybe turning this to be Cow would be good enough?

compiler/rustc_middle/src/ty/error.rs

estebank · 2023-01-06T20:20:50Z

src/test/ui/argument-suggestions/two-mismatch-notes.stderr

  --> $DIR/two-mismatch-notes.rs:10:9
   |
 LL |     foo(f, w);
   |         ^
   = note: expected fn pointer `fn(i32)`
                 found fn item `fn(u32) {f}`
-note: expected `i32`, found `isize`
+   = note: when the arguments and return types match, functions can be coerced to function pointers


Added this note to partially address the comment above.

bors · 2023-01-18T10:06:42Z

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

nagisa

r=me. This looks great. I could only muster enough willpower to look through like 50% of the changed stderrs, but I didn’t see any moajor regressions in what I’ve looked at and overall improvement is very welcome and nice!

Sorry for the late review!

The previous output was unintuitive to users.

estebank · 2023-01-30T22:03:14Z

@bors r=nagisa

bors · 2023-01-30T22:03:16Z

📌 Commit 449dfc6 has been approved by nagisa

It is now in the queue for this repository.

bors · 2023-01-31T13:53:44Z

⌛ Testing commit 449dfc6 with merge f361413...

bors · 2023-01-31T17:24:58Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing f361413 to master...

rust-timer · 2023-01-31T18:45:48Z

Finished benchmarking commit (f361413): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.6%	[1.3%, 4.0%]	4
Regressions ❌ (secondary)	2.3%	[0.6%, 6.9%]	42
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[1.3%, 4.0%]	4

Cycles

This benchmark run did not return any relevant results for this metric.

rustbot assigned nagisa Jan 3, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 3, 2023

This comment was marked as outdated.

Sign in to view

estebank force-pushed the type-err-span-label branch from 10e1dc0 to 3ccdaab Compare January 3, 2023 05:57

This comment was marked as resolved.

Sign in to view

estebank force-pushed the type-err-span-label branch from 3ccdaab to c1caae9 Compare January 3, 2023 06:39

jyn514 approved these changes Jan 3, 2023

View reviewed changes

estebank force-pushed the type-err-span-label branch from c1caae9 to 5fad8b5 Compare January 3, 2023 18:39

This comment was marked as resolved.

Sign in to view

estebank force-pushed the type-err-span-label branch from 5fad8b5 to dc6cdf0 Compare January 4, 2023 04:54

estebank force-pushed the type-err-span-label branch 2 times, most recently from af1e93b to 9eecea8 Compare January 4, 2023 05:57

This comment was marked as resolved.

Sign in to view

estebank force-pushed the type-err-span-label branch from 9eecea8 to f316d81 Compare January 5, 2023 07:07

nagisa reviewed Jan 6, 2023

View reviewed changes

estebank force-pushed the type-err-span-label branch 2 times, most recently from 2e55fe8 to 6e9f60b Compare January 6, 2023 18:41

estebank commented Jan 6, 2023

View reviewed changes

estebank force-pushed the type-err-span-label branch 2 times, most recently from 98f610d to c36437b Compare January 11, 2023 21:40

This comment was marked as resolved.

Sign in to view

estebank force-pushed the type-err-span-label branch from c36437b to 5a8f2a3 Compare January 12, 2023 18:26

nagisa approved these changes Jan 23, 2023

View reviewed changes

estebank added 3 commits January 30, 2023 20:12

Modify primary span label for E0308

62ba3e7

The previous output was unintuitive to users.

review comments

3fa95b8

Don't show for<'lt> in force trimmed paths

81973a3

estebank added 7 commits January 30, 2023 20:12

Do not mention lifetime names in force trimmed paths

252c43b

Hide lifetimes of impl Trait in force trimmed paths

ab8dc9a

Tweak use of trimmed paths

153ed09

Mention fn coercion rules (needs to be expanded)

5ae8e23

Tweak E0271 wording

6c2c8ed

Make structured suggestion for fn casting verbose

d868357

Fix unquoted projection types in label

449dfc6

estebank force-pushed the type-err-span-label branch from 5a8f2a3 to 449dfc6 Compare January 30, 2023 22:02

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 Jan 30, 2023

bors mentioned this pull request Jan 31, 2023

small refactor to new projection code #107348

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 31, 2023

bors merged commit f361413 into rust-lang:master Jan 31, 2023

rustbot added this to the 1.69.0 milestone Jan 31, 2023

This was referenced Jan 31, 2023

Remove identity_future indirection #104833

Merged

Remove a few actually_rustdoc uses #107289

Closed

Add display of lifetime of Ref in fn fmt() for TypeError #107475

Closed

estebank deleted the type-err-span-label branch November 9, 2023 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify primary span label for E0308 #106399

Modify primary span label for E0308 #106399

estebank commented Jan 3, 2023 •

edited

Loading

rustbot commented Jan 3, 2023

This comment was marked as outdated.

This comment was marked as resolved.

jyn514 left a comment

This comment was marked as resolved.

rustbot commented Jan 4, 2023

This comment was marked as resolved.

nagisa Jan 5, 2023

estebank Jan 6, 2023

nagisa Jan 5, 2023

estebank Jan 6, 2023

estebank Jan 6, 2023 •

edited

Loading

nagisa Jan 5, 2023

estebank Jan 6, 2023

nagisa Jan 6, 2023

estebank Jan 6, 2023

estebank Jan 6, 2023

nagisa Jan 6, 2023

estebank Jan 6, 2023

estebank Jan 6, 2023

This comment was marked as resolved.

bors commented Jan 18, 2023

nagisa left a comment

estebank commented Jan 30, 2023

bors commented Jan 30, 2023

bors commented Jan 31, 2023

bors commented Jan 31, 2023

rust-timer commented Jan 31, 2023

Modify primary span label for E0308 #106399

Modify primary span label for E0308 #106399

Conversation

estebank commented Jan 3, 2023 • edited Loading

rustbot commented Jan 3, 2023

This comment was marked as outdated.

This comment was marked as resolved.

jyn514 left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

rustbot commented Jan 4, 2023

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

bors commented Jan 18, 2023

nagisa left a comment

Choose a reason for hiding this comment

estebank commented Jan 30, 2023

bors commented Jan 30, 2023

bors commented Jan 31, 2023

bors commented Jan 31, 2023

rust-timer commented Jan 31, 2023

Overall result: no relevant changes - no action needed

estebank commented Jan 3, 2023 •

edited

Loading

estebank Jan 6, 2023 •

edited

Loading