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

errors: generate typed identifiers in each crate #103042

Merged
merged 3 commits into from
Feb 22, 2023

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Oct 14, 2022

Instead of loading the Fluent resources for every crate in rustc_error_messages, each crate generates typed identifiers for its own diagnostics and creates a static which are pulled together in the rustc_driver crate and provided to the diagnostic emitter.

There are advantages and disadvantages to this change..

Advantages

  • Changing a diagnostic now only recompiles the crate for that diagnostic and those crates that depend on it, rather than rustc_error_messages and all crates thereafter.
  • This approach can be used to support first-party crates that want to supply translatable diagnostics (e.g. rust-lang/thorin in Migrate codegen_ssa to diagnostics structs - [Part 1] #102612 (comment), cc @JhonnyBillM)
  • We can extend this a little so that tools built using rustc internals (like clippy or rustdoc) can add their own diagnostic resources (much more easily than those resources needing to be available to rustc_error_messages)

Disadvantages

  • Crates can only refer to the diagnostic messages defined in the current crate (or those from dependencies), rather than all diagnostic messages.
  • rustc_driver (or some other crate we create for this purpose) has to directly depend on everything that has error messages.
    • It already transitively depended on all these crates.

Pending work

  • I don't know how to make rustc_codegen_gcc's translated diagnostics work with this approach - because rustc_driver can't depend on that crate and so can't get its resources to provide to the diagnostic emission. I don't really know how the alternative codegen backends are actually wired up to the compiler at all.
  • Update triagebot.toml to track the moved FTL files.

r? @compiler-errors
cc #100717

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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. labels Oct 14, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 14, 2022
@davidtwco davidtwco mentioned this pull request Oct 14, 2022
84 tasks
@rust-log-analyzer

This comment was marked as resolved.

@davidtwco davidtwco force-pushed the translation-distributed-ftl branch from c87a9c0 to c5bb5ee Compare October 14, 2022 14:13
@rust-log-analyzer

This comment was marked as resolved.

Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

I wonder if it would be possible to collect all the locale files into one directory in compiler/, maybe compiler/locales. Then the translation tool could be pointed at that, and you would only have one directory for each individual language, instead of one per crate. It would also help with ignoring the ftl files during a grep.

Edit: the "ftl contained in crate" use case is still valuable IMO e.g. for outside projects like custom codegen backends or clippy. But for the large majority of cases, the ftl files would reside in one directory.

Also I wonder if instead of extending the driver, one could add another crate that collects all the translations, and have rustc_driver depend on that. Not sure about this one tho. Thoughts?

compiler/rustc_errors/src/json/tests.rs Outdated Show resolved Hide resolved
@bors

This comment was marked as resolved.

@davidtwco davidtwco force-pushed the translation-distributed-ftl branch from c5bb5ee to 0d307dc Compare October 17, 2022 08:39
@davidtwco
Copy link
Member Author

I wonder if it would be possible to collect all the locale files into one directory in compiler/, maybe compiler/locales. Then the translation tool could be pointed at that, and you would only have one directory for each individual language, instead of one per crate. It would also help with ignoring the ftl files during a grep.

I don't have a particularly strong preference here. I don't know if there is a significant advantage to this. I haven't looked into how the translation tooling works to know whether a single directory will be easier or not, but it is very likely that only the original English will be in this repository regardless.

Also I wonder if instead of extending the driver, one could add another crate that collects all the translations, and have rustc_driver depend on that. Not sure about this one tho. Thoughts?

We could do this, it's really only an aesthetic consideration, so I don't have a strong opinion.

In both cases, will wait for others to weigh in.

@est31
Copy link
Member

est31 commented Oct 17, 2022

I don't really know how the alternative codegen backends are actually wired up to the compiler at all.

I've looked at rustc_codegen_gcc and it seems to use the CodegenBackend trait through the codegen-backend feature. It seems that rustc_driver calls a function to generate a boxed trait object implementing that trait. Maybe that trait could be extended? cc @bjorn3

it is very likely that only the original English will be in this repository regardless.

Oh that changes a few things. In that case, the current state is okay. I wonder about the extra directory level, but that's only a papercut, as the disadvantage of it is quite minor (extra click in many IDEs). Some people might in fact prefer the locale/ dir over a raw file due to stylistic concerns.

@rust-log-analyzer

This comment has been minimized.

@davidtwco davidtwco force-pushed the translation-distributed-ftl branch from 0d307dc to 0198466 Compare October 17, 2022 10:42
@rust-log-analyzer

This comment has been minimized.

@davidtwco davidtwco force-pushed the translation-distributed-ftl branch from e9f4a47 to 9ae650e Compare October 17, 2022 14:03
@rust-log-analyzer

This comment has been minimized.

@davidtwco davidtwco force-pushed the translation-distributed-ftl branch from 9ae650e to fc34a0a Compare October 17, 2022 14:53
@bors

This comment was marked as resolved.

@davidtwco davidtwco force-pushed the translation-distributed-ftl branch from fc34a0a to c8d990b Compare October 18, 2022 08:45
@davidtwco davidtwco force-pushed the translation-distributed-ftl branch 2 times, most recently from 6552aa0 to e19f06b Compare October 18, 2022 10:37
@bors

This comment was marked as resolved.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b869e84): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.1% [1.0%, 1.2%] 2
Regressions ❌
(secondary)
2.2% [2.0%, 2.6%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.1% [1.0%, 1.2%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.1% [3.1%, 3.1%] 1
Regressions ❌
(secondary)
2.4% [1.2%, 3.0%] 8
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.8% [-5.1%, -2.5%] 2
All ❌✅ (primary) 3.1% [3.1%, 3.1%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
5.8% [4.2%, 7.5%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

@rustbot rustbot added the perf-regression Performance regression. label Feb 22, 2023
@nnethercote
Copy link
Contributor

Probably noise, keccak and cranelift-codegen are unreliable at the moment :/

@rustbot label: +perf-regression-triaged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-testsuite Area: The testsuite used to check the correctness of rustc A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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.