Line/Column on Fancy is always 1:1

[khionu]

Unless I'm mistaken, it should infer the correct position from the label's SourceSpan?

[zkat]

This is a little confusing, yes. The number next to the name is actually the start of the displayed snippet, not the SourceSpan itself. I've gotten multiple questions about this and people seem to consistenly expect it to be the SourceSpan location, so maybe it's worth changing.

The one catch with that is... which SourceSpan? miette globs together nearby SourceSpans to come up with the displayed snippet. So which one do we pick? The first one we ran into? The first one that gets displayed? (these can be separate SourceSpans).

Anyway I'm happy to take a patch that changes what this is. It shouldn't be too hard :)

[khionu]

Thinking on it more now, I think it might be better to remove entirely. I know the option is there, but by default you already have the line number(s), and you have the specific span highlighted, eliminating the need for the column number. The Rust compiler doesn't display this specific information either.

[zkat]

let's throw it out, then :D

[rrbutani]

The Rust compiler doesn't display this specific information either.

Sorry, I'm not sure I follow; I think rustc does show the line/col for the span attached to errors/warnings:

---

This is maybe not a compelling reason to keep the line/col numbers around but: some tools (VS Code at the very least and an LSP adapter IIRC) scan command line output for these. VS Code's built-in terminal turns these into "links" that jump the editor to the file/line/pos when clicked (this is why a.rs:2:3 is underlined in the screenshot above).

Personally, this seems useful – even if the line/col reported are only in the vicinity of the actual problematic span.

[khionu]

Huh... I had tested some output, didn't have it. Maybe my tooling dropped it, as it did create the link as you mentioned. Regardless, my mistake.

That is a good point. I guess if we're looking from the perspective that this is important for tooling (imo, it should be through an API not text parsing, but that's beside the point), it should match.

Keeping it, I would do the following order of precedence:

• Explicitly marked via label("label", origin = true)
• Implicitly as the first label
• Implicitly as the only label

[zkat]

Hm. That seems like a good a plan as any tbh. I'm honestly ok with any way, but the VS Code linking is definitely a compelling argument to keep this.

[khionu]

This will require adding a trait method, I think? And it changes visible behaviour, so... I think this will require a minor version bump?

[khionu]

Working on a PR, is using the term "origin" good to refer to where the extension points to?

[zkat]

I don't think you need a new trait method, tbh. And yeah, a minor bump is probably the best idea.

"origin" seems fine idk. Like I said, it doesn't need to be public, but it does need to be reimplemented across the three reporters.

[khionu]

I was thinking having it be in the Diagnostic trait so it's less code duplication. Thoughts?

[zkat]

Hmm. I thought about this for a little while and dug through the source code and how things work and... I think this should be an addition to LabeledSpan, and instead of origin, it should add a method (probably just span_type()), that returns a variant of a SpanType enum that, for now, will have two variants: Primary and Secondary, with the default value being Primary. This will also open the door to eventually adding a third variant I've had in my mind, Suggestion, that will support the sorts of suggestions the Rust compiler can do. (the things with the ++++ signs under code).

This way, we don't have to modify the Diagnostic trait at all.

You'll also want to add some other methods for creating secondary LabeledSpans, such as new_secondary() and new_secondary_with_span().

What do you think of that plan?

[khionu]

I like the idea of making labels able to be varied in type, but that seems to be moving from the scope of this issue. It's my understanding that the issue is more about where and how to communicate the line/col index of the overall error, not how to identify a label as being the main highlight of the error. Additionally, having to iterate on the labels to find the primary seems excessive.

I am wondering what your thoughts are for not making this a trait method? It would be relatively trivial for the proc macro to have the impl "hardcode" the index of the label, and for manual implementations of the trait to track the point themselves.

[zkat]

In practice, at least for miette's reporters, you already have to iterate over all the Diagnostics for a Report in order to figure out which one should be the "primary" one, as part of reporting the diagnostic in general. For some reason, something seems off about just storing an index into an arbitrary iterable. But I guess that could work? Go for it, I guess :)

[khionu]

For some reason, something seems off about just storing an index into an arbitrary iterable.

Generally, I agree with this instinct, but the proc macro constructs the iterable, and so it knows where in the iterable it would be. Further more, people who manually impl the trait can do even better in optimising this, something they couldn't optimize at all if we stored it as a value on the label.

you already have to iterate

Yup, I've been learning lately how heavy error paths can be in general, but imo it's more incentive to save where we can

[erratic-pattern]

Perhaps in the case where all spans occur on the same line, it could instead display that line number. That seems to be what many people are expecting to happen in simpler/common case where all the labels occurs on a given line.

I do like the "primary" label idea as well, with an attribute macro or meta arg to specify which label is primary. But this could be confusing if the "primary" label looks exactly like all the others (why did it show this line number instead of the other one?)

Since you could argue that this is somewhat of an arbitrary choice to make with regards to displaying line numbers, maybe this could be on a reporter option so that users can opt in or out.

[zkat]

tbh, I've started thinking that, at least for now, a better option is just to display the line/col for the beginning of the first span being displayed in that snippet, since we're already sorting them. It might not be perfect, but it's better than 1:1.