Make deleted code in a suggestion clearer

[estebank]

Show suggestions that include deletions in a way similar to diff's output.

For changes that do not have deletions, use + as an underline for additions and ~ as an underline for replacements.

For multiline suggestions, we now use ~ in the gutter to signal replacements and + to signal whole line replacements/additions.

In all cases we now use color to highlight the specific spans and snippets.

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[Nicholas-Baron]

I personally view the narrow end of an arrow as a destination to insert some items.
Thus, the bottom line of that error message would something look like

fn foo(&self, _ : &impl Debug) { }
     \/

However, I do get the choice of /\ from an aesthetic perspective (e.g. looks like the ^).
Another idea may be using red for the /\ to signify deletion, which seems to be a common cross-platform idiom.

[iago-lito]

Just thinking.. what about making it clearer with underline+overline, to avoid @Nicholas-Baron "insert location" interpretation?

      \/             (* snip! *)
fn next(&mut self)
      /\

[jieyouxu]

Adding to the bikeshed...

help: try removing the generic parameter and using `impl Trait` instead
  |
8 |         fn foo(&self, _: &impl Debug) { }
  |              --           ++++++++++

Red for deletions -, green for additions +, perhaps like diffs? This has the advantage of showing clearly what's added and should be easier to compute the width for, though the - symbol is already to highlight.

Maybe use x to signal deletions?

Maybe even highlight what the user should try to add?

[bjorn3]

Maybe even highlight what the user should try to add?

The code used to be the same color as the highlights below it, but was changed to be the default color at some point.

[camsteffen]

Another idea:

[bjorn3]

That doesn't work without colors. Think about a color blind person or copy pasted error messages.

[camsteffen]

I think it works without colors.

 help: try removing the generic parameter and using `impl Trait` instead
   |
 8 |         fn foo(&self, _: &impl Debug){ }
   |              ><          >++++++++++<

[bjorn3]

Oh, the > and < are outside of the inserted range instead of inside. That is somewhat non-obvious.

[estebank]

The other option is to keep the removed code and give it a red background or color, and a different underline, like was proposed above so that it still works with colorless output:

help: try removing the generic parameter and using `impl Trait` instead
  |
8 |         fn foo<U: Debug>(&self, _: &impl Debug) { }
  |               xxxxxxxxxx            ++++++++++

What we lose with that is the ability to just copy and paste the code from the terminal (although I doubt that's a common occurrence).

[camsteffen]

Oh, the > and < are outside of the inserted range instead of inside. That is somewhat non-obvious.

Yeah. On second thought I don't think I would use >+< for inserted code.

I think that >< may be more intuitive than /\ for deleted code. Neither seems clearly better to me.

keep the removed code

I don't like that option personally. I would like the output to show exactly what the code looks like after applying the suggestion. I can compare with the current version of the code in the first error message.

[tlyu]

I think /\ looks too much like a proofreader's mark for insertion, so using it to signify deletion seems like a bad idea. Also, I'm not sure it always renders as a legible arrow or caret type appearance in all likely terminal fonts.

I kind of like >< because it looks like it's pointing to the empty space where the deleted text used to be.

There seems to be a suggestion for electronic texts to use marks including % to signify deletion, because of the visual similarity to the traditional handwritten proofreader's mark. For our case, that would have to go inline with the old text that we're suggesting deleting, which is probably not something we want.

Another possibility is to use inline comments like /* deleted */ to signify the deletion, or even just surround the deleted text with an inline comment. Drawbacks include less familiarity with this style of comment (it doesn't seem to be the prevailing style, and rarely appears in tutorial material) and repetition of the deleted text (if we use the "comment out the deleted text" approach).

[estebank]

I would like the output to show exactly what the code looks like after applying the suggestion. I can compare with the current version of the code in the first error message.

That's been my personal take, but I've encountered enough people that have gotten confused by it that it's making me reconsider my stance for the projects sake.

---

The problem I see with using + for the underline is that if we are replacing text it doesn't convey the right message, as we are not informing the user that the replacement occurred. That being said:

---

Edit: implemented one of the proposals keeping deletions around (which is buggy, but gives an idea of what it'd look like):

[camsteffen]

The problem I see with using + for the underline is that if we are replacing text it doesn't convey the right message, as we are not informing the user that the replacement occurred.

I'm convinced on that. It looks like diff output but the semantics are different. That's confusing.

[Artoria2e5]

While we’re at it with the red mark, maybe a strikethrough can be tried too? The \e[9 stuff, not the Unicode hack.

[estebank]

While we’re at it with the red mark, maybe a strikethrough can be tried too? The \e[9 stuff, not the Unicode hack.

We use termcolor which doesn't have support to specify strikethrough.

Using unicode, I have this mockup. Please ignore the obvious bugs with color positions:

I don't know if the strikethrough will help that much.

[tlyu]

While we’re at it with the red mark, maybe a strikethrough can be tried too? The \e[9 stuff, not the Unicode hack.

I think that ANSI terminal strikethrough capability isn't widely supported? Also, I think it's important that any annotations survive copying and pasting, especially because we often ask people to copy and paste full error messages when they ask about compiler errors.

[tlyu]

The problem I see with using + for the underline is that if we are replacing text it doesn't convey the right message, as we are not informing the user that the replacement occurred.

I'm convinced on that. It looks like diff output but the semantics are different. That's confusing.

I agree. Maybe ~ as an underline for changes? (It's similar to the proofreader's mark for transposition, which is a bit of a stretch but close enough, I think.) [edit: also, I think the existing ^ underline for insertions/changes is probably good enough, and it doesn't have the association with diff that + would]

[tlyu]

I would like the output to show exactly what the code looks like after applying the suggestion. I can compare with the current version of the code in the first error message.

That's been my personal take, but I've encountered enough people that have gotten confused by it that it's making me reconsider my stance for the projects sake.

I think if we're going to leave the deleted text inline, I'm leaning toward surrounding it with a /* */ comment.

[estebank]

Three other options. I'm partial to the second one, but the first one looks easier to understand what's going on.

[estebank]

Current output for this PR:

[JohnTitor]

Three other options. I'm partial to the second one, but the first one looks easier to understand what's going on.

I'm worried that how the first looks like with --color=never. The second and the current look nice to me.

[jieyouxu]

Current output for this PR:

I also like this current output. I would like to summarize some UX considerations presented in the
discussions prior that I believe contributes to good suggestion UX:

• The replacement suggestion should be valid Rust code (or at least a step towards that
  direction) which guides the user towards a proper solution. This is the case with not leaving the
  removed spans behind in the suggestion.
• The deletion sigil used (~) corresponds well to spell check errors (squiggly/wave-y underlines)
  commonly used by word processors, editors and browsers to hint that a "suggested replacement" is
  available; this can perhaps be more newcomer-friendly due to familiarity. I also think that
  - might collide with its use to indicate location; can be confusing especially without colors. I
  also agree that + can be ambiguous cf. diffs.

• Different sigils are used (~ for deletion, ^ for addition) that are able to clearly
  distinguish between additions and deletions without relying on colors (friendly for those who
  are colorblind). For example:

help:  try removing the generic parameter and using `impl Trait` instead
  |
8 |         fn foo  (&self, _: &impl Debug) { }
  |               ~~            ^^^^^^^^^^

• Simple ASCII characters used without exotic whitespace or other symbols; easy for user to type
  out and copy, wide range of terminal/editor/IDE support. The additional advantage of having
  simpler underlines such as ~~~ and ^^^ is that it should be easier to compute their respective
  spans.
• The selected sigils also works next to each other reasonably well (contrived example...
  since currently presented mockups don't have addition/deletion spans next to each other):

help:  an interesting error message
  |
2 |     let x: u64 = 0;
  |             ~^

(On a side note, how would the addition/deletion underlines be handled if their spans overlap or
are neighbouring?)

[bjorn3]

help:  an interesting error message
  |
2 |     let x: u64 = 0;
  |             ~^

Gcc uses the exact same squiggles for a different purpose:

<source>: In function 'int square(int)':
<source>:3:18: error: 'foo' was not declared in this scope
    3 |     return num * foo;
      |                  ^~~

or

<source>: In function 'int square(int)':
<source>:5:16: error: no match for 'operator*' (operand types are 'int' and 'Foo')
    5 |     return num * Foo{};
      |            ~~~ ^ ~~~~~
      |            |     |
      |            int   Foo

For reference this is what gcc uses for insertions:

<source>:5:16: error: stray '@' in program
    5 |     return num @ Foo{};
      |                ^
<source>: In function 'int square(int)':
<source>:5:15: error: expected ';' before 'Foo'
    5 |     return num @ Foo{};
      |               ^  ~~~
      |               ;

and clang:

<source>:5:15: error: expected ';' after return statement
    return num @ Foo{};
              ^
              ;
1 error generated.

[iago-lito]

I agree with the following principles to stick to:

• A. Replacement suggestion is valid rust code
  • A.1 If possible well-formatted.
• B Deletion and insertion can be read without colors, colors is only a bonus.
• C No fancy unicode chars or terminal strikethrough.
• D Attach neat semantics to the chars used to delimitate deleted/added/changed regions
  • D.1 Yet don't conflict with usual diff semantics.

Then what about :

With red >< for deletion, blue ~ for edition and green ^ for addition.

The only problem I see is possible overlap, in which case I would only derogate to A.1 and fix the overlap with additional whitespace, e.g.:

[flip1995]

@estebank blessing the Clippy tests with ./x.py test src/tools/clippy --bless worked for me. Using --stage 2 shouldn't be necessary for this, but that shouldn't be the problem here...

Anyway, here's a gist of the Clippy patch

[estebank]

@flip1995 thanks for checking! Somehow I changed branches some time back without realizing and that's why --stage 2 --bless wasn't working for me 🤦‍♂️

@bors r=petrochenkov rollup=never p=1

[bors]

📌 Commit 657caa5 has been approved by petrochenkov

[bors]

⌛ Testing commit 657caa5 with merge d8036c085d6934bf52caa7b9ab656f95a31c1616...

[bors]

💥 Test timed out

[estebank]

@bors retry

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[bors]

⌛ Testing commit 657caa5 with merge ccffcaf...

[bors]

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing ccffcaf to master...