Add new optional tidy check to check if ftl error messages are unused

[GuillaumeGomez]

I realized recently while removing a rustdoc feature that error messages could be unused and we apparently had no checks for that. This PR fills this void.

There is one unresolved question (likely due to my ignorance rather than an actual issue): we have (a lot of) error message IDs that are actually not anywhere in ftl files, like attr_parsing_remove_neg_sugg. Is it expected or not?

In any case, there were unused error message IDs so I removed them in this PR as well.

cc @davidtwco
r? @Kobzol

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

tidy extra checks were modified.

cc @lolbinarycat

[bjorn3]

attr_parsing_remove_neg_sugg is defined at

rust/compiler/rustc_attr_parsing/messages.ftl

Lines 208 to 210 in a2db928

	attr_parsing_invalid_meta_item = expected a literal (`1u8`, `1.0f32`, `"string"`, etc.) here, found {$descr}
	.remove_neg_sugg = negative numbers are not literals, try removing the `-` sign
	.quote_ident_sugg = surround the identifier with quotation marks to make it into a string literal

I think

[GuillaumeGomez]

$ git grep -n attr_parsing_remove_neg_sugg
rustc_attr_parsing/src/session_diagnostics.rs:844:#[multipart_suggestion(attr_parsing_remove_neg_sugg, applicability = "machine-applicable")]

Now I'm worried. 👀

[GuillaumeGomez]

Ah the message error ID you displayed is attr_parsing_invalid_meta_item. Did I miss something?

[bjorn3]

I would expect the #[multipart_suggestion(attr_parsing_remove_neg_sugg)] to reference the .remove_neg_sugg attribute. InvalidMetaItemRemoveNegSugg is the remove_neg_sugg field of InvalidMetaItem, which references attr_parsing_invalid_meta_item.

[GuillaumeGomez]

Oh I see. Definitely didn't expect that. Ok, gonna remove the warning then.

[RalfJung]

Yeah the process for naming these "attributes" is quite surprising. It is partially documented at https://rustc-dev-guide.rust-lang.org/diagnostics/translation.html but nothing really explains how fluent message names are looked up, I think. IIRC it was something like, given attr_parsing_remove_neg_sugg we strip the crate prefix and then take the rest as a name for an attribute that is looked up in the scope of the surrounding diagnostic? But I have no clue how it knows whether to do an attribute lookup or a top-level-message lookup, and I have no clue how it makes any sense that the name in rustc (attr_parsing_remove_neg_sugg) matches absolutely nothing in the ftl file.

Maybe @davidtwco can explain the rules.

[RalfJung]

This checks all fluent messages, right? Many of them are not errors but lints. So something like "fluent_messages" or so is probably a better name for this module.

EDIT: There's already several checks called fluent_something so fluent_messages is probably not specific enough.

[RalfJung]

Also, what about the existing check in src/tools/tidy/src/fluent_used.rs? Isn't that trying to do kind of the same thing? It's confusing though:

• That file is documented as "Checks that all Fluent messages appear at least twice", without any explanation for why that is what we want.
• The commit that adds the file is titled "Add a tidy check that checks whether the fluent slugs only appear once" which is the exact opposite of the comment in the file added by that commit
• It has code to emit an error saying "{filename}: message `{name}` is not used" which looks like what this PR is trying to do

Cc @mu001999 (#121860)

[GuillaumeGomez]

It definitely seems to check what this PR does, except it apparently doesn't work?

Well, if confirmed then I'll remove fluent_used.rs and rename error_messages.rs into it, although considering how many files it goes through, I'll still keep it as an optional tidy check.

[lolbinarycat]

There's a few issues with this being an extra check:

1. the original point of extra-checks was that they call an external tool, which this does not (ofc, now that these external tools are automatically downloaded, this doesn't matter much)
2. as-is, this check will never be run unless explicitly requested (no, not even in CI). The existing extra checks (besides the shellcheck one, which is basically dead code) all are set to be run automatically through the tools profile (via the build.tidy-extra-checks key), and are explicitly requested in CI.

the only advantage I see of this being an extra check is that it gets to be part of the same files_modified_batch_filter call as the rest of the extra checks, saving an exec call... but that could also be addressed by improving the caching of that function (something that would also benefit all the other checks that do something similar)

also, if this is to be an extra-check, it should have kind lint, not None (None is kinda a hack put in for spellcheck, which was initially added without my knowledge)

[Kobzol]

Yeah I think that this can be just a normal check that runs everytime. Unless it's super expensive for some reason.

[GuillaumeGomez]

Well, it run a regex on all compiler source code, so takes ~2 seconds on my machine.

[lolbinarycat]

Well, it run a regex on all compiler source code, so takes ~2 seconds on my machine.

just call files_modified directly like other checks do.

[GuillaumeGomez]

😮 TIL, that'd be SO MUCH more efficient. Thanks for the tip!

[mu001999]

Also, what about the existing check in src/tools/tidy/src/fluent_used.rs? Isn't that trying to do kind of the same thing?

It tried to do the same thing and will check whether fluent msg is used only once or not.

The rule is simple, if a fluent msg only appeared once in .flt files, it must be unused in rust.

So based on this, there will be some false-negatives like using the fluent msg in the comment.

But it should work at least for the first removed msg in this PR, I check it manually and it only appears once in .flt files. It's weird 🤔

[mu001999]

Oh, I print the input of fluent_used and find it is empty, it reuse the result of check fluent_alphabetical. So I test the check fluent_alphabetical then, and find it is also broken.

[mu001999]

Found what is wrong 💥

#143724 changed the fluent file extension checking but had a typo 😂 cc @hkBst
https://github.com/rust-lang/rust/pull/143724/files#diff-20fa4bee394077196782fcafd9d4e3475d6dc99f8b6bcd2c0411ab874017ba49R16-R17

So fluent_alphabetical and fluent_used are both broken.

After fixing this, things got fine, I got correct errors on my machine:

tidy error: /Users/mu00/repos/rust/compiler/rustc_attr_parsing/messages.ftl: message `attr_parsing_multiple_renamings` is not used
tidy error: /Users/mu00/repos/rust/compiler/rustc_metadata/messages.ftl: message `metadata_whole_archive_needs_static` is not used
tidy error: /Users/mu00/repos/rust/compiler/rustc_parse/messages.ftl: message `parse_trailing_vert_not_allowed_suggestion` is not used
tidy error: /Users/mu00/repos/rust/compiler/rustc_metadata/messages.ftl: message `metadata_wasm_import_form` is not used
tidy error: /Users/mu00/repos/rust/compiler/rustc_attr_parsing/messages.ftl: message `attr_parsing_multiple_renamings_second` is not used

[hkBst]

@mu001999 Oops, good catch. Strange that the extension for fluent is "ftl" and not "flt"...

Apparently: "FTL stands for Fluent Translation List." -- https://github.com/projectfluent/fluent?tab=readme-ov-file#fluent-syntax-ftl

[RalfJung]

It definitely seems to check what this PR does, except it apparently doesn't work?

Yeah, apparently.
So obviously we have to figure out why instead of adding another check doing the same thing.

Looks like @mu001999 did that. :)

[GuillaumeGomez]

Let's close this PR once the fix for the existing code is open then. :)

[Kivooeo]

Fix just landed, so it should be possible to continue working on it

[bors]

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

[GuillaumeGomez]

Seems like your fix covers everything I did here so even better: closing it. :)

[davidtwco]

Yeah the process for naming these "attributes" is quite surprising. It is partially documented at rustc-dev-guide.rust-lang.org/diagnostics/translation.html but nothing really explains how fluent message names are looked up, I think. IIRC it was something like, given attr_parsing_remove_neg_sugg we strip the crate prefix and then take the rest as a name for an attribute that is looked up in the scope of the surrounding diagnostic? But I have no clue how it knows whether to do an attribute lookup or a top-level-message lookup, and I have no clue how it makes any sense that the name in rustc (attr_parsing_remove_neg_sugg) matches absolutely nothing in the ftl file.

Maybe @davidtwco can explain the rules.

My recollection is that for anything that derives Subdiagnostic, or for #[label]/#[help]/etc attributes on a Diagnostic, it looks up the .name in the context of the message for the containing Diagnostic. I don't recall supporting referring to these with the crate prefix, but that might have been added later by someone else and I've forgotten.