New attribute macros format for diagnostic structs without fluent slug #117867

chenyukang · 2023-11-13T10:31:35Z

The background for this:
Split out Fluent from Diagnostics Structs - HackMD

The change on compiler/rustc_macros/src/diagnostics are making Diagnostic and Subdiagnostic compatible with the old format and new format, after we migrating all crates, we may need to clean up old code.
I made a program to migrate from old format to the new format for each crate automatically(but still need some trivial manual touch), I'd like to get some feedback for parser as a starting point.

Ping @estebank @oli-obk who may also interested on this part.

rustbot · 2023-11-13T10:31:47Z

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

rustc_errors::translation was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

chenyukang · 2023-11-13T11:13:37Z

compiler/rustc_macros/src/diagnostics/diagnostic_builder.rs

-                    return Ok(());
+            match &attr.meta {
+                // support syntax `#[diag("message ...", code = "E0045", note = "node message")]`
+                Meta::List(MetaList { path, tokens: token_stream, .. }) => {


This is to support:

#[diag("message....")] // or #[diag("message", code = "...", note = "...")]

If it's:

#[diag(label = "message....")]

it will be handled by attr.parse_nested_meta below.

I'd suggest we only support the late one, which is more consistent with suggestion, multipart_suggestion, or we only support #[diag("message....")](without extra nested elements), if there are multiple nested elements label = is required?

chenyukang · 2023-11-13T11:14:35Z

compiler/rustc_parse/src/parser/attr.rs

@@ -58,9 +56,10 @@ impl<'a> Parser<'a> {
                    let span = self.token.span;
                    let mut err = self.sess.span_diagnostic.struct_span_err_with_code(
                        span,
-                        fluent::parse_inner_doc_comment_not_permitted,


How do we handle this?
maybe we need a new macro to mark the content need to be translated.

fmease · 2023-11-13T12:42:34Z

I think this needs an MCP (major change proposal over at https://github.com/rust-lang/compiler-team)?
@rustbot label needs-mcp

Noratrieb

+1 from me, but I think it would be nicer to have a smaller crate with fewer diagnostics as an example here instead of rustc_parse, which is quite big and will lead to some merge conflicts

rustbot · 2023-11-14T01:15:15Z

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

rustc_errors::translation was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

chenyukang · 2023-11-14T01:37:33Z

compiler/rustc_parse/messages.ftl

-
-parse_inner_attr_explanation = inner attributes, like `#![no_std]`, annotate the item enclosing them, and are usually found at the beginning of source files
-parse_inner_attr_not_permitted = an inner attribute is not permitted in this context
-    .label_does_not_annotate_this = {parse_label_inner_attr_does_not_annotate_this}


How do we support this scenario in the new format?
I think we may support it like this:

#[derive(Diagnostic)] #[diag(label = "the label...", does_not_annotate_this = "the content can be refer...")] pub(crate) struct ErrorStruct { ... pub sugg: Option<SubErrorStruct>, } #[derive(Subdiagnostic)] pub(crate) struct SubErrorStruct { #[suggestion("{does_not_annotate_this}") pub span: Span }

chenyukang · 2023-11-14T02:28:48Z

+1 from me, but I think it would be nicer to have a smaller crate with fewer diagnostics as an example here instead of rustc_parse, which is quite big and will lead to some merge conflicts

yeah, I fixed the conflict.
I started with parser because its errors are complex enough to contain different kinds of scenarios😆

rustbot · 2023-11-19T17:46:45Z

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

rustbot · 2023-12-08T11:11:44Z

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

bors · 2023-12-08T19:28:13Z

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

bors · 2023-12-10T14:34:57Z

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

apiraino · 2023-12-28T11:31:57Z

Switching to waiting on author to take action. Seems that an MCP should be appropriate for these changes and also the rust-analyzer comment) suggest to be resolved.

Thanks!

@rustbot author

chenyukang · 2023-12-28T11:48:50Z

Switching to waiting on author to take action. Seems that an MCP should be appropriate for these changes and also the rust-analyzer comment) suggest to be resolved.

Thanks!

@rustbot author

The rust-analyzer related change was committed by accident, I already rollback it.
Seems there are some conflicts right now, I will fix it and add a MCP.

bors · 2024-01-04T22:26:56Z

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

davidtwco

Apologies it took me so long to get to this. I've left some feedback. As it's been a while, this may be a big rebase, particularly given the many changes to the diagnostic internals that have been happening recently.

davidtwco · 2024-02-12T13:21:51Z

compiler/rustc_macros/src/diagnostics/diagnostic.rs

+                    }
+                }
+                (Some(_slug), Some(_raw_label)) => {
+                    unreachable!("BUG: slug and raw label specified");


This should be an error from the proc macro rather than a unreachable!.

davidtwco · 2024-02-12T13:23:33Z

compiler/rustc_errors/src/translation.rs

+            DiagnosticMessage::FluentRaw(msg) => {
+                // FIXME(yukang): calculate the `slug` from the raw fluent content,
+                // The fluent resources are generated by a simple standalone visitor:
+                // https://github.com/chenyukang/fluent-utils/blob/main/src/visitor.rs#L13-L97


I think I still prefer some solution where the compiler is able to emit the reference ftl rather than needing another tool to extract it. I know that we tried to do this by writing static variables into a section, but we might also be able to do it from the proc macro by writing to the filesystem (which isn't ideal) based on an environment variable.

davidtwco · 2024-02-12T13:24:15Z

compiler/rustc_macros/src/diagnostics/diagnostic_builder.rs

@@ -44,9 +46,15 @@ pub(crate) struct DiagnosticDeriveVariantBuilder {
    /// has the actual diagnostic message.
    pub slug: SpannedOption<Path>,

+    /// Label is a the text embedded in the struct attribute and corresponds to the diagnostic


Maybe message instead of label because label is the term we use for a type of subdiagnostic.

davidtwco · 2024-02-12T13:26:42Z

compiler/rustc_macros/src/diagnostics/diagnostic_builder.rs

@@ -182,59 +193,116 @@ impl DiagnosticDeriveVariantBuilder {
        let name = attr.path().segments.last().unwrap().ident.to_string();
        let name = name.as_str();

-        let mut first = true;
+        let mut set_label = false;
+        let keys = vec!["note", "help", "warning", "suggestion"];

        if name == "diag" {


Would it be simpler to introduce a mutually exclusive diag_new and re-use some logic, what would parse as slug becomes message (or label as you have it now)?

davidtwco · 2024-02-12T13:28:07Z

compiler/rustc_macros/src/diagnostics/utils.rs

@@ -601,6 +612,9 @@ pub(super) struct SubdiagnosticVariant {
    pub(super) kind: SubdiagnosticKind,
    pub(super) slug: Option<Path>,
    pub(super) no_span: bool,
+    /// A subdiagnostic can have a raw_label field, e.g. `#[help("some text")]`.
+    /// if `slug` is None, this field need to be set.
+    pub(super) raw_label: Option<LitStr>,


Can we use an enum for slug that has a message or a slug, to capture the mutual exclusive-ness of these?

davidtwco · 2024-02-12T13:29:06Z

compiler/rustc_macros/src/diagnostics/utils.rs

@@ -29,6 +29,17 @@ pub(crate) fn new_code_ident() -> syn::Ident {
    })
 }

+pub(crate) fn convert_to_litstr(lit: &proc_macro2::Literal) -> LitStr {


Why is this necessary?

estebank · 2024-02-16T19:27:17Z

I think it might be a good idea for @chenyukang, @davidtwco, @oli-obk and myself to talk synchronously sometime soon. We need to come to an agreement to what the long term plan is here and I know I have thoughts about options we have available which would affect whether this PR can be merged as is or would require changes. I'm really excited about this project and want to ensure its success.

chenyukang · 2024-02-18T13:26:28Z

@estebank good idea, I'm ok for online meeting.

Dylan-DPC · 2024-08-01T07:39:23Z

@chenyukang any updates on this? thanks

oli-obk · 2024-08-01T09:02:47Z

cc @Manishearth as you also have opinions on translation infrastructure, there's a summary of what this PR is about in https://hackmd.io/@e0xmMzbUT7SeCAVAVjBv2Q/S1XOUOdQa (the hackmd linked from the main post)

Manishearth · 2024-08-01T09:18:52Z

I've expressed opinions on this before: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Localization.20infra.20interferes.20with.20grepping.20for.20error

TLDR: translation files are source code, they have intentionalilty to them, they ought to be commentable, they should be managed by the translation team like source code.

It's theoretically possible to have an autogenerate-the-ftl-file workflow that retains this, but you'd need a bunch more design work. At the very very least: we'd need an attribute that allows one to provide context to translators, which turns into a comment in the FTL file. Another common need is organizing the file, it's less important and something people can get without, but definitely something we lose here.

This proposal seems to be designed entirely with the needs of compiler devs in mind, without consideration for the needs of translators.

instead of referring to slugs, we'd generate those from the type name and a hash of the message contents, i.e. signature-redeclaration-8af429de or something. Because of the message being present here, there are no unused Fluent messages or else there would be unused types; and we can check the fields match the interpolations at compile-time.

I don't think this is a good idea: there is a distinction between "this is a wording change that's just sprucing stuff up in English" and "this is a wording change that changes the meaning of this slug and needs retranslation".

I do recognize that the current system is prone to people making changes that do not notify the translation teams (was hoping to have some automation set up eventually that does this).

Furthermore, slugs are meant to be human readable.

This separation also makes the next steps more difficult, we want to use Pontoon to do the translations, and it doesn't handle versioning of messages. We have to manually add -1, -2 prefixes to the slugs and that's dumb.

This is intentional choice (and common practice in translation environments): people should be carefully considering whether or not the prefix needs to be added when this crops up.

So I'm mostly against such a change. If someone who understands the needs of translation teams were to design such a change it could work, but at the moment that's not the case, the most basic ability to comment and organize the translation file is lost.

I'd be grudgingly okay with such a change with the following modifications:

There is a way (probably a new attribute) to provide context to translators, that turns into an FTL comment
The hash autogeneration is removed and replaced with an explicit versioning attribute. I think there's tons of room for improvement on workflows there¹, but a hash is basically a complete non starter here.

Would very much be in favor of automation that catches whenever an English string changes without a version bump, and requires the translation team to approve it ↩

rustbot assigned davidtwco Nov 13, 2023

chenyukang commented Nov 13, 2023

View reviewed changes

chenyukang changed the title ~~[WIP] New macro proc format for diagnostic structs without fluent slug~~ [WIP] New attribute macros format for diagnostic structs without fluent slug Nov 13, 2023

rustbot added the needs-mcp This change is large enough that it needs a major change proposal before starting work. label Nov 13, 2023

Noratrieb reviewed Nov 13, 2023

View reviewed changes

chenyukang force-pushed the errors-refactor-no-fluent branch from 176bb46 to bf3b14d Compare November 14, 2023 01:15

This comment has been minimized.

Sign in to view

chenyukang commented Nov 14, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

chenyukang force-pushed the errors-refactor-no-fluent branch from 728a864 to 9d8105d Compare November 14, 2023 02:14

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

chenyukang force-pushed the errors-refactor-no-fluent branch 3 times, most recently from b1820bd to 1a3dc67 Compare November 16, 2023 08:22

This comment was marked as resolved.

Sign in to view

chenyukang force-pushed the errors-refactor-no-fluent branch from d5ece88 to 1df013b Compare November 19, 2023 17:46

chenyukang force-pushed the errors-refactor-no-fluent branch from 1df013b to db6553b Compare November 19, 2023 18:02

chenyukang force-pushed the errors-refactor-no-fluent branch from 14ed478 to 03c8665 Compare December 9, 2023 02:58

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 28, 2023

chenyukang added 9 commits December 28, 2023 20:51

begin on no-fluent-errors

ce78bfb

more cleanup on diags

46c8b80

more updates on parser

670d1a1

remove messages.ftl from parser

c9f8439

remove parser fluent from rustc_expand

653159c

fix diagnostic derive

d768dbb

add FluentRaw to represent fluent raw content

bb2eb3f

find fluent resource from hash slug

a173063

fix conflicts in diagnostics

b25a2bb

chenyukang force-pushed the errors-refactor-no-fluent branch from 03c8665 to b25a2bb Compare December 28, 2023 15:24

davidtwco requested changes Feb 12, 2024

View reviewed changes

davidtwco mentioned this pull request Feb 20, 2024

Raw fluent diagnostic structs #121334

Closed

alex-semenyuk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2024

chenyukang closed this Oct 17, 2024

jieyouxu mentioned this pull request Oct 26, 2024

Tracking Issue for rustc's translatable diagnostics infrastructure #132181

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New attribute macros format for diagnostic structs without fluent slug #117867

New attribute macros format for diagnostic structs without fluent slug #117867

chenyukang commented Nov 13, 2023

rustbot commented Nov 13, 2023

chenyukang Nov 13, 2023

chenyukang Nov 13, 2023

fmease commented Nov 13, 2023

Noratrieb left a comment

rustbot commented Nov 14, 2023

This comment has been minimized.

chenyukang Nov 14, 2023

This comment has been minimized.

chenyukang commented Nov 14, 2023

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

rustbot commented Nov 19, 2023

rustbot commented Dec 8, 2023

bors commented Dec 8, 2023

bors commented Dec 10, 2023

apiraino commented Dec 28, 2023

chenyukang commented Dec 28, 2023

bors commented Jan 4, 2024

davidtwco left a comment

davidtwco Feb 12, 2024

davidtwco Feb 12, 2024

davidtwco Feb 12, 2024

davidtwco Feb 12, 2024

davidtwco Feb 12, 2024

davidtwco Feb 12, 2024

estebank commented Feb 16, 2024

chenyukang commented Feb 18, 2024

Dylan-DPC commented Aug 1, 2024

oli-obk commented Aug 1, 2024

Manishearth commented Aug 1, 2024

New attribute macros format for diagnostic structs without fluent slug #117867

New attribute macros format for diagnostic structs without fluent slug #117867

Conversation

chenyukang commented Nov 13, 2023

rustbot commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease commented Nov 13, 2023

Noratrieb left a comment

Choose a reason for hiding this comment

rustbot commented Nov 14, 2023

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

chenyukang commented Nov 14, 2023

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

rustbot commented Nov 19, 2023

rustbot commented Dec 8, 2023

bors commented Dec 8, 2023

bors commented Dec 10, 2023

apiraino commented Dec 28, 2023

chenyukang commented Dec 28, 2023

bors commented Jan 4, 2024

davidtwco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank commented Feb 16, 2024

chenyukang commented Feb 18, 2024

Dylan-DPC commented Aug 1, 2024

oli-obk commented Aug 1, 2024

Manishearth commented Aug 1, 2024

Footnotes