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: only eagerly translate subdiagnostics #121085

Merged
merged 1 commit into from
Feb 17, 2024

Conversation

davidtwco
Copy link
Member

Subdiagnostics don't need to be lazily translated, they can always be eagerly translated. Eager translation is slightly more complex as we need to have a DiagCtxt available to perform the translation, which involves slightly more threading of that context.

This slight increase in complexity should enable later simplifications - like passing DiagCtxt into AddToDiagnostic and moving Fluent messages into the diagnostic structs rather than having them in separate files (working on that was what led to this change).

r? @nnethercote

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 14, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2024

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2024

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

rustc_errors::translation was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

fn decorate(mut self, dcx: &DiagCtxtInner) -> Diagnostic {
// FIXME: Cannot use `Diagnostic::subdiagnostic` which takes `DiagCtxt`, but it
// just uses `DiagCtxtInner` functions.
let subdiag_with = |diag: &mut Diagnostic, msg| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you pull this closure out further and use it for the FIXME cases above?

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 not sure I understand what that would look like.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you pulled this closure out into a method on DiagCtxtInner, then you could use it in flush_delayed, where the FIXME comment is.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a little tricky to do this. I can't make the closure a method because the signature wouldn't match (add_to_diagnostic_with doesn't expect a &Self), it can't be a function which doesn't take self because it needs the reference to DiagCtxtInner (which we capture currently), and returning a closure that does match from this function is tricky because the closure captures the lifetime. I'm hoping to be able to remove the closure here in a follow-up anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

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

This seems mostly good, but it feels like it's missing a couple of things.

  • Should DiagnosticMessage::Eager and SubdiagnosticMessage::Eager be renamed? Also, the comments on those variants talk about eager vs. non-eager translation, and should be updated.
  • Can AddToDiagnostic be simplified? Is add_to_diagnostic useful any more? Maybe add_to_diagnostic_with can be removed and add_to_diagnostic always do what that subdiag_with closure does (though that requires a dcx).

@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2024

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

@davidtwco
Copy link
Member Author

  • Should DiagnosticMessage::Eager and SubdiagnosticMessage::Eager be renamed? Also, the comments on those variants talk about eager vs. non-eager translation, and should be updated.

Fixed!

  • Can AddToDiagnostic be simplified? Is add_to_diagnostic useful any more? Maybe add_to_diagnostic_with can be removed and add_to_diagnostic always do what that subdiag_with closure does (though that requires a dcx).

I intend to do this as a follow-up, this is just the first part of larger changes I'm making to move Fluent messages into the structs and out of the ftl files, to merge IntoDiagnostic, AddToDiagnostic and LintDiagnostic together, and things like that.

@nnethercote
Copy link
Contributor

AFAICT you've updated DiagnosticMessage, but not SubdiagnosticMessage.

Subdiagnostics don't need to be lazily translated, they can always be
eagerly translated. Eager translation is slightly more complex as we need
to have a `DiagCtxt` available to perform the translation, which involves
slightly more threading of that context.

This slight increase in complexity should enable later simplifications -
like passing `DiagCtxt` into `AddToDiagnostic` and moving Fluent messages
into the diagnostic structs rather than having them in separate files
(working on that was what led to this change).

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco
Copy link
Member Author

AFAICT you've updated DiagnosticMessage, but not SubdiagnosticMessage.

🤦 fixed!

@nnethercote
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 15, 2024

📌 Commit b80fc5d has been approved by nnethercote

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 Feb 15, 2024
oli-obk added a commit to oli-obk/rust that referenced this pull request Feb 15, 2024
…, r=nnethercote

errors: only eagerly translate subdiagnostics

Subdiagnostics don't need to be lazily translated, they can always be eagerly translated. Eager translation is slightly more complex as we need to have a `DiagCtxt` available to perform the translation, which involves slightly more threading of that context.

This slight increase in complexity should enable later simplifications - like passing `DiagCtxt` into `AddToDiagnostic` and moving Fluent messages into the diagnostic structs rather than having them in separate files (working on that was what led to this change).

r? `@nnethercote`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 17, 2024
…, r=nnethercote

errors: only eagerly translate subdiagnostics

Subdiagnostics don't need to be lazily translated, they can always be eagerly translated. Eager translation is slightly more complex as we need to have a `DiagCtxt` available to perform the translation, which involves slightly more threading of that context.

This slight increase in complexity should enable later simplifications - like passing `DiagCtxt` into `AddToDiagnostic` and moving Fluent messages into the diagnostic structs rather than having them in separate files (working on that was what led to this change).

r? `@nnethercote`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 17, 2024
…, r=nnethercote

errors: only eagerly translate subdiagnostics

Subdiagnostics don't need to be lazily translated, they can always be eagerly translated. Eager translation is slightly more complex as we need to have a `DiagCtxt` available to perform the translation, which involves slightly more threading of that context.

This slight increase in complexity should enable later simplifications - like passing `DiagCtxt` into `AddToDiagnostic` and moving Fluent messages into the diagnostic structs rather than having them in separate files (working on that was what led to this change).

r? ``@nnethercote``
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#120952 (Don't use mem::zeroed in vec::IntoIter)
 - rust-lang#121079 (distribute tool documentations and avoid file conflicts on `x install`)
 - rust-lang#121085 (errors: only eagerly translate subdiagnostics)
 - rust-lang#121091 (use build.rustc config and skip-stage0-validation flag)
 - rust-lang#121193 (Use fulfillment in next trait solver coherence)
 - rust-lang#121209 (Make `CodegenBackend::join_codegen` infallible.)
 - rust-lang#121210 (Fix `cfg(target_abi = "sim")` on `i386-apple-ios`)
 - rust-lang#121228 (create stamp file for clippy)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#120952 (Don't use mem::zeroed in vec::IntoIter)
 - rust-lang#121085 (errors: only eagerly translate subdiagnostics)
 - rust-lang#121091 (use build.rustc config and skip-stage0-validation flag)
 - rust-lang#121149 (Fix typo in VecDeque::handle_capacity_increase() doc comment.)
 - rust-lang#121193 (Use fulfillment in next trait solver coherence)
 - rust-lang#121209 (Make `CodegenBackend::join_codegen` infallible.)
 - rust-lang#121210 (Fix `cfg(target_abi = "sim")` on `i386-apple-ios`)
 - rust-lang#121228 (create stamp file for clippy)
 - rust-lang#121231 (remove a couple of redundant clones)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 45d5773 into rust-lang:master Feb 17, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 17, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2024
Rollup merge of rust-lang#121085 - davidtwco:always-eager-diagnostics, r=nnethercote

errors: only eagerly translate subdiagnostics

Subdiagnostics don't need to be lazily translated, they can always be eagerly translated. Eager translation is slightly more complex as we need to have a `DiagCtxt` available to perform the translation, which involves slightly more threading of that context.

This slight increase in complexity should enable later simplifications - like passing `DiagCtxt` into `AddToDiagnostic` and moving Fluent messages into the diagnostic structs rather than having them in separate files (working on that was what led to this change).

r? ```@nnethercote```
Comment on lines +44 to +53

// Override `translate_message` for the silent emitter because eager translation of
// subdiagnostics result in a call to this.
fn translate_message<'a>(
&'a self,
message: &'a rustc_errors::DiagnosticMessage,
_: &'a rustc_errors::translation::FluentArgs<'_>,
) -> Result<Cow<'_, str>, rustc_errors::error::TranslateError<'_>> {
rustc_errors::emitter::silent_translate(message)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't got confirmation from the user yet if they're using a recent nightly version of rustfmt, but we just got a report about the SilentEmitter in rust-lang/rustfmt#6082 and this PR is the only one I can think of that made changes recently. @davidtwco would you mind looking into this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@davidtwco @nnethercote I got confirmation from the user who reported the issue that this started happening after they installed the latest nightly. Also, they helped to provide this minimal input snippet, which I've also tested and can confirm that it causes a panic when running rustfmt +nightly-2024-02-18

macro_rules! test {
    ($T:ident, $b:lifetime) => {
        Box<$T<$b>>
    };
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this PR is almost certainly the cause. I assume @davidtwco will look into it soon.

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 working on this in #121301.

Copy link
Contributor

Choose a reason for hiding this comment

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

@davidtwco thank you for the help on this one 🙏🏼

@davidtwco davidtwco deleted the always-eager-diagnostics branch February 19, 2024 13:10
GuillaumeGomez pushed a commit to GuillaumeGomez/rust that referenced this pull request Feb 21, 2024
…, r=nnethercote

errors: only eagerly translate subdiagnostics

Subdiagnostics don't need to be lazily translated, they can always be eagerly translated. Eager translation is slightly more complex as we need to have a `DiagCtxt` available to perform the translation, which involves slightly more threading of that context.

This slight increase in complexity should enable later simplifications - like passing `DiagCtxt` into `AddToDiagnostic` and moving Fluent messages into the diagnostic structs rather than having them in separate files (working on that was what led to this change).

r? ```@nnethercote```
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jun 22, 2024
…, r=nnethercote

errors: only eagerly translate subdiagnostics

Subdiagnostics don't need to be lazily translated, they can always be eagerly translated. Eager translation is slightly more complex as we need to have a `DiagCtxt` available to perform the translation, which involves slightly more threading of that context.

This slight increase in complexity should enable later simplifications - like passing `DiagCtxt` into `AddToDiagnostic` and moving Fluent messages into the diagnostic structs rather than having them in separate files (working on that was what led to this change).

r? ```@nnethercote```
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.

6 participants