make tidy slightly less noisy

[RalfJung]

For some reason, tidy prints a bunch of stuff even when there's nothing to complain about. That's not great, it means there's a bunch of text one has to parse to understand if there's any errors hidden inside those messages.

With this patch, the output for me is

fmt check
fmt: checked modified file src/tools/tidy/src/error_codes.rs
fmt: checked modified file src/tools/tidy/src/rustdoc_json.rs
tidy check
Checking tidy rustdoc_json...
x.py completions check

Do these lines saying what is being checked make sense? I kept them for now because it may be that the error messages are hard to parse without that context, though ideally the errors would all establish this context themselves.

Do we really need to list all files that are being formatted?

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
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

[jieyouxu]

See also #146316

[jieyouxu]

I think I would be very much in favor of somehow making this easier to tell what's "INFO"-level versus what's "WARN"/"ERROR"-level.

r? Kobzol (won't get to this during weekday)

[Kobzol]

It's funny that we have just discussed this exact thing last week in #146316 😆 As was noted there, the output that something was (or was not) checked is actually useful, because tidy can do various optional things and it's not always obvious if it did check something successfully or if that check was actually skipped.

I think that an ideal solution would be to use some more comprehensive logging system in tidy, which would make it obvious where the individual checks start and end, and what is their conclusion. But that would be more work, ofc.

A temporary solution might be to print a big green "EVERYTHING IS FINE" message when tidy ends successfully. Would that help you understand what is the outcome of tidy execution?

[RalfJung]

This PR doesn't remove the lines that say "checking x...". But it removes the lines saying "X was not changed", which is an entirely pointless thing to say IMO. We also don't list all the files that did not get changed and hence don't have their format checked (for obvious reasons, but the same reasons apply for the other checks as well IMO).

This seems separate from the question of how to format the various "checking foo..." messages -- I think the output of our test harness is a good template for that, but obviously making that all consistent is a bunch of work.

[Kobzol]

That specific message is there because tidy has essentially three modes for doing optional checks - don't check, check always, and check only if the input files were modified. So ideally, it should print something like "check disabled" vs "check skipped" to make it clear that the files weren't completely ignored.

@GuillaumeGomez Do you find the information that the rustdoc files were not modified valuable enough to keep it in tidy, even though it's a bit spammy?

[RalfJung]

Everyone is seeing those messages, and 99% of the time people aren't even aware what those rustdoc files do.

In which sense is that check even optional? I didn't enable it, it seems to be on-by-default.

If you want to know when it ran on a file, have it print the files it works on (similar to what fmt already does). That way it's only verbose in the rare case that someone actually changed the file.

[Kobzol]

Implemented #146592 to solve this in a more structured way.

[RalfJung]

Wow, that's huge! I'll close this one then. Thanks a ton :-)

[GuillaumeGomez]

@GuillaumeGomez Do you find the information that the rustdoc files were not modified valuable enough to keep it in tidy, even though it's a bit spammy?

I need to know that it was at least checked that there was nothing to check (ie, no rustdoc file modified). Otherwise it's hard to know whether the files are now ignored by tidy or if they're actually ignored for a good reason.

[RalfJung]

I don't see why rustdoc would be so special that everyone needs to be constantly reminded that they did, in fact, not change rustdoc. We also don't do this for all the other things tidy checks.

[GuillaumeGomez]

I don't think that rustdoc is special but that the others that aren't checked all the time should also mention that there is nothing to be checked for them.

[RalfJung]

No, they shouldn't. That just doesn't scale. We can fill entire screens with all the things we are not checking.

Why would tidy forget to check these files when they do get changed? Are you expecting someone to break tidy's logic or so? We have tests and reviews for that. But nowhere else do we give such a detailed trace of what something does out of paranoia that it might change one day. Again, just imagine rustc or Miri or clippy worked like that. They'd be entirely unusable.

[GuillaumeGomez]

Well, considering how many times these tests broke because files were moved/renamed, I don't think it's paranoia, just carefulness. You'll note that we don't display each rustdoc file that is ignored, just that a rustdoc tidy check was skipped. A few lines of output is fine.

[RalfJung]

A few lines of output is fine.

Tragedy of the commons. A few lines of output in your own pet tool only you use is fine. A few lines of output in large shared infrastructure, multiplied by a dozen projects each adding a few lines of output, is a mess. The only reason it's not a mess yet is that everyone else kept their checks quiet.

The fact that two team members independently went so far as to actually make a PR to cut down the output should make it clear that there's a real concern here.

[GuillaumeGomez]

Sure, but how many lines are we talking about exactly here?

[Kobzol]

I think that a reasonable compromise might be to only print this line if the auto mode was used for JS files, which should be easily doable after #146592 lands. I think that could alleviate both concerns.

[GuillaumeGomez]

That sounds like an acceptable compromise indeed. 👍

[RalfJung]

I'm looking forward to #146592. :)