Stream: t-compiler/wg-nll

Topic: reformatting diagnostics


nikomatsakis (Oct 16 2018 at 20:20, on Zulip):

I was leaning a different way

nikomatsakis (Oct 16 2018 at 20:21, on Zulip):

I was thinking of detecting cases where combining all labels into one was problematic

nikomatsakis (Oct 16 2018 at 20:21, on Zulip):

and trying to break things up into multi-part messages (automatically)

nikomatsakis (Oct 16 2018 at 20:21, on Zulip):

I would also like this for cases where the labels should be read in a particular order

nikomatsakis (Oct 16 2018 at 20:21, on Zulip):

but sometimes that order doesn't happen to be the one in which they appear in the source

nikomatsakis (Oct 16 2018 at 20:21, on Zulip):

(and this can't be easily predicted)

nikomatsakis (Oct 16 2018 at 20:21, on Zulip):

anyway..

pnkfelix (Oct 16 2018 at 20:21, on Zulip):

yeah I've been struggling with that

pnkfelix (Oct 16 2018 at 20:22, on Zulip):

our use of ellipses in diagnsotics is an attempt to compensate for that

pnkfelix (Oct 16 2018 at 20:22, on Zulip):

but it clearly is a problem at times

nikomatsakis (Oct 16 2018 at 20:22, on Zulip):

right, I think maybe we should "fall back" to something more like the older style

nikomatsakis (Oct 16 2018 at 20:22, on Zulip):

or at least I wanted to toy with it

davidtwco (Oct 16 2018 at 20:24, on Zulip):

By "older style", do you mean when there's lots of snippets stacked atop each other in the error?

nikomatsakis (Oct 16 2018 at 20:24, on Zulip):

hmm just had another idea :) but we should definitely break this out into a different topic

nikomatsakis (Oct 16 2018 at 20:24, on Zulip):

I meant something where there are many "blocks" each with their own snippet + label

memoryruins (Oct 16 2018 at 20:27, on Zulip):

"blocks" :thinking: ┰ ┖

memoryruins (Oct 16 2018 at 20:28, on Zulip):

(that could help with the linear reading)

memoryruins (Oct 16 2018 at 20:29, on Zulip):

how do you imagine would look for the error message before?

davidtwco (Oct 16 2018 at 20:29, on Zulip):

I couldn't find a good example from the top of my head, but it's roughly something like this that I wasn't a fan of:

error[E0597]: this can't happen
  --> $DIR/some_test.rs
   |
LL |  main error code
   |
= note: something happens that must live until...
   |
LL |  not really relevant
   |
= note: ...this is because something is used here...
   |
LL |  not really relevant
   |
= note: ...which must live until here or something like that.
   |
LL |  not really relevant
   |
davidtwco (Oct 16 2018 at 20:30, on Zulip):

I don't think NLL has any errors like that

davidtwco (Oct 16 2018 at 20:30, on Zulip):

I found those pretty hard to follow.

nikomatsakis (Oct 16 2018 at 20:30, on Zulip):

yes, that is the older style

nikomatsakis (Oct 16 2018 at 20:30, on Zulip):

I agree it is often less good

nikomatsakis (Oct 16 2018 at 20:31, on Zulip):

some of the new-style errors are also pretty hard to follow though :)

nikomatsakis (Oct 16 2018 at 20:31, on Zulip):

that said, it occured to me now that a "footnote" approach might be interesting

davidtwco (Oct 16 2018 at 20:32, on Zulip):

It's true. I think it's mostly because I ended up scanning up and down the error trying to work out what was referring to what because everything was so split apart. I prefer the new errors because everything is presented in a single snippet in source order and there's less working out what everything is.

davidtwco (Oct 16 2018 at 20:33, on Zulip):

I've got a better way of saying that on the tip of my tongue and it just isn't happening.

nikomatsakis (Oct 16 2018 at 20:33, on Zulip):
15 |     };
   |     ^^
   |     1|
   |      2
   |
   | 1:  `heap_ref` dropped here while still borrowed
   | 2: borrowed value needs to live until here
nikomatsakis (Oct 16 2018 at 20:33, on Zulip):

maybe something like that

nikomatsakis (Oct 16 2018 at 20:34, on Zulip):

hopefully using prettier "arrows"

nikomatsakis (Oct 16 2018 at 20:34, on Zulip):

this could also address the "telling a story" problem

nikomatsakis (Oct 16 2018 at 20:34, on Zulip):

since you could read 1, 2, 3 and then see the points in the code

davidtwco (Oct 16 2018 at 20:34, on Zulip):

That could be useful where long diagnostic messages are causing the pointers to things to get particularly long and obscuring what is happening.

nikomatsakis (Oct 16 2018 at 20:34, on Zulip):

right

nikomatsakis (Oct 16 2018 at 20:35, on Zulip):

and/or there are overlapping things

nikomatsakis (Oct 16 2018 at 20:35, on Zulip):

I'd like to present the underlines on separate lines

davidtwco (Oct 16 2018 at 20:35, on Zulip):

In other situations though, I think labelling directly makes more sense.

nikomatsakis (Oct 16 2018 at 20:35, on Zulip):

e.g.,

  | foo(bar)
  | -------- (1)
  |     --- (2)
  |
  | (1) blah blah blah
  | (2) blah blah blah
nikomatsakis (Oct 16 2018 at 20:35, on Zulip):

In other situations though, I think labelling directly makes more sense.

yes, my hope was that the user would just say "this label here" and the code would pick an optimal organization

nikomatsakis (Oct 16 2018 at 20:36, on Zulip):

basically falling back to footnotes if the ordering constraints can't be met and/or spans overlap or get too close

davidtwco (Oct 16 2018 at 20:36, on Zulip):

If you did it as a "this is getting hard to read, reformat as references" then it could even be automatic by the diagnostic builder when it detects that it would be better.

nikomatsakis (Oct 16 2018 at 20:36, on Zulip):

right

nikomatsakis (Oct 16 2018 at 20:37, on Zulip):

I didn't expet to change any of the diagnostic reporting code, except maybe to indicate where ordering is needed

nikomatsakis (Oct 16 2018 at 20:37, on Zulip):
15 |     };
   |     - (1)
   |      - (2)
   |
   | 1:  `heap_ref` dropped here while still borrowed
   | 2: borrowed value needs to live until here
davidtwco (Oct 16 2018 at 20:37, on Zulip):

Would it depend on the order that labels are applied so they are numbered correctly or by source order?

nikomatsakis (Oct 16 2018 at 20:37, on Zulip):

in that alternate style, which I kind of like now, it might look like that

nikomatsakis (Oct 16 2018 at 20:37, on Zulip):

Would it depend on the order that labels are applied so they are numbered correctly or by source order?

I would say that when you do span_label, you should also have span_ordered_label

nikomatsakis (Oct 16 2018 at 20:37, on Zulip):

all calls to span_ordered_label are meant to be read in that order

nikomatsakis (Oct 16 2018 at 20:38, on Zulip):

often, that 'just works' in the source

nikomatsakis (Oct 16 2018 at 20:38, on Zulip):

because earlier things come earlier

nikomatsakis (Oct 16 2018 at 20:38, on Zulip):

but around loops and method calls that's not always true

davidtwco (Oct 16 2018 at 20:38, on Zulip):

Overall, I really like the idea.

nikomatsakis (Oct 16 2018 at 20:38, on Zulip):

@Esteban Küber you around? =)

nikomatsakis (Oct 16 2018 at 20:38, on Zulip):

the footnotes was the missing piece for me

nikomatsakis (Oct 16 2018 at 20:38, on Zulip):

I hadn't thought of that before

nikomatsakis (Oct 16 2018 at 20:39, on Zulip):

I guess it would still be hard to handle multple multi-line spans

nikomatsakis (Oct 16 2018 at 20:39, on Zulip):

but those are a mess anyway

nikomatsakis (Oct 16 2018 at 20:39, on Zulip):

I guess we can "do our best"

nikomatsakis (Oct 16 2018 at 20:39, on Zulip):

and actually even those could maybe be handled with this idea of "multiple underlines per line"

davidtwco (Oct 16 2018 at 20:40, on Zulip):

Could annotate two places with the same reference number?

nikomatsakis (Oct 16 2018 at 20:40, on Zulip):

I guess for multispans

nikomatsakis (Oct 16 2018 at 20:45, on Zulip):

I'm excited about this idea :)

memoryruins (Oct 16 2018 at 20:46, on Zulip):

I'm a fan :)

memoryruins (Oct 16 2018 at 20:46, on Zulip):

worth hand converting some more, will highlight a bit more how it might (or places where it might not) work

memoryruins (Oct 16 2018 at 20:46, on Zulip):

heading out for now, have a good one everyone :wave:

nikomatsakis (Oct 16 2018 at 20:49, on Zulip):

wrote up a gist: https://gist.github.com/nikomatsakis/80866d4407f1cc38fba0c2da7175dcf7

nikomatsakis (Oct 16 2018 at 20:49, on Zulip):

does that sound about right?

nikomatsakis (Oct 16 2018 at 20:49, on Zulip):

I was going to open up an internals thread or a rust issue or something

Esteban Küber (Oct 16 2018 at 21:10, on Zulip):

I'm intrigued if not entirely convinced, but I see that it _could_ work

Esteban Küber (Oct 16 2018 at 21:11, on Zulip):

I would keep the current behavior around span positioning though...

15 |     };
   |     -- (2)
   |     |
   |     (1)
   (1): `heap_ref` dropped here while still borrowed
   (2): borrowed value needs to live until here
Esteban Küber (Oct 16 2018 at 21:11, on Zulip):

@nikomatsakis

nikomatsakis (Oct 16 2018 at 21:11, on Zulip):

I personally find that less clear

pnkfelix (Oct 16 2018 at 21:12, on Zulip):

@Esteban Küber but doesn't that run into the same problem of not being able to readily distinguish separate the } from the ; ?

Esteban Küber (Oct 16 2018 at 21:13, on Zulip):

that's fair...
Sometime back I considered having a different char to represent multiple overlapping spans, something like

  | foo(bar)
  | ----===- (1)
  |     |
  |     (2)
  |
  | (1) blah blah blah
  | (2) blah blah blah
pnkfelix (Oct 16 2018 at 21:13, on Zulip):

we already do that, don't we? the problem here is that the spans are adjacent, rather than overlapping

pnkfelix (Oct 16 2018 at 21:13, on Zulip):

Maybe the overlapping logic should be applied also to adjacent spans?

Esteban Küber (Oct 16 2018 at 21:13, on Zulip):

I discarded it because it was hard to find an ascii safe char to represent ^ and - overlapping

nikomatsakis (Oct 16 2018 at 21:14, on Zulip):

it also seems like there could be >2 labels

pnkfelix (Oct 16 2018 at 21:14, on Zulip):

(am I just wrong about overlapping labels? Let me go find an example....)

Esteban Küber (Oct 16 2018 at 21:15, on Zulip):

We don't. A long time ago I took a stab at using unicode chars to try something like that https://github.com/rust-lang/rust/pull/37703

nikomatsakis (Oct 16 2018 at 21:15, on Zulip):

@Esteban Küber another option — in this particular case at least — would be to print the start character in some way

nikomatsakis (Oct 16 2018 at 21:15, on Zulip):

e.g. with an up-arrow ▲ or something

nikomatsakis (Oct 16 2018 at 21:15, on Zulip):

ok, not so heavy

nikomatsakis (Oct 16 2018 at 21:15, on Zulip):

this of course gets into unicode

Esteban Küber (Oct 16 2018 at 21:15, on Zulip):

using and makes it clear where every span starts and end :)

Esteban Küber (Oct 16 2018 at 21:16, on Zulip):

Yep... and as you can see in that original PR, it needs a lot of work before we have something nice

pnkfelix (Oct 16 2018 at 21:17, on Zulip):

wait, so what is this then:

10 |         if let Some(entry) = heap_ref.peek_mut() {
   |                              ^^^^^^^^-----------
   |                              |
   |                              borrowed value does not live long enough
   |                              a temporary with access to the borrow is created here ...
nikomatsakis (Oct 16 2018 at 21:17, on Zulip):

using and makes it clear where every span starts and end :)

well, even then you can have two instances of the same span

Esteban Küber (Oct 16 2018 at 21:17, on Zulip):

regardless, we can get around the problem by having a starting and ending delimiter for spans
something along the lines of

  | foo(bar)
  | \--/\---/ (1)
  |     |
  |     (2)
  |
  | (1) blah blah blah
  | (2) blah blah blah
pnkfelix (Oct 16 2018 at 21:17, on Zulip):

(i.e. that is the case I was thinking of with overlapping spans)

Esteban Küber (Oct 16 2018 at 21:18, on Zulip):

@pnkfelix that's only for when 1 primary spans overlaps 1 or more secondary spans
we handle that overlap by making sure we always show the shortest

nikomatsakis (Oct 16 2018 at 21:18, on Zulip):
10 |         if let Some(entry) = heap_ref.peek_mut() {
   |                              ^^^^^^^^ (1)
   |                              ------------------- (2)
   |
   | (1) borrowed value does not live long enough
   | (2) a temporary with access to the borrow is created here ...
pnkfelix (Oct 16 2018 at 21:19, on Zulip):

ah okay I forgot (slash never learned properly) about the primary/secondary distinction

Esteban Küber (Oct 16 2018 at 21:19, on Zulip):

@nikomatsakis the reason I am a bit doubtful is because we can have potentially lots of very small overlapping spans that can make it hard to align vertically beyond the first span underline

Esteban Küber (Oct 16 2018 at 21:20, on Zulip):

granted, this might not be a problem in practice

Esteban Küber (Oct 16 2018 at 21:20, on Zulip):

Another option is to use coloring for span differentiation...

nikomatsakis (Oct 16 2018 at 21:20, on Zulip):

I don't quite understand the concern

nikomatsakis (Oct 16 2018 at 21:21, on Zulip):

(but I don't doubt there are painful edge cases...)

Esteban Küber (Oct 16 2018 at 21:21, on Zulip):
10 |         if let Some(entry) = heap_ref.peek_mut() {
   |                              ^^^^^^^^ (1)
   |                              ------------------- (2)
   |                                               - (3)
   |
   | (1) borrowed value does not live long enough
   | (2) a temporary with access to the borrow is created here ...
   | (3) where does this span point to?
nikomatsakis (Oct 16 2018 at 21:22, on Zulip):

this seems ok to me

nikomatsakis (Oct 16 2018 at 21:22, on Zulip):

I guess you mean that it's hard to visually lift your eye up a few lines

nikomatsakis (Oct 16 2018 at 21:22, on Zulip):

that's true, though it could be addressed by collapsing when possible

nikomatsakis (Oct 16 2018 at 21:22, on Zulip):

it also seems plausible to figure it out (e.g., in emacs :P you just press up arrow a few times)

nikomatsakis (Oct 16 2018 at 21:23, on Zulip):
 5:21 PM

10 |         if let Some(entry) = heap_ref.peek_mut() {
   |                              ^^^^^^^^ (1)     - (3)
   |                              ------------------- (2)
   |
   | (1) borrowed value does not live long enough
   | (2) a temporary with access to the borrow is created here ...
   | (3) where does this span point to?
nikomatsakis (Oct 16 2018 at 21:23, on Zulip):

that is what I meant by "collapsing when possible" (basically whenever non-overlapping)

Esteban Küber (Oct 16 2018 at 21:23, on Zulip):

I'm trying to keep in mind the minimum common case + the worst possible case for our diagnostics ^_^

Esteban Küber (Oct 16 2018 at 21:23, on Zulip):

Overlapping that way is definitely something we'd want to do

nikomatsakis (Oct 16 2018 at 21:23, on Zulip):

I guess I think that this sort of case (where you have overlapping spans) is pretty confusing in the current mode

nikomatsakis (Oct 16 2018 at 21:24, on Zulip):

so even if something like footnotes is not perfect, it's potentially a step up

nikomatsakis (Oct 16 2018 at 21:24, on Zulip):

not to say I would always want to do it

Esteban Küber (Oct 16 2018 at 21:24, on Zulip):

I guess the best thing we could do is actually implement the change and inspect the ui diffs :)

nikomatsakis (Oct 16 2018 at 21:24, on Zulip):

confirm

nikomatsakis (Oct 16 2018 at 21:24, on Zulip):

(should I open an issue or internals thread or something?)

nikomatsakis (Oct 16 2018 at 21:25, on Zulip):

just to keep from losing the idea...

Esteban Küber (Oct 16 2018 at 21:25, on Zulip):

Yes, although I think this is an idea that's been floating for a while, it'd be nice to have an actual place with it

Esteban Küber (Oct 16 2018 at 21:26, on Zulip):

Adding to https://github.com/rust-lang/rust/issues/51528 would also be nice

Esteban Küber (Oct 16 2018 at 21:26, on Zulip):

Although I feel some of the things listed there are higher priority

Esteban Küber (Oct 16 2018 at 21:28, on Zulip):

Like being able to specify span labels in a subdiagnostic. That would make a nice difference.

nikomatsakis (Oct 16 2018 at 21:29, on Zulip):

I would like to break out the formatting code into its own crate

nikomatsakis (Oct 16 2018 at 21:29, on Zulip):

that is, something that lives on crates.io

nikomatsakis (Oct 16 2018 at 21:29, on Zulip):

I wonder how plausible that is

nikomatsakis (Oct 16 2018 at 21:29, on Zulip):

it'd take some careful work to "detach" it nicely from the compiler

nikomatsakis (Oct 16 2018 at 21:30, on Zulip):

but I feel like we could iterate a lot faster this way

nikomatsakis (Oct 16 2018 at 21:30, on Zulip):

/me all about detaching from the compiler these days

Esteban Küber (Oct 16 2018 at 21:30, on Zulip):

@nikomatsakis possibly. I can take a look at it, maybe @Zack M. Davis might be interested as well

nikomatsakis (Oct 16 2018 at 21:30, on Zulip):

the idea would be to make it generic over the span type and the codemap type etc

nikomatsakis (Oct 16 2018 at 21:31, on Zulip):

anyway

Esteban Küber (Oct 16 2018 at 21:32, on Zulip):

But in order to move it out we'd need to stabilize the API surface, right? There are a couple of things I'd like to add before that, and remove (like span_suggestion without Applicability)

nikomatsakis (Oct 16 2018 at 21:32, on Zulip):

Oh, I see that @csmoe is way ahead of me

nikomatsakis (Oct 16 2018 at 21:32, on Zulip):

But in order to move it out we'd need to stabilize the API surface, right? There are a couple of things I'd like to add before that, and remove (like span_suggestion without Applicability)

I mean it can be at a 0.x version for a while that changes its API surface frequently

nikomatsakis (Oct 16 2018 at 21:33, on Zulip):

but yes, part of the benefit is trying to stabiize and convert on somethign nice

Esteban Küber (Oct 16 2018 at 21:36, on Zulip):

I can see how we can enable the footnote output only if certain heuristics are triggered. I'm coming around to the idea...

Last update: Nov 22 2019 at 00:40UTC