Span off-by-one error in diagnostics

[estebank]

When two diagnostic spans are of the same width/location (effectively the same) and only one of them has a label, the output is the same as for a single one. There's a corner case where having a zero-width span (which get's rendered as a 1-char-wide span) fall in the same place as a 1-char-wide span without a label, resulting in the following output:

{foo(bar {}}
    -      ^
    |      |
    |      help: `)` may belong here
    |
    unclosed delimiter

This should instead be

{foo(bar {}}
    -      ^ help: `)` may belong here
    |
    unclosed delimiter

[relchar]

Happy to take this on if no one else has. It'll be my first time contributing to Rust.

[estebank]

@relchar that'd be awesome!

The actual case that made the bug evident is being introduced in #57944 (if you look at https://github.com/rust-lang/rust/pull/57944/files#diff-beb01736b1c52103f2dcd2eac341b810R4).

The following is the code that turns small suggestions into an inline label:

rust/src/librustc_errors/emitter.rs

Lines 37 to 63 in d173180

	if let Some((sugg, rest)) = db.suggestions.split_first() {
	if rest.is_empty() &&
	// don't display multi-suggestions as labels
	sugg.substitutions.len() == 1 &&
	// don't display multipart suggestions as labels
	sugg.substitutions[0].parts.len() == 1 &&
	// don't display long messages as labels
	sugg.msg.split_whitespace().count() < 10 &&
	// don't display multiline suggestions as labels
	!sugg.substitutions[0].parts[0].snippet.contains('\n') {
	let substitution = &sugg.substitutions[0].parts[0].snippet.trim();
	let msg = if substitution.len() == 0 || !sugg.show_code_when_inline {
	// This substitution is only removal or we explicitly don't want to show the
	// code inline, don't show it
	format!("help: {}", sugg.msg)
	} else {
	format!("help: {}: `{}`", sugg.msg, substitution)
	};
	primary_span.push_span_label(sugg.substitutions[0].parts[0].span, msg);
	} else {
	// if there are multiple suggestions, print them all in full
	// to be consistent. We could try to figure out if we can
	// make one (or the first one) inline, but that would give
	// undue importance to a semi-random suggestion
	suggestions = &db.suggestions;
	}
	}

the following is the code where we account for zero-width spans

rust/src/librustc_errors/emitter.rs

Lines 220 to 227 in d173180

	// Watch out for "empty spans". If we get a span like 6..6, we
	// want to just display a `^` at 6, so convert that to
	// 6..7. This is degenerate input, but it's best to degrade
	// gracefully -- and the parser likes to supply a span like
	// that for EOF, in particular.
	if lo.col_display == hi.col_display && lo.line == hi.line {
	hi.col_display += 1;
	}

and finally here is the logic for displaying spans, which is a bit verbose, but at least it has some comments that explain what is supposed to be going on (this is where I would expect the fix to consider zero-width spans as a 1 char wide span to be made)

rust/src/librustc_errors/emitter.rs

Lines 400 to 521 in d173180

	// First, figure out where each label will be positioned.
	//
	// In the case where you have the following annotations:
	//
	// vec.push(vec.pop().unwrap());
	// -------- - previous borrow ends here [C]
	// ||
	// |this makes no sense [B]
	// previous borrow of `vec` occurs here [A]
	//
	// `annotations_position` will hold [(2, A), (1, B), (0, C)].
	//
	// We try, when possible, to stick the rightmost annotation at the end
	// of the highlight line:
	//
	// vec.push(vec.pop().unwrap());
	// --- --- - previous borrow ends here
	//
	// But sometimes that's not possible because one of the other
	// annotations overlaps it. For example, from the test
	// `span_overlap_label`, we have the following annotations
	// (written on distinct lines for clarity):
	//
	// fn foo(x: u32) {
	// --------------
	// -
	//
	// In this case, we can't stick the rightmost-most label on
	// the highlight line, or we would get:
	//
	// fn foo(x: u32) {
	// -------- x_span
	// |
	// fn_span
	//
	// which is totally weird. Instead we want:
	//
	// fn foo(x: u32) {
	// --------------
	// | |
	// | x_span
	// fn_span
	//
	// which is...less weird, at least. In fact, in general, if
	// the rightmost span overlaps with any other span, we should
	// use the "hang below" version, so we can at least make it
	// clear where the span *starts*. There's an exception for this
	// logic, when the labels do not have a message:
	//
	// fn foo(x: u32) {
	// --------------
	// |
	// x_span
	//
	// instead of:
	//
	// fn foo(x: u32) {
	// --------------
	// | |
	// | x_span
	// <EMPTY LINE>
	//
	let mut annotations_position = vec![];
	let mut line_len = 0;
	let mut p = 0;
	for (i, annotation) in annotations.iter().enumerate() {
	for (j, next) in annotations.iter().enumerate() {
	if overlaps(next, annotation, 0) // This label overlaps with another one and both
	&& annotation.has_label() // take space (they have text and are not
	&& j > i // multiline lines).
	&& p == 0 // We're currently on the first line, move the label one line down
	{
	// This annotation needs a new line in the output.
	p += 1;
	break;
	}
	}
	annotations_position.push((p, annotation));
	for (j, next) in annotations.iter().enumerate() {
	if j > i {
	let l = if let Some(ref label) = next.label {
	label.len() + 2
	} else {
	0
	};
	if (overlaps(next, annotation, l) // Do not allow two labels to be in the same
	// line if they overlap including padding, to
	// avoid situations like:
	//
	// fn foo(x: u32) {
	// -------^------
	// | |
	// fn_spanx_span
	//
	&& annotation.has_label() // Both labels must have some text, otherwise
	&& next.has_label()) // they are not overlapping.
	// Do not add a new line if this annotation
	// or the next are vertical line placeholders.
	|| (annotation.takes_space() // If either this or the next annotation is
	&& next.has_label()) // multiline start/end, move it to a new line
	|| (annotation.has_label() // so as not to overlap the orizontal lines.
	&& next.takes_space())
	|| (annotation.takes_space() && next.takes_space())
	|| (overlaps(next, annotation, l)
	&& next.end_col <= annotation.end_col
	&& next.has_label()
	&& p == 0) // Avoid #42595.
	{
	// This annotation needs a new line in the output.
	p += 1;
	break;
	}
	}
	}
	if line_len < p {
	line_len = p;
	}
	}
	if line_len != 0 {
	line_len += 1;
	}

Feel free to reach out to anyone in the team either here or at https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fdiagnostics

Please take a look at The Forge, which has a lot of relevant information on how to work with the rustc codebase, particularly how to run tests (usually, ./x.py test src/test/ui --stage 1 --bless).

[gs0510]

@relchar Are you working on this? If not can I work on this issue? Thanks!

[relchar]

Hey Gargi, feel free to take it. I haven’t been able to take a look yet.

…

On Feb 26, 2019, at 4:15 PM, Gargi Sharma ***@***.***> wrote: @relchar Are you working on this? If not can I work on this issue? Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[saleemjaffer]

@gs0510 Are you working on this? Please let me know if I can work on this.

[gs0510]

Hi @saleemjaffer, yes I'm working on it. :) I will have a PR open by EOD today.

[xldenis]

@gs0510 have you had a chance to work on this? Or would you like me to take over?

[gs0510]

hey @xldenis, please feel free to take over. Thanks!

[xldenis]

@estebank is there any reason to ever keep a zero width annotation with no label?

[estebank]

@xldenis zero width annotations are not always zero width conceptually. They mark a place in between two characters, and are often used for suggestions to mark a place where we'll add code or by diagnostics to point at a place in between two tokens. Sometimes having a label doesn't increase the understanding of the error. In the example shown above, we have a suggestion to add ) in between the two } characters and we also have the primary span pointing at the closing }. Does that make sense?

[xldenis]

yea it does, I need to do some more debugging and investigating but I think I'll be able to come up with a fix for this edge-case soon :)