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

Migrate codegen_ssa to diagnostics structs - [Part 1] #102612

Merged

Conversation

JhonnyBillM
Copy link
Contributor

Initial migration of codegen_ssa. Going to split this crate migration in at least two PRs in order to avoid a huge PR and to quick off some questions around:

  1. Translating messages from "external" crates.
  2. Interfacing with OS messages.
  3. Adding UI tests while migrating diagnostics.

See comments below.

@rustbot rustbot added 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. labels Oct 3, 2022
@rust-highfive
Copy link
Collaborator

r? @eholk

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2022

rustc_error_messages was changed

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 3, 2022
@JhonnyBillM JhonnyBillM changed the title Migrate codegen_ssa to diagnostics structs _[Part 1]_ Migrate codegen_ssa to diagnostics structs - [Part 1] Oct 3, 2022
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

This is a great start, thanks!

compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/errors.rs Show resolved Hide resolved
compiler/rustc_error_messages/src/lib.rs Outdated Show resolved Hide resolved
@eholk
Copy link
Contributor

eholk commented Oct 3, 2022

Looks good to me, but I'll hand it off to @davidtwco to make sure he feels his comments are adequately addressed.

@bors r? @davidtwco

@rust-highfive rust-highfive assigned davidtwco and unassigned eholk Oct 3, 2022
@JhonnyBillM JhonnyBillM force-pushed the migrate-codegen-ssa-to-diagnostics-structs branch from 8d94cda to 954a62f Compare October 4, 2022 02:10
@bors

This comment was marked as resolved.

@JhonnyBillM JhonnyBillM force-pushed the migrate-codegen-ssa-to-diagnostics-structs branch from 7d5f50f to f69f1b0 Compare October 4, 2022 17:42
@JhonnyBillM JhonnyBillM requested a review from davidtwco October 4, 2022 17:47
@JhonnyBillM JhonnyBillM force-pushed the migrate-codegen-ssa-to-diagnostics-structs branch from f69f1b0 to 41ab94c Compare October 5, 2022 12:56
@JhonnyBillM JhonnyBillM requested a review from davidtwco October 5, 2022 12:57
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 6, 2022

📌 Commit 41ab94cb00f0e08590660260d20d4ba534042a88 has been approved by davidtwco

It is now in the queue for this repository.

@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 Oct 6, 2022
@bors

This comment was marked as resolved.

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 7, 2022
@JhonnyBillM JhonnyBillM force-pushed the migrate-codegen-ssa-to-diagnostics-structs branch from 41ab94c to 13d4f27 Compare October 7, 2022 14:04
@JhonnyBillM
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 7, 2022
@JhonnyBillM JhonnyBillM requested a review from davidtwco October 7, 2022 14:14
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 10, 2022

📌 Commit 13d4f27 has been approved by davidtwco

It is now in the queue for this repository.

@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 Oct 10, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 11, 2022
…-diagnostics-structs, r=davidtwco

Migrate `codegen_ssa` to diagnostics structs - [Part 1]

Initial migration of `codegen_ssa`. Going to split this crate migration in at least two PRs in order to avoid a huge PR and to quick off some questions around:

1. Translating messages from "external" crates.
2. Interfacing with OS messages.
3. Adding UI tests while migrating diagnostics.

_See comments below._
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 11, 2022
…-diagnostics-structs, r=davidtwco

Migrate `codegen_ssa` to diagnostics structs - [Part 1]

Initial migration of `codegen_ssa`. Going to split this crate migration in at least two PRs in order to avoid a huge PR and to quick off some questions around:

1. Translating messages from "external" crates.
2. Interfacing with OS messages.
3. Adding UI tests while migrating diagnostics.

_See comments below._
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2022
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#100387 (Check uniqueness of impl items by trait item when applicable.)
 - rust-lang#101727 (Stabilize map_first_last)
 - rust-lang#101774 (Warn about safety of `fetch_update`)
 - rust-lang#102227 (fs::get_path solarish version.)
 - rust-lang#102445 (Add `is_empty()` method to `core::ffi::CStr`.)
 - rust-lang#102612 (Migrate `codegen_ssa` to diagnostics structs - [Part 1])
 - rust-lang#102685 (Interpret EH actions properly)
 - rust-lang#102869 (Add basename and dirname aliases)
 - rust-lang#102889 (rustc_hir: Less error-prone methods for accessing `PartialRes` resolution)
 - rust-lang#102893 (Fix ICE rust-lang#102878)
 - rust-lang#102912 (:arrow_up: rust-analyzer)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 24722e8 into rust-lang:master Oct 11, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 11, 2022
@JhonnyBillM JhonnyBillM deleted the migrate-codegen-ssa-to-diagnostics-structs branch October 12, 2022 15:31
@davidtwco davidtwco mentioned this pull request Nov 8, 2022
84 tasks
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…-diagnostics-structs, r=davidtwco

Migrate `codegen_ssa` to diagnostics structs - [Part 1]

Initial migration of `codegen_ssa`. Going to split this crate migration in at least two PRs in order to avoid a huge PR and to quick off some questions around:

1. Translating messages from "external" crates.
2. Interfacing with OS messages.
3. Adding UI tests while migrating diagnostics.

_See comments below._
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#100387 (Check uniqueness of impl items by trait item when applicable.)
 - rust-lang#101727 (Stabilize map_first_last)
 - rust-lang#101774 (Warn about safety of `fetch_update`)
 - rust-lang#102227 (fs::get_path solarish version.)
 - rust-lang#102445 (Add `is_empty()` method to `core::ffi::CStr`.)
 - rust-lang#102612 (Migrate `codegen_ssa` to diagnostics structs - [Part 1])
 - rust-lang#102685 (Interpret EH actions properly)
 - rust-lang#102869 (Add basename and dirname aliases)
 - rust-lang#102889 (rustc_hir: Less error-prone methods for accessing `PartialRes` resolution)
 - rust-lang#102893 (Fix ICE rust-lang#102878)
 - rust-lang#102912 (:arrow_up: rust-analyzer)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Feb 21, 2023
…ftl, r=oli-obk

errors: generate typed identifiers in each crate

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 rust-lang#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
- [x] 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.
- [x] Update `triagebot.toml` to track the moved FTL files.

r? `@compiler-errors`
cc rust-lang#100717
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2023
…l, r=oli-obk

errors: generate typed identifiers in each crate

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 rust-lang#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
- [x] 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.
- [x] Update `triagebot.toml` to track the moved FTL files.

r? `@compiler-errors`
cc rust-lang#100717
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants