UI tests: remove redundant colons in line annotations

[petrochenkov]

The first commit is a mechanical change.
Search: (//(\[.*\])?~.*(HELP|ERROR|NOTE|HELP|WARN|WARNING)):
Replace with: $1
In: ./tests/{ui,ui-fulldeps,rustdoc-ui,incremental}/**/*.{rs,fixed}

The colons are redundant, and annotations without the colons are shorter, less noisy, and overwhelmingly more common, so let's standardize on the colon-less syntax.
#139427 and one more follow up PR will make sure that new colons won't appear.

After merging the first commit will need to be added to .git-blame-ignore-revs.

r? @jieyouxu

[petrochenkov]

@rust-lang/compiler What do you think?

[fee1-dead]

I don't think this is a useful change as I don't see the benefit in forcing annotations to be one way or the other.

It also makes it harder for someone to just copy the error message from stderr and turn that into an annotation, i.e.:

error: const stability on the impl does not match the const stability on the trait
  --> $DIR/staged-api.rs:98:1
   |
LL | impl const U for u8 {}
   | ^^^^^^^^^^^^^^^^^^^^^^
   |

turned into

impl const U for u8 {}
//~^ error: const stability on the impl does not match the const stability on the trait

[petrochenkov]

just copy the error message from stderr and turn that into an annotation

#139427 will also make it harder by standardizing on ERROR instead of error (which is rare in the test suite, more rare than the colon), the part of the string after error: will need to be copypasted.

Also "do not just copy-paste" is sort of by design?
For compiler copy-paste there are .stderr files, and annotations are the way to do a human double check on them, i.e. at least to read the annotation and think "yep, that's the right diagnostic at the right place".

[fee1-dead]

Fair. I am not seeing a lot of benefits for the churn here, but I won't stand in the way of progress if others agree that this is a good idea :)

[lcnr]

personally in favor, our UI test suite will continue to exist for quite a while and the merge conflicts caused by this change should be rare and easy to fix. I think having multiple possibe ways to express stuff here is confusing

[RalfJung]

I think we should write these with a colon, and would rather see the style without colon removed. The absence of the colon makes it harder to read since it lacks a clear separation of "keyword" (error level) and message. The ui_test crate (used by clippy and Miri for their ui tests) also requires the colon.

So IMO this PR makes things strictly worse. Also IMO this requires an MCP given that it's a tree-wide change that will affect basically every compiler dev.

[petrochenkov]

I think we should write these with a colon, and would rather see the style without colon removed.

That's an order of magnitude larger change though.

we@WIN-GQ6AS7OLS34 MINGW64 ~/rust
$ grep -r ERROR: ./tests/ui | wc -l
3104

we@WIN-GQ6AS7OLS34 MINGW64 ~/rust
$ grep -r ERROR ./tests/ui | wc -l
33401

The absence of the colon makes it harder to read since it lacks a clear separation of "keyword" (error level) and message.

If the keyword is uppercase, like ERROR, then it's quite visible, IMO.

Also IMO this requires an MCP given that it's a tree-wide change that will affect basically every compiler dev.

Sure, just collecting an initial reaction for now.

[nnethercote]

I didn't know you could (a) write error in lower case, or (b) include a colon. So this PR seems fine to me, in that it eliminates less-common alternatives I didn't even know were possible, and which don't provide any additional functionality.

[oli-obk]

Funny data point: ui_test and thus miri and clippy require the colon

[jieyouxu]

Personally I'd be in favor of always requiring a colon, so sth more like

//~ DIAG_LEVEL: the actual message

But agreed that this is something that we should have an MCP to decide on a consistent way to write the ui test annotations (I'm less attached to which, but just having a consistent style).

[jieyouxu]

I didn't know you could (a) write error in lower case, or (b) include a colon. So this PR seems fine to me, in that it eliminates less-common alternatives I didn't even know were possible, and which don't provide any additional functionality.

Slightly off-topic, but, compiletest currently has multiple bits where it's too lax in what "alternative forms" are accepted for IMHO no real benefit (and arguably leads to confusion), such as in directive handling. In that sense, I'm very much in favor of PRs such as this where we reduce the variations possible and just converge on more canonical forms.

[RalfJung]

FWIW there are already other differences in style to ui_test, where apparently compiletest was inspired by ui_test (at least, ui_test used that syntax first) but then decided to use a different style for no reason that I can recall having been discussed: //@ compile-flags vs //@compile-flags -- compiletest accepts both of them but for some reason we always add the space. (This also requires a colon in ui_test; not sure if it does in compiletest.)

[RalfJung]

That's an order of magnitude larger change though.

You should add changing Miri and Clippy to the numbers for "remove colons". Though their test suites are much smaller so the result would probably not change substantially.

However, I think the fact the one tool we use in the tree that forces a particular style does require the colon should carry some weight as well.

The colons are redundant, and annotations without the colons are shorter, less noisy

By that argument, Rust should use name type instead of name: type -- all the same arguments apply. And yet we use name: type syntax since it is much easier to read.

[petrochenkov]

By that argument, Rust should use name type instead of name: type -- all the same arguments apply. And yet we use name: type syntax since it is much easier to read.

FWIW, I don't have anything against the first syntax, or against C++'s type name, as a user.
They are just hard to unambiguously parse for the compiler, and hard to make the type optional.
For compiletest annotations that's not the case, both KIND: msg and KIND msg are equally easy to parse.

[RalfJung]

I have a really hard time reading (and writing) Go code due to the lack of a colon, I find it quite irritating. type name works for me, maybe because I used enough C/C++/Java. So maybe overall that's more a matter of habits. And my habit for ui tests has always been to use the colon (not sure why, I guess the very first ui test file I looked at used them); I didn't even realize most files do not use colons -- most files I ever looked at use them^^.

[bors]

☔ The latest upstream changes (presumably #139482) made this pull request unmergeable. Please resolve the merge conflicts.

[WaffleLapkin]

I always strongly preferred level: style (i.e. lowercase + colon) as I find it the most readable and it also mirrors rustc output (which makes it even more readable, since everyone is used to rustc output by necessity). LEVEL really feels off, especially with rust (where only constants scream).

I like the idea of having a single style, thought I also think this should be an MCP.

While I understand the idea behind making the most common choice the only possible, I don't think that LEVEL (uppercase + no colon) is the most popular because it's any good. Seems to me that it's just an artifact of the past, of the way the code base evolved.

[lcnr]

given my perceived agreement that we should unify the style here, and the disagreement about what it should be. Please open an MCP (with prolly just a quick vote of compiler team members to paint the bikeshed)

I always strongly preferred level: style (i.e. lowercase + colon)

:< I very much prefer forced upper-case style //~^ ERROR, don't really care about the colon. It makes these annotations pop out more and makes the files easier to parse imo

[petrochenkov]

I'll close this since there's an open MCP now.