compiletest: Improve diagnostics for line annotation mismatches

[petrochenkov]

When some line annotations are missing or misplaced, compiletest reports an error, but the error is not very convenient.
This PR attempts to improve the user experience.

• The "expected ... not found" messages are no longer duplicated.
• The proc_res.status and proc_res.cmdline message is no longer put in the middle of other messages describing the annotation mismatches, it's now put into the end.
• Compiletest now makes suggestions if there are fuzzy matches between expected and actually reported errors (e.g. the annotation is put on a wrong line).
• Missing diagnostic kinds are no longer produce an error eagerly, but instead treated as always mismatching kinds, so they can produce suggestions telling the right kind.

I'll post screenshots in the thread below, but the behavior shown on the screenshots can be reproduced locally using the new test tests/ui/compiletest-self-test/line-annotation-mismatches.rs.

This also fixes #140940.

r? @jieyouxu

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[petrochenkov]

Before: that's the only shown error when some annotation kind is missing.

[petrochenkov]

Before: without missing annotation kinds (some stuff at the top is cropped, but the picture is clear).

[petrochenkov]

After: with top and bottom messages merged, and suggestions added.

[jieyouxu]

Nice! I'll play around with this tmrw on Monday.

[petrochenkov]

@jieyouxu
By the way, do you think the line1:column1: line2:column2: part is useful in error messages?

I does two things

• it's displayed in annotation mismatch messages, but it mostly duplicates the file:line part which is also displayed.
  • the column part could be slightly useful in the past when we didn't have UI tests with captured .stderr, but probably not now
• it can actually be matched in line annotations, e.g. you can write //~ ERROR 28:16: cannot find value, but it seems to be an antipattern

Also, this part is not always added, it's only added to primary diagnostics, but not to labels or suggestions, for example.

I think maybe just drop it to reduce noise and duplication.
(The column can be moved to file:line:column instead of being fully dropped, if necessary.)

[petrochenkov]

Another possible improvement is to display relative paths to test files instead of absolute paths, also to avoid noise, duplication and rightward shift.

The main requirement for the paths is to be "clickable", so you can quickly go from a file:line string in shell to that file and line in editor.

Relative paths here would work for me (vscode + rust is the only directory open in the workspace), but I'm not sure what other setups people can use.

[jieyouxu]

Sorry for the wait, I played around with this locally, and I think it's an improvement. I have some feedback:

1. There's a starting message

   Blessing the stderr of in "/home/joe/repos/rust/tests/ui/compiletest-self-test/line-annotation-mismatches.stderr"

   where I think the actual source file path is missing. That might be pre-existing though.
   We could also maybe highlight the paths in yellow/magenta, but yeah.

2. I think we should only report the source file path once, /home/joe/repos/rust/tests/ui/compiletest-self-test/line-annotation-mismatches.rs, otherwise repeating that seems very noisy if we do it for every mismatch, and the rightwards drift is kinda annoying.

3. I don't find the line1:column1: line2:column2: useful in error messages themselves, especially not in error annotations like //~ ERROR 23:22 (unless someone wants to match on the exact column but...).

4. The column numbers might be useful if you want to jump in exactly, e.g. with neovim and whatever you can nvim path.rs:LINE:COL, or if there's > 1 diagnostics emitted on the same line? But for me, the line number is usually what matters. Agreed on stderr snapshots. I guess it might be useful if you don't check in a stderr snapshot (//@ dont-check-compiler-stderr)? I would be fine dropping it, and see if people actually have a specific use case where they want the exact column number too.

5. I'm fine with showing only a relative path, although if we go with (2), I don't think it would matter as much.

Misc design feedback

For the actual messages, I might even do sth like:

expected but with a different kind: tests/ui/compiletest-self-test/line-annotation-mismatches.rs:22:
          message: WARN: cannot find value `unresolved2` in this scope
    expected kind: ERROR
    reported kind: WARN

reported with a different message: tests/ui/compiletest-self-test/line-annotation-mismatches.rs:25:
    expected message: NOTE: not found in this scope
    reported message: 

If possible, we should report both the one we got and the one we expect (kind, message, line number).

Questions

• Also, how does this handle normalizations? E.g. //@ normalize-stderr (haven't played around with this much, I'll do that tmrw).

[jieyouxu]

Let me know what u think.
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[petrochenkov]

There's a starting message ... Blessing the stderr
That might be pre-existing though.

Yeah, that's from an entirely different part of compiletest, not from expected error checking.

I think we should only report the source file path once, /home/joe/repos/rust/tests/ui/compiletest-self-test/line-annotation-mismatches.rs

What do you suggest reporting instead?

• A relative path tests/ui/compiletest-self-test/line-annotation-mismatches.rs (compiletest: Improve diagnostics for line annotation mismatches #140622 (comment)) - this is ok.
• Just compiletest-self-test/line-annotation-mismatches.rs or ui/compiletest-self-test/line-annotation-mismatches.rs - this is bad because the path is no longer clickable.

For the actual messages, I might even do sth like:

I'll try something like this.

Also, how does this handle normalizations?

I didn't change any normalization rules.
As I understand, before the annotations are checked, the compiler output is normalized, but without applying the custom normalization rules from //@ normalize-stderr.

[jieyouxu]

What do you suggest reporting instead?

• A relative path tests/ui/compiletest-self-test/line-annotation-mismatches.rs (compiletest: Improve diagnostics for line annotation mismatches #140622 (comment)) - this is ok.
• Just compiletest-self-test/line-annotation-mismatches.rs or ui/compiletest-self-test/line-annotation-mismatches.rs - this is bad because the path is no longer clickable.

I mean the relative-from-check-root path, i.e. tests/ui/compiletest-self-test/line-annotation-mismatches.rs. AFAIK, this is what the vast majority of contributors would use (i.e. ./x invocations from checkout root).

Agreed that the clickable part is really important, because I use that too :D Also compiletest.../ or ui/compiletest.../ is quite ambiguous especially when it's possible to run multiple test suites, so yeah.

[jieyouxu]

Re. what I said about

I think we should only report the source file path once

I was thinking we only report that path once, i.e. after the "Blessing...` lines. However, now that I think about it some more, that can be easy to miss esp. if there's a ton of mismatches. So I was thinking maybe sth like what I suggested re.

expected but with a different kind: tests/ui/compiletest-self-test/line-annotation-mismatches.rs:22:
          message: WARN: cannot find value `unresolved2` in this scope
    expected kind: ERROR
    reported kind: WARN

reported with a different message: tests/ui/compiletest-self-test/line-annotation-mismatches.rs:25:
    expected message: NOTE: not found in this scope
    reported message: 

strikes a reasonable balance between repeating the file too many times (noisy) vs easy to miss / click. I'm ofc open to alternative formats, I can't really think of a better one in this moment.

[petrochenkov]

Updated the PR, although I still need to cleanup the code.

I've applied all the suggestions from the thread, but compressed the output from #140622 (comment) to eat less vertical space (more vertical space is inconvenient when there are many failing tests).
The output looks like this now.

[petrochenkov]

@rustbot ready

[rustbot]

Some changes occurred in tests/ui/sanitizer

cc @rcvalle

[jieyouxu]

Thanks! I played around with this locally, and I think this is definitely better than what we had.

[jieyouxu]

@bors r+ rollup

[bors]

📌 Commit 7a4f09c has been approved by jieyouxu

It is now in the queue for this repository.