Newlines make diagnostic fix suggestions render strangely

[compiler-errors]

Given the following code: playground

fn foo() -> bool {
    &
    if true { true } else { false }
}

The current output is:

error[E0308]: mismatched types
 --> src/lib.rs:2:5
  |
1 |   fn foo() -> bool {
  |               ---- expected `bool` because of return type
2 | /     &
3 | |     if true { true } else { false }
  | |___________________________________^ expected `bool`, found `&bool`
  |
help: consider removing the borrow
  |
2 -     &
2 +     if true { true } else { false }

What's wrong

The suggestion looks weird, seeming to suggest removing & and replacing it with a duplicate of the line that follows it.

The actual fixup'd code (via cargo fix) isn't actually broken. cargo fix does the change as expected, removing just the &. So this leads me to believe this is instead just a bug in the code that renders suggestions.

@camelid spotted this in #91545 but this is not related to that PR.

[camelid]

cc @estebank -- I think you wrote the code that generates these diffs?

[estebank]

I did. I guess what we could do is detect this case in particular and show only the 2 - & part. Alternatively, that + should instead be ~.

[camelid]

@rustbot claim

Perhaps I'll try this out :)

Which part of the code would need to be changed to only show the - & line?

[compiler-errors]

@camelid I bet the code is somewhere around here https://github.com/rust-lang/rust/blob/master/compiler/rustc_errors/src/emitter.rs#L1671

haven't looked much into the rendering, but a theory: is_multiline might be wrong for removal suggestions that end in \n.

Otherwise maybe we just need to suppress emitting + on the line following any time we render a removal suggestion ending in in \n.

[camelid]

@compiler-errors Thanks for the suggestion! I tried trimming newlines before calling is_multiline, which didn't seem to help, so I think it may be elsewhere.

Some results from investigating:

The span for the substitution with "" is 2:5 (line 2, col. 5) through 3:5. So the source range doesn't actually end in a newline; it ends in spaces for indentation on the next line. Then I think line 3 is computed as the line to display as being added (which makes sense given the output), though I'm still not sure why.

[CastilloDel]

@rustbot claim

[CastilloDel]

Hey! I took a look at this and I have found some things, but I don't see a clear path to fix it.

@camelid was right that the span seems to be wrong. It takes the line with the ampersand, but also the indentation of the next line. The code that produces that span is in rustc_typeck.

After experimenting a bit, I understood why the span is like that. It tries to remove the part of a &T expression that comes before a T expression. We can't just remove one character (the &), because the borrow could also be mutable (&mut). So I understand this is working as expected and shouldn't be touched, right?

The problem is, that even using that span, the suggestion we are getting is wrong. I would expect something like this:

  |
2 -     &
2 +    if true { true } else { false }
3 -    if true { true } else { false }

Maybe that isn't ideal, but it makes sense (at least to me), as the new line takes the whitespace from the first one. However, this doesn't happen and I'm having difficulties finding where the error is in the code. I have found that in this line complete takes the value " if true { true } else { false }", but I'm not sure if this is the correct value or not. Not really sure about what complete is supposed to be.

So, to sum up:

• Should I "fix" the span or the display of that span?
• What is complete supposed to hold?

[estebank]

I think that fixing the rendering of the span is the appropriate solution. We could infer for example that the only changes that are additions (rendered with a + next to the line number) are purely whitespace, and then not render it at all. I think that would convey the "delete this line" meaning well enough.

[CastilloDel]

I'm not sure I understand. The addition that appears in the diagnostic isn't only whitespace. If we remove the whole span, the line 2 is another almost entirely different line (in fact, the only thing that doesn't change from the compiler point of view is precisely the whitespace).

I think the root of the problem is that right now the compiler has trouble when rendering removals that span more than one line. The problem is even worse if there are more lines involved:

error[E0308]: mismatched types
 --> test:2:5
  |
1 |   fn foo() -> bool {
  |               ---- expected `bool` because of return type
2 | /     & 
3 | |     mut
4 | |     if true { true } else { false }
  | |___________________________________^ expected `bool`, found `&mut bool`
  |
help: consider removing the borrow
  |
2 -     & 
2 +     if true { true } else { false }
  |

I don't think this can be fixed without restructuring at least part of the rendering, but I've found the code to be really difficult to understand (I guess there are lots of edge cases for things like this).

[estebank]

@CastilloDel, the patch rendering of the suggested change would have been

-     & 
-     mut
-     if true { true } else { false }
+     if true { true } else { false }

because the span that is being removed (and hence the lines that are being modified) is

2 |       & 
  |  _____^
3 | |     mut
4 | |     if true { true } else { false }
  | |____^

which puts the start of the if true... directly after the whitespace that comes before the &, instead of removing the lines with the & and the mut (which is what a human would do). The span is correct. Calculating that you can get the same effect by removing lines 2 and 3 when preparing the suggestion is hard (and too bespoke, the same logic would have to be written in multiple places). Detecting cases like these when rendering the suggestion will work for every suggestion and likely be easier to implement.

Does this make sense?

[CastilloDel]

Yes! That makes sense.

The problem is that what is actually happening in the code is that it is inferring that it should substitute the & line with if true { true } else { false }, without showing that the next line should be removed. And with the way that the rendering code is structured, I'm having trouble seeing how to fix that. I'll keep looking. Thanks!