Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

diagnostics: translation infrastructure #95512

Merged
merged 23 commits into from
Apr 5, 2022

Conversation

davidtwco
Copy link
Member

An implementation of the infrastructure required to have translatable diagnostic messages.

  • Introduces a DiagnosticMessage type which can represent both the current non-translatable messages and identifiers for Fluent.
  • Modifies current diagnostic API so that existing calls still work but DiagnosticMessages can be provided too.
  • Adds support for always loading a "fallback bundle" containing the English diagnostic messages, which are used when a DiagnosticMessage::FluentIdentifier is used in a diagnostic being emitted.
  • Adds support for loading a "primary bundle" which contains the user's preferred language translation, and is used preferentially when it contains a diagnostic message being emitted. Primary bundles are loaded either from the path provided to -Ztranslate-alternate-ftl (for testing), or from the sysroot at $sysroot/locale/$locale/*.ftl given a locale with -Ztranslate-lang (which is parsed as a language identifier).
  • Adds "diagnostic args" which enable normally-interpolated variables to be made available as variables for Fluent messages to use.
  • Updates #[derive(SessionDiagnostic)] so that it can only be used for translatable diagnostics and update the handful of diagnostics which used the derive to be translatable.

For example, the following diagnostic...

#[derive(SessionDiagnostic)]
#[error = "E0195"]
pub struct LifetimesOrBoundsMismatchOnTrait {
    #[message = "lifetime parameters or bounds on {item_kind} `{ident}` do not match the trait declaration"]
    #[label = "lifetimes do not match {item_kind} in trait"]
    pub span: Span,
    #[label = "lifetimes in impl do not match this {item_kind} in trait"]
    pub generics_span: Option<Span>,
    pub item_kind: &'static str,
    pub ident: Ident,
}

...becomes...

#[derive(SessionDiagnostic)]
#[error(code = "E0195", slug = "typeck-lifetimes-or-bounds-mismatch-on-trait")]
pub struct LifetimesOrBoundsMismatchOnTrait {
    #[primary_span]
    #[label]
    pub span: Span,
    #[label = "generics-label"]
    pub generics_span: Option<Span>,
    pub item_kind: &'static str,
    pub ident: Ident,
}
typeck-lifetimes-or-bounds-mismatch-on-trait =
    lifetime parameters or bounds on {$item_kind} `{$ident}` do not match the trait declaration
    .label = lifetimes do not match {$item_kind} in trait
    .generics-label = lifetimes in impl do not match this {$item_kind} in trait

r? @estebank
cc @oli-obk @Manishearth

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 31, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt.

cc @calebcartwright

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Mar 31, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2022
@davidtwco davidtwco marked this pull request as ready for review April 1, 2022 02:39
@rust-log-analyzer

This comment has been minimized.

@davidtwco
Copy link
Member Author

I'll wait for a review and feedback before fixing any CI failures, just in case any larger changes are required.

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks quite good from an i18n standpoint! For the fluent stuff we probably want to support directories of ftl files, not just single large ftl files.

You can see what the website does here

compiler/rustc_error_messages/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_error_messages/src/lib.rs Show resolved Hide resolved
/// Simplified version of `FluentArg` that can implement `Encodable` and `Decodable`. Collection of
/// `DiagnosticArg` are converted to `FluentArgs` (consuming the collection) at the start of
/// diagnostic emission.
pub type DiagnosticArg<'source> = (Cow<'source, str>, DiagnosticArgValue<'source>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: probably worth making this a struct (tuple or otherwise)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm deliberately using a tuple here because the FromIterator impl on FluentArgs works with this representation.

compiler/rustc_error_messages/src/lib.rs Outdated Show resolved Hide resolved
@davidtwco
Copy link
Member Author

For the fluent stuff we probably want to support directories of ftl files, not just single large ftl files.

This is how I've implemented the user-requested language support, it loads every resource (.ftl file) in the $sysroot/share/locale/$locale/ directory. I haven't done this with the fallback bundle just because I was using include_str!, but there's no reason we couldn't just add more include_str!s for each new .ftl file we want in the default bundle.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 2, 2022

r? @oli-obk

Taking over from Esteban to keep his queue from growing. Will sync separately with him on it

@rust-highfive rust-highfive assigned oli-obk and unassigned estebank Apr 2, 2022
@rust-log-analyzer

This comment has been minimized.

src/tools/tidy/src/deps.rs Outdated Show resolved Hide resolved
@bors

This comment was marked as resolved.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 3, 2022

@bors r+ p=1 bitrotty

@bors
Copy link
Contributor

bors commented Apr 3, 2022

📌 Commit bb04e494e2c237e5195b33fbdf4db1afb8ffb878 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2022
@bors
Copy link
Contributor

bors commented Apr 3, 2022

⌛ Testing commit bb04e494e2c237e5195b33fbdf4db1afb8ffb878 with merge 4c269a7b4d2715f1379c500ff51ffc6f4aa6dd08...

@bors

This comment was marked as outdated.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 3, 2022
@bors
Copy link
Contributor

bors commented Apr 5, 2022

📌 Commit ccd4820 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 5, 2022
@bjorn3
Copy link
Member

bjorn3 commented Apr 5, 2022

This is how I've implemented the user-requested language support, it loads every resource (.ftl file) in the $sysroot/share/locale/$locale/ directory.

Does it fallback to the default sysroot if --sysroot is passed and this sysroot doesn't have translations?

@davidtwco
Copy link
Member Author

davidtwco commented Apr 5, 2022

This is how I've implemented the user-requested language support, it loads every resource (.ftl file) in the $sysroot/share/locale/$locale/ directory.

Does it fallback to the default sysroot if --sysroot is passed and this sysroot doesn't have translations?

It will only fail when it looks for a locale-specific directory in the sysroot and it isn't there, i.e. when the $user_sysroot/share/locale/$locale directory doesn't exist.

If that directory exists but doesn't contain any Fluent resources (or even anything that all), then it won't fail, it'll just always fallback to the default locale. If that directory contains Fluent resources which are empty, then it also won't fail, it'll just fallback to the default locale. In both of these cases we construct a FluentBundle for that locale, it just ends up being empty and that's the same as if it had every message except for one and we wanted to fallback to the English default.

I don't do anything different if it's a --sysroot from the user or the default.

@bjorn3
Copy link
Member

bjorn3 commented Apr 5, 2022

I don't do anything different if it's a --sysroot from the user or the default.

You might want to. Otherwise using xargo and other similar programs would cause translations to break as they don't add translations from the default sysroot to the created sysroot.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#95234 (bootstrap.py: nixos check in /etc/os-release with quotes)
 - rust-lang#95449 (Fix `x doc --stage 0 compiler`)
 - rust-lang#95512 (diagnostics: translation infrastructure)
 - rust-lang#95607 (Note invariance reason for FnDef types)
 - rust-lang#95645 (Fix intra doc link ICE when trying to get traits in scope for primitive)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Apr 5, 2022

⌛ Testing commit ccd4820 with merge 634770c...

@bors bors merged commit d473024 into rust-lang:master Apr 5, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 5, 2022
@davidtwco davidtwco deleted the diagnostic-translation branch April 5, 2022 12:12
rylev added a commit to rylev/rust that referenced this pull request Apr 8, 2022
…ation, r=oli-obk"

This reverts commit d473024, reversing
changes made to 73eab35.
fee1-dead added a commit to fee1-dead-contrib/rust that referenced this pull request Apr 12, 2022
…t-only-warn, r=oli-obk

sess: warn w/out fluent bundle w/ user sysroot

Addresses rust-lang#95512 (comment).

When a custom sysroot is requested, then don't error when translation resources are not found, only warn.

r? `@bjorn3`
davidtwco added a commit to davidtwco/rust that referenced this pull request Apr 21, 2022
The documentation comment for this derive is out-of-date, it should have
been updated in rust-lang#95512.

Signed-off-by: David Wood <[email protected]>
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 21, 2022
…nor-changes, r=oli-obk

errors: minor translation-related changes

- For one error in typeck, specifying "suggestion" as the attribute for the only suggestion is unnecessary, it's the default of the derive.
- The documentation comment for the `SessionDiagnostic` derive is out-of-date, it should have been updated in rust-lang#95512.

r? `@oli-obk`
@est31
Copy link
Member

est31 commented Jul 9, 2022

Can the approach be changed to maybe auto-generate the ftl file or something? Because I really like the experience of grepping for error messages and then directly getting to the code that creates the error. Now I'd have to figure out a fluent identifier, then I'd have to replace the -s with _...

@est31
Copy link
Member

est31 commented Jul 9, 2022

Furthermore, it also makes contributor experience worse because you now have to edit one more file to add a new error/lint/etc.

Sorry for posting to this pull request but I couldn't find any issue/rfc/etc linked, and wouldn't know how to get the attention of / reach the stakeholders otherwise.

@est31
Copy link
Member

est31 commented Jul 9, 2022

I've found something that looks like it could be the "canonical" issue for this: rust-lang/community-localization#4

It also looks like that initial plan included keeping the english error strings inside the rust source code.

@davidtwco
Copy link
Member Author

@est31 Let's chat about this in #t-compiler/wg-diagnostics :)

@Manishearth
Copy link
Member

Yeah, that was the initial plan, but tbh it's getting more and more rare to do in-source l10n strings. Sure it makes grepping a bit annoying but it really just means you have to grep twice instead of once.

@davidtwco davidtwco added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.