compiletest: Improve diagnostics for line annotation mismatches 2

[petrochenkov]

Follow up to #140622 based on feedback from #144590.

[rustbot]

r? @Kobzol

rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[petrochenkov]

Before:

After:

[Kobzol]

Looks slightly better to me, especially the separated headers.

@RalfJung What do you think?

[RalfJung]

That helps a lot, thanks!

I am slightly concerned that from the output it looks like it will still sometimes print multiple "reported with different X", which seems like it could drown the output -- these are just guesses at things that might be matching but they could be entirely unrelated, so we should limit the output to reduce the noise caused by wrong guesses.

[petrochenkov]

I am slightly concerned that from the output it looks like it will still sometimes print multiple "reported with different X", which seems like it could drown the output -- these are just guesses at things that might be matching but they could be entirely unrelated, so we should limit the output to reduce the noise caused by wrong guesses.

That's what I disagree with, #140622 was based on my experience porting error-pattern and kind-less annotations to line annotations with kind. There the extra suggestions were quite helpful, that's why I preferred to keep more of them.
This PR is a trade-off, more suggestions are filtered if better suggestions exist, but multiple suggestions are still reported if they have the same rank.

[RalfJung]

#140622 was based on my experience porting error-pattern and kind-less annotations to line annotations with kind.

That's a one-time migration effort, so I don't see why we'd optimize for that. The common cases in my experience are

• a diagnostic message changed: same kind, same line, the hint is useful but it is shown twice (once as "expected different message" and once as "reported with different message") -- so its already quite verbose for this case (which matters a lot as it means one has to scroll up much more when fixing 20 tests at the same time)
• something changes from warn to err: same message, same line, the hint is useful but I expect it'll also be shown twice like above
• a diagnostic moved to a different line -- seems like the tool can even detect that, very cool, but again it is printed twice
• everything massively changed and you have to take a closer look -- in this case the hints are ~never going to be helpful

multiple suggestions are still reported if they have the same rank.

What is an example of multiple suggestions with the same rank? In the screenshot, the two suggestions at the very bottom are both just confusing to me, I don't see how they could be helpful. Message and kind are completely different, why would we even assume that this is talking about the same error?

[petrochenkov]

In the screenshot, the two suggestions at the very bottom are both just confusing to me, I don't see how they could be helpful. Message and kind are completely different, why would we even assume that this is talking about the same error?

Because in this case if you write //~ stub the suggestions will give you all the diagnostics on this line so you can choose what to cover and copypaste some of them.

[RalfJung]

Okay I can see how if there's an expected diagnostic without a level, and multiple similar diagnostics on that line, there could be multiple matches.

But why would it suggest diagnostics that don't even contain stub? Both error and kind are different then, at that point the chances of the suggestion being right are fairly low in my experience, and the chances of this being entirely unrelated start to get significant.

[petrochenkov]

but again it is printed twice

The duplication is a real problem, yes.
I'd like to resolve it but didn't want to include the fix into #140622, which was already a large change in diagnostics.
Need to figure out

• what exactly is duplication, to not filter away something useful
• what version is better to report when there are duplicates - the "reported but not expected" or "expected but not reported", or maybe choose either one or another based on other factors.

[petrochenkov]

But why would it suggest diagnostics that don't even contain stub?

Because none of the diagnostics contain stub.
Personally, I often literally write some nonsense into the annotations first to see the right annotations in the diagnostics later.

[RalfJung]

Personally, I often literally write some nonsense into the annotations first to see the right annotations in the diagnostics later.

That's a cute trick, but most of the .rs files don't have nonsense in the annotations, so it seems like an odd point to optimize for. (Also, is it documented? Please make the tool usable for everyone, not just yourself. :) At least in my work, I have to fix existing ui tests much more often than write new ones, and having so many incorrect and duplicate suggestions makes that harder than it was a year ago. The signal-to-noise ratio of the output has gone up significantly. (Ofc I also knew exactly how the old output looked. The new one is a lot easier to interpret when first seeing this, especially after this PR.)

I don't think compiletest should take "the message is entirely different" as a signal for "probably the user wrote deliberate nonsense, let's show all the messages". In general that's just not a good assumption. If you want a specific mode where compiletest shows you all the errors on a line so you can copy-paste the text, that should be explicitly requested, not implicitly assumed. Do we allow empty annotations? If not, they could be used as that signal. But I guess we allow them and it means "just any message", so this may need a keyword/sigil of sorts.

[RalfJung]

What are the conditions under which is still shows multiple suggestions? Did I understand correctly that in the case of #144590, it would now show only one suggestion? Does it ever show multiple suggestions for //~ERROR: stub or only for //~ stub? If it's just the latter, that's so rare I think I could live with it.

[petrochenkov]

What are the conditions under which is still shows multiple suggestions?

If there are multiple suggestions with the same rank.
There's a comment in code

            // - message and line / message and kind - great, suggested
            // - only message - good, suggested
            // - known line and kind - ok, suggested
            // - only known line - meh, but suggested

and the ranks are assigned according to that.
It's probably better to look at the examples I'll post below.

Did I understand correctly that in the case of #144590, it would now show only one suggestion?

Yes, in that example it will show one suggestion in all cases.

[Kobzol]

(I'm gonna:

r? RalfJung

since I wasn't involved in the previous discussions and don't work with the UI suite that much to have strong opinions here).

[petrochenkov]

Does it ever show multiple suggestions for //~ERROR: stub

Yes, if there are multiple diagnostics containing "stub" reported on different lines (which is unlikely if it's literally "stub").
Or if there are multiple other ERRORs not containing "stub" are reported on the same line.
Or if there are multiple other WARNs and NOTEs (but not ERRORs) not containing "stub" are reported on the same line.

or only for //~ stub? If it's just the latter, that's so rare I think I could live with it.

This is equivalent to //~ UNKNOWN stub where UNKNOWN simply doesn't match any kind that can be produced by the compiler, otherwise the same logic as above applies.

[RalfJung]

Yes, if there are multiple diagnostics containing "stub" reported on different lines (which is unlikely if it's literally "stub").

So it's listing all lines that have a diagnostic that matches the text? That could be extremely verbose if there's many similar / identical diagnostics. At that point it makes much more sense to look at the stderr file and figure out what is going on IMO.

Or is it only diagnostics that haven't been matched already? That would be a lot better, but still risks printing a lot of irrelevant output.

This is equivalent to //~ UNKNOWN stub where UNKNOWN simply doesn't match any kind that can be produced by the compiler, otherwise the same logic as above applies.

Oh interesting, I would have expected this to match all kinds produced by the compiler.

[RalfJung]

since I wasn't involved in the previous discussions and don't work with the UI suite that much to have strong opinions here).

I wasn't involved in the previous discussions here either, but fundamentally we just don't seem to have consensus on whether the tool should err on the side of being more verbose vs more concise.

I would suggest we land this PR (with the nit about colons resolved, if we have consensus on that), since I consider it a clear improvement -- but it shouldn't close the issue as output still seems very verbose in some cases.

[petrochenkov]

Yeah, I also suggest to land this change, then address the duplication, then maybe reconsider showing the remaining suggestions.

[petrochenkov]

Or is it only diagnostics that haven't been matched already?

Yes, only unmatched diagnostics and annotations are shown in this output.

Oh interesting, I would have expected this to match all kinds produced by the compiler.

It was like that before #139720, but now not specifying the kind is prohibited, so UNKNOWN doesn't match anything.

[petrochenkov]

Updated with the added colons (#144747 (comment)).

[RalfJung]

Thanks!

@bors r+

[bors]

📌 Commit d5ffb06 has been approved by RalfJung

It is now in the queue for this repository.

[petrochenkov]

@bors rollup