test suite: ERROR and WARN annotations are not mandatory in UI tests

[petrochenkov]

Example: ui/parser/if-in-in.rs.
The test has errors and has corresponding .stderr file, but has no //~ ERROR annotations and it still passes.
This should not happen, //~ ERROR and //~ WARN annotations must be mandatory even if none of them are written in the test yet.

[pnkfelix]

I had thought this was deliberate, in the sense that if you didn’t have any such annotation, then the output wasn’t checked?

[petrochenkov]

From what I've seen no annotations usually mean forgotten annotations.

Annotations also make compile-fail tests readable - you can see what the test is testing, what lines have issues. Otherwise you have to compare the source and the output side-by-side and that's hard and unreliable, especially given that line numbers in .stderr files are partially anonymized.

I'd prefer to keep this minor annotation burden to avoid tests that are accidentally wrong or unreadable.

[pnkfelix]

I personally agree that no annotations usually mean forgotten (or at least unanticipated) annotations.

And I'm certainly a strong proponent of using annotations when one really wants to ensure that daagnostics are actually associated with particular points in the code.

Personally I think we should require them for ERROR (*)

But I don't want to require them in all cases for NOTE or HELP; that would make the annotation burden for test writers too extreme.

And I don't know what to think about whether to require it for WARN or not.

That dilemma is what I think got us to where we are today, where (in principle), if you write a single annotation of a certain kind, then you get checking for all annotations and diagnostics of that kind for the given test.

---

(*): The fact that currently we don't is probably a holdover from the categorization of the tests into ui/compile-fail/run-pass directories. (In hindsight, maybe everything would have been better doing the same thing to compile-fail that I did to run-pass: namely, make all the tests there use the ui infrastructure.)

[pnkfelix]

Lets discuss this at the compiler meeting.

[pnkfelix]

Discussed at T-compiler meeting.

The general consensus was that we should do what @petrochenkov suggests (in that we should require tests in the ui (and run-pass) suite to have accurate //~ ERROR and //~ WARN annotations.

My main hesitation about this was about the barrier this would introduce for test authors. But then I was reminded that if a test wants to opt out, it can just add #![allow(warnings)]. That was enough to convince me that @petrochenkov is absolutely correct here.

[petrochenkov]

if a test wants to opt out, it can just add #![allow(warnings)].

Beside that, UI tests allow(unused) by default via compiletest, so if warning happens it usually means something actually needs attention.

[RalfJung]

then I was reminded that if a test wants to opt out, it can just add #![allow(warnings)].

This will also remove warnings from the stderr file though. Currently one can test warnings via the stderr file only, that would no longer be possible. Any test that adds allow(warnings) because of this is a test that tests fewer things than it does right now.

[petrochenkov]

With unused allowed by default warnings are rare and can be easily annotated with //~ WARN, no need to allow(warnings).

We should actually make a pass through tests and remove allow(...) annotations when possible.
There are few exceptions, like some test checking name resolution conflicts may want to disable the noise from non_camel_case_... and similar lints.

[petrochenkov]

A somewhat opposite issue - #56277.

[saleemjaffer]

@petrochenkov I would like to help with this. I am a newbie to rust.

[petrochenkov]

#65759 (comment) tells how to resolve this issue.

[petrochenkov]

@saleemjaffer
Sorry, I never noticed your comment.
If you are still interested, there's now an instruction.
x.py test --bless --stage 1 src/test/ui will run the tests and update their outputs.