Rust: 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:

https://github.com/rust-lang/rust/blob/d1731801163df1d3a8d4ddfa68adac2ec833ef7f/src/librustc_errors/emitter.rs#L37-L63

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

https://github.com/rust-lang/rust/blob/d1731801163df1d3a8d4ddfa68adac2ec833ef7f/src/librustc_errors/emitter.rs#L220-L227

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)

https://github.com/rust-lang/rust/blob/d1731801163df1d3a8d4ddfa68adac2ec833ef7f/src/librustc_errors/emitter.rs#L400-L521

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 [e-mail] 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 :)