Shorten types more when only interesting part is lifetimes#158992
Shorten types more when only interesting part is lifetimes#158992estebank wants to merge 14 commits into
Conversation
|
r? @mejrs rustbot has assigned @mejrs. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| = note: expected signature `fn(ValueRef<'1>) -> Result<(&'2 str, &'1 &'2 str), ...>` | ||
| found signature `fn(ValueRef<'1>) -> Result<(&'1 str, &'1 &'1 str), ...>` |
There was a problem hiding this comment.
This is not using the "highlighted expected/found" we use for E0308... It'll likely require extending that logic to also learn to replace lifetime names in the same way that Highlight does.
This comment has been minimized.
This comment has been minimized.
ff889d6 to
cd0972b
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thanks, this looks mostly okay to me. I have some thoughts.
@rustbot author
| write!(self, "{coroutine_kind}")?; | ||
| if coroutine_kind.is_fn_like() { | ||
| if self.should_truncate() { | ||
| write!(self, "@...}}")?; |
There was a problem hiding this comment.
I'm not a fan of printing @.... Here and elsewhere we end up printing something like {async closure@...}, {coroutine@...} and I think if shown out of context that'll be confusing.
Instead consider trimming the paths leading up to the file, so e.g. {async closure@dir/path/to/the/file.rs:LL:CC: LL:CC} becomes {async closure@file.rs:LL:CC: LL:CC}.
Or maybe we should just say {async closure}? 🤔
In any case most users will have module names much shorter than our test file's names, so I think the ui test output isn't really representative of what users get in the wild.
There was a problem hiding this comment.
I could swear that we have some logic doing that, writing only the filename if the path is too long and we're shortening things... I need to check more closely where we do that.
I'm trying out this (making the output {async closure}) and the one below (using _ instead of ...) and will push on its own commit to show what it looks like.
| | ^^^^^^^^^^^^^^^^^^^^ implementation of `FnOnce` is not general enough | ||
| | | ||
| = note: closure with signature `fn(&'0 Vec<i32>) -> std::slice::Iter<'_, i32>` must implement `FnOnce<(&'1 Vec<i32>,)>`, for any two lifetimes `'0` and `'1`... | ||
| = note: closure with signature `fn(&'0 ...) -> Iter<'_, i32>` must implement `FnOnce<(&'1 Vec<i32>,)>`, for any two lifetimes `'0` and `'1`... |
There was a problem hiding this comment.
This doesn't look like an improvement to me.
There was a problem hiding this comment.
The removed Vec<i32> or the std::slice?
There was a problem hiding this comment.
The fn(&'0 ...) part. ... in function signatures is a type (sort of) so this is an unacceptable regression in my eyes.
There was a problem hiding this comment.
Does the change to &'0 _ change the equation for you?
There was a problem hiding this comment.
It's still a bit weird, but I like it better.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@rustbot ready |
95cd4a6 to
fe43871
Compare
This comment has been minimized.
This comment has been minimized.
| @@ -1,15 +1,15 @@ | |||
| error[E0369]: cannot add `(..., ..., ..., ...)` to `(..., ..., ..., ...)` | |||
| error[E0369]: cannot add `((_, _, _, _), _, _, _)` to `((_, _, _, _), _, _, _)` | |||
There was a problem hiding this comment.
This is interesting, I think I like this better as well.
There was a problem hiding this comment.
Most of these do seem like improvements but I dunno about the closure type changes. Is it possible for these errors to ever be split across files, or located in such a large file, in such a way that the initial error line doesn't usefully indicate what I'm looking at? e.g. files with 5000 lines with 10 similar large closures that fill the screen.
But if we do want them, then I'm not sure we shouldn't just cut even more?
| @@ -15,7 +15,7 @@ error: lifetime may not live long enough | |||
| LL | let x = async move || { &s }; | |||
| | ------------- ^^^^^^ returning this value requires that `'1` must outlive `'2` | |||
| | | | | |||
| | | return type of async closure `{async closure body@$DIR/not-lending.rs:16:31: 16:37}` contains a lifetime `'2` | |||
| | | return type of async closure `{async closure body}` contains a lifetime `'2` | |||
There was a problem hiding this comment.
Is this {async closure body} doing anything here? The previous variant indicated the type's location in a useful way, but here "async closure" is a complete idea, and we're possibly just wasting our breath on saying "the async closure that, btw, is an async closure..." Really, rustc? You don't say?
An alternative way to reduce the number of characters here would just be
return type of `{async closure@$DIR/not-lending.rs:16:31:16:37}` contains a lifetime `'2`
This removes about 16 characters that at least seem to me like no one could benefit from... about a 5th of the amount of content that can easily be scanned on a line if one adheres to the "3 alphabets" rule... without reducing the actual information transmitted. If the body is important, then we can keep that and still save a fair amount "for free".
For why not remove the in-file location: I'm worried that we're potentially losing the span info here, and relying on the end of the closure being easily findable from the start.
There was a problem hiding this comment.
Also, pragmatically, even in a more conservative version, we could save more space by not including column numbers and mmmaybe intelligently collapsing them if they're on a single line? If someone puts multiple closures on a single line I do feel comfortable saying "...don't do that, though? or at least not if you want good diagnostics..." Sometimes generated code looks like that but formatters exist. That could look like...
return type of `{async closure body@$DIR/not-lending.rs:16}` contains a lifetime `'2`
There was a problem hiding this comment.
We can make a distinction between a top-level closure-like type that got shortened and one that is in a type parameter. It is still useful to see Result<{closure}, Error>, specially when it is highly nested. In these errors, the most important bit of information is the lifetimes involved, not the types themselves (because those already type checked so they are right).
We might be able to check "is the span label this name going to be used in pointing at the closure itself?" to completely avoid the type...
I want to point out that in type errors where the distinction of closures is more important, on the expected/found note we do print the closure's span's fs path, so the information might not be included in a span label.
I'm not happy with the rendering of closure-likes in general, but we can argue about that elsewhere :)
Calling these "async closure body return type" is weird indeed. I'm intrigued why we did that in the first place.
There was a problem hiding this comment.
Ok, looking at this more closely I realize what's going on:
The message is talking about "the return type for the closure being pointed at, which is {...}, contains lifetime 'x". That returned type is the evaluated type for the closure's body. So if we have |()| (), it would be (), but if we have |()| || (), it would be {closure@...}. This is confusing and removing the path is certainly a problem. But this gets even worse with async closures, because those get desugared to something like |_| async {} , so the return type of the outer closure is the async body. That's why we see these bizarre output. We need to fix an outright bug on these diagnostics when it comes to async closures.
There was a problem hiding this comment.
Partly addressed the above in the last commit: we now present the right type, but the primary span used in these errors is incorrect, it should be pointing at the tail expression of the async closure, instead of the whole body of the closure.
0e61216 to
b399d1a
Compare
This comment has been minimized.
This comment has been minimized.
| @@ -15,7 +15,7 @@ error: lifetime may not live long enough | |||
| LL | let x = async move || { &s }; | |||
| | ------------- ^^^^^^ returning this value requires that `'1` must outlive `'2` | |||
| | | | | |||
| | | return type of async closure `{async closure body@$DIR/not-lending.rs:16:31: 16:37}` contains a lifetime `'2` | |||
| | | return type of async closure is &'2 String | |||
There was a problem hiding this comment.
...Wow, yeah, my eyes had just kind of glazed over and I didn't really think about how this was... kinda babble? I'm glad I raised the point of "this should either keep more or cut more". This is enormously clearer.
There was a problem hiding this comment.
We still need to fix the Span to point at &s instead of { &s }, but I waded through that for a bit and I think I won't be fixing it myself anytime soon. 😬
This comment has been minimized.
This comment has been minimized.
We have a mechanims (`Highlight`) to change the name of inferred lifetimes in types so that we can refer to them by a number. Modify it so that it also avoids printing sub-parts of the type that are irrelevant to the situation at hand. The new behavior will print any sub part that has *any* lifetime (as they might be informative) instead of *just* the lifetimes being replaced. This was found to produce the best effect empirically in the test suite, striking a nice balance between shortening labels and being informative.
```
error: lifetime may not live long enough
--> $DIR/wrong-closure-arg-suggestion-125325.rs:46:18
|
LL | take(|_| to_fn(|_| self.counter += 1));
| --- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
| | |
| | return type of closure `aux::Map<{closure@...}>` contains a lifetime `'2`
| lifetime `'1` represents this closure's body
|
= note: closure implements `Fn`, so references to captured variables can't escape the closure
```
```
error: implementation of `FnOnce` is not general enough
--> $DIR/higher-ranked-auto-trait-15.rs:20:5
|
LL | require_send(future);
| ^^^^^^^^^^^^^^^^^^^^ implementation of `FnOnce` is not general enough
|
= note: closure with signature `fn(&'0 ...) -> std::slice::Iter<'_, i32>` must implement `FnOnce<(&'1 Vec<i32>,)>`, for any two lifetimes `'0` and `'1`...
= note: ...but it actually implements `FnOnce<(&Vec<i32>,)>`
```
Made this change after noticing that the first of the errors above had a corresponding `.stderr` file with lines longer than 150 columns.
```
error[E0700]: hidden type for `impl Iterator<Item = u32>` captures lifetime that does not appear in bounds
--> $DIR/static-return-lifetime-infered.rs:11:9
|
LL | fn iter_values<'a>(&'a self) -> impl Iterator<Item=u32> {
| -- ----------------------- opaque type defined here
| |
| hidden type `Map<std::slice::Iter<'a, ...>, {closure@...}>` captures the lifetime `'a` as defined here
LL | self.x.iter().map(|a| a.0)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
```
8310ea8 to
2836709
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
…n the *root* type This means that when we encounter a closure, we never hide it's path from the rendering, but if the closure is in a type parameter, it will get shortened. This should be fine for most lifetime errors to be understandable.
2836709 to
c6f5692
Compare
| | hidden type `Map<std::slice::Iter<'a, (u32, u32)>, {closure@$DIR/static-return-lifetime-infered.rs:11:27: 11:30}>` captures the lifetime `'a` as defined here | ||
| | hidden type `Map<Iter<'a, _>, {closure@...}>` captures the lifetime `'a` as defined here | ||
| LL | self.x.iter().map(|a| a.0) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
One thing to improve would be making this primary span mention "this type captures lifetime 'a".
There was a problem hiding this comment.
I like this, happy to approve it. Sorry/not sorry for sending you on this goose chase doing related changes 😆
Thanks to you and @workingjubilee for your work here. I'll give @workingjubilee some time to look over these changes.
View all comments
We have a mechanims (
Highlight) to change the name of inferred lifetimes in types so that we can refer to them by a number. Modify it so that it also avoids printing sub-parts of the type that are irrelevant to the situation at hand. The new behavior will print any sub part that has any lifetime (as they might be informative) instead of just the lifetimes being replaced. This was found to produce the best effect empirically in the test suite, striking a nice balance between shortening labels and being informative.Made this change after noticing that the first of the errors above had a corresponding
.stderrfile with lines longer than 150 columns by usingrg ".. \| .{150}" -g *.stderr.