Migrate rustc_codegen_llvm to SessionDiagnostics

[SLASHLogin]

WIP: Port current implementation of diagnostics to the new SessionDiagnostics.

Part of #100717

@rustbot label +A-translation

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @oli-obk (or someone else) soon.

Please see the contribution instructions for more information.

[oli-obk]

r? @davidtwco (I'm still catching up)

[davidtwco]

This is a great start!

[bors]

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

[SLASHLogin]

@davidtwco Two last things that block this are those two calls to the Handler. And I've found one method that makes use of the Handler too.
https://github.com/rust-lang/rust/blob/2227d5117d0d8fe9d897bb3a2d020e75bb3edad5/compiler/rustc_codegen_llvm/src/lib.rs#L420-L422
https://github.com/rust-lang/rust/blob/2227d5117d0d8fe9d897bb3a2d020e75bb3edad5/compiler/rustc_codegen_llvm/src/back/lto.rs#L79-L85
https://github.com/rust-lang/rust/blob/2227d5117d0d8fe9d897bb3a2d020e75bb3edad5/compiler/rustc_codegen_llvm/src/back/write.rs#L41-L48
However, all Handlers stem from this implementation of the WriteBackendMethods trait.
https://github.com/rust-lang/rust/blob/2227d5117d0d8fe9d897bb3a2d020e75bb3edad5/compiler/rustc_codegen_llvm/src/lib.rs#L168-L171
Which is then defined in the rust_codegen_ssa.rs
https://github.com/rust-lang/rust/blob/2227d5117d0d8fe9d897bb3a2d020e75bb3edad5/compiler/rustc_codegen_ssa/src/traits/write.rs#L7-L10
What's the preferred solution in this situation?

[davidtwco]

You might just need to skip these for now, otherwise we would have to change SessionDiagnostic to use a Handler rather than a ParseSess (which might be doable).

[davidtwco]

This looks great, left a couple comments

[davidtwco]

Why can't you just add a #[help] to the type of TargetFeatureDisableOrEnable?

[SLASHLogin]

TargetFeatureDisableOrEnable is present in llvm_util.rs, but without the help. I was thinking of splitting them into separate diagnostics, and then I could just add #[help] as you proposed.
https://github.com/rust-lang/rust/blob/2227d5117d0d8fe9d897bb3a2d020e75bb3edad5/compiler/rustc_codegen_llvm/src/llvm_util.rs#L497-L502

[SLASHLogin]

Other solution could be usage of enums instead of struct here. That would boil down to the same problem as the other comment here.

[Noratrieb]

Hi, I've seen you changed some diagnostic structs in your PR. After #103345, the way we refer to fluent messages changed. They are now in a flat namespace with the same identifier as in the fluent file. For example, parser::cool_thing is now parser_cool_thing and parser::suggestion just suggestion.
You should rebase to the latest master and change your fluent message references as described above. Thanks!

[SLASHLogin]

@davidtwco I've ported remaining two diagnostics and changed old ones, which were implemented manually. However, it seems like SharedEmitter implements Translate such that it always panics, and it fails that one test.

rust/compiler/rustc_codegen_ssa/src/back/write.rs

Lines 1734 to 1742 in e96c330

	implTranslateforSharedEmitter{
	fnfluent_bundle(&self) -> Option<&Lrc<rustc_errors::FluentBundle>>{
	None
	}
	fnfallback_fluent_bundle(&self) -> &rustc_errors::FluentBundle{
	panic!("shared emitter attempted to translate a diagnostic");
	}
	}

https://github.com/rust-lang/rust/blob/3f66cd3ce40aa58404c901c16c211fff42a3f1a1/compiler/rustc_codegen_llvm/src/back/lto.rs#L93-L96

[rustbot]

rustc_error_messages was changed

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

[davidtwco]

@davidtwco I've ported remaining two diagnostics and changed old ones, which were implemented manually. However, it seems like SharedEmitter implements Translate such that it always panics, and it fails that one test.

Are you able to change rustc_codegen_ssa::back::write::Diagnostic so that the message field takes a rustc_errors::DiagnosticMessage? Then if we remove the call to translate_messages, it should be okay?

rust/compiler/rustc_codegen_ssa/src/back/write.rs

Line 1748 in e96c330

msg:self.translate_messages(&diag.message,&fluent_args).to_string(),

I guess you'll also need to keep track of the diagnostic arguments in rustc_codegen_ssa::back::write::Diagnostic too and set those when it recreates the rustc_errors::Diagnostic on the other side.

I'm assuming that diagnostics emitted from SharedEmitter get sent back to another thread where the actual emitter with the translation resources will be available.

[SLASHLogin]

I was able to change rustc_codegen_ssa::back::write::Diagnostic so that it takes Vec<(DiagnosticMessage, Style)>, just like in rustc_errors::diagnostic::Diagnostic. I'm also copying arguments from the latter one to the former. However, with this approach, I have to pass new String when recreating rustc_errors::Diagnostic. Additionally, I had to implement IntoDiagnosticArg for DiagnosticArgValue to be able to pass arguments back.
With this solution I was able to pass this UI test, but I'm failing two codegen tests though. Should I commit those changes to this branch, or should I make new PR for it?

[davidtwco]

I was able to change rustc_codegen_ssa::back::write::Diagnostic so that it takes Vec<(DiagnosticMessage, Style)>, just like in rustc_errors::diagnostic::Diagnostic. I'm also copying arguments from the latter one to the former. However, with this approach, I have to pass new String when recreating rustc_errors::Diagnostic. Additionally, I had to implement IntoDiagnosticArg for DiagnosticArgValue to be able to pass arguments back. With this solution I was able to pass this UI test, but I'm failing two codegen tests though. Should I commit those changes to this branch, or should I make new PR for it?

Feel free to commit them to this branch.

[SLASHLogin]

Sorry, I've used GitHub's feature and didn't check properly that it would do the merge commit.

[SLASHLogin]

@bors r=davidtwco

[bors]

@SLASHLogin: 🔑 Insufficient privileges: Not in reviewers

[SLASHLogin]

r? davidtwco

[rustbot]

Could not assign reviewer from: davidtwco.
User(s) davidtwco are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

[davidtwco]

@bors r+

[bors]

📌 Commit caada74 has been approved by davidtwco

It is now in the queue for this repository.

[bors]

⌛ Testing commit caada74 with merge 34115d0...