Migrate rustc_codegen_llvm to SessionDiagnostics #101005

SLASHLogin · 2022-08-25T14:47:23Z

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

@rustbot label +A-translation

rust-highfive · 2022-08-25T14:47:28Z

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 · 2022-08-25T15:18:21Z

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

davidtwco

This is a great start!

compiler/rustc_codegen_llvm/src/errors.rs

bors · 2022-08-27T06:59:30Z

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

SLASHLogin · 2022-08-30T10:53:35Z

@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 · 2022-08-30T14:37:16Z

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 · 2022-08-30T14:38:50Z

compiler/rustc_codegen_llvm/src/attributes.rs

+        cx.tcx
+            .sess
+            .create_err(TargetFeatureDisableOrEnable { features: f, span: Some(span) })
+            .subdiagnostic(MissingFeatures)


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

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

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

compiler/rustc_codegen_llvm/src/context.rs

Noratrieb · 2022-10-23T11:53:35Z

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 · 2022-10-30T19:36:34Z

@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

    
           impl Translate for SharedEmitter { 
        
               fn fluent_bundle(&self) -> Option<&Lrc<rustc_errors::FluentBundle>> { 
        
                   None 
        
               } 
        
               fn fallback_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 · 2022-10-31T11:33:05Z

rustc_error_messages was changed

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

davidtwco · 2022-10-31T14:15:01Z

@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 · 2022-10-31T20:08:10Z

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 · 2022-11-02T14:05:03Z

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.

compiler/rustc_codegen_ssa/src/back/write.rs

Co-authored-by: David Wood <[email protected]>

…Style)>`

SLASHLogin · 2022-11-09T14:01:20Z

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

SLASHLogin · 2022-11-09T14:59:47Z

@bors r=davidtwco

bors · 2022-11-09T14:59:49Z

@SLASHLogin: 🔑 Insufficient privileges: Not in reviewers

SLASHLogin · 2022-11-09T15:00:04Z

r? davidtwco

rustbot · 2022-11-09T15:00:07Z

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 · 2022-11-09T15:53:13Z

@bors r+

bors · 2022-11-09T15:53:15Z

📌 Commit caada74 has been approved by davidtwco

It is now in the queue for this repository.

…nostics, r=davidtwco Migrate rustc_codegen_llvm to SessionDiagnostics WIP: Port current implementation of diagnostics to the new SessionDiagnostics. Part of rust-lang#100717 `@rustbot` label +A-translation

…nostics, r=davidtwco Migrate rustc_codegen_llvm to SessionDiagnostics WIP: Port current implementation of diagnostics to the new SessionDiagnostics. Part of rust-lang#100717 ``@rustbot`` label +A-translation

…earth Rollup of 9 pull requests Successful merges: - rust-lang#101005 (Migrate rustc_codegen_llvm to SessionDiagnostics) - rust-lang#103307 (Add context to compiler error message) - rust-lang#103464 (Add support for custom mir) - rust-lang#103929 (Cleanup Apple-related code in rustc_target) - rust-lang#104015 (Remove linuxkernel targets) - rust-lang#104020 (Limit efiapi calling convention to supported arches) - rust-lang#104156 (Cleanups in autoderef impl) - rust-lang#104171 (Update books) - rust-lang#104184 (Fix `rustdoc --version` when used with download-rustc) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

bors · 2022-11-10T02:05:22Z

⌛ Testing commit caada74 with merge 34115d0...

rust-highfive assigned oli-obk Aug 25, 2022

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 Aug 25, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2022

rust-highfive assigned davidtwco and unassigned oli-obk Aug 25, 2022

davidtwco reviewed Aug 25, 2022

View reviewed changes

compiler/rustc_codegen_llvm/src/errors.rs Outdated Show resolved Hide resolved

davidtwco mentioned this pull request Aug 25, 2022

Diagnostic Translation #100717

Open

84 tasks

SLASHLogin force-pushed the rustc_codegen_llvm_diagnostics branch from 773d3c8 to 6b8539a Compare August 26, 2022 21:27

This comment has been minimized.

Sign in to view

ellishg mentioned this pull request Aug 27, 2022

Migrate rustc_codegen_gcc to SessionDiagnostics #101075

Merged

SLASHLogin force-pushed the rustc_codegen_llvm_diagnostics branch from fc3bcde to 79ff769 Compare August 29, 2022 14:04

This comment has been minimized.

Sign in to view

davidtwco reviewed Aug 30, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

SLASHLogin force-pushed the rustc_codegen_llvm_diagnostics branch from 2227d51 to 48758e7 Compare October 30, 2022 13:38

This comment has been minimized.

Sign in to view

SLASHLogin marked this pull request as ready for review October 31, 2022 11:33

davidtwco reviewed Nov 3, 2022

View reviewed changes

compiler/rustc_codegen_ssa/src/back/write.rs Outdated Show resolved Hide resolved

SLASHLogin and others added 6 commits November 9, 2022 14:56

Delay diagnostic translation in rustc_codegen_ssa

b4820a3

Add replace_args method for rustc_errors::diagnostic::Diagnostic

3b949eb

Formatting

0381e51

Use LayoutError's implementation of IntoDiagnostic

a8a8055

Update compiler/rustc_codegen_llvm/src/back/archive.rs

0baac88

Co-authored-by: David Wood <[email protected]>

Add constructor for Diagnostic that takes `Vec<(DiagnosticMessage, …

39895b0

…Style)>`

SLASHLogin force-pushed the rustc_codegen_llvm_diagnostics branch from eb829d6 to 39895b0 Compare November 9, 2022 13:58

Add missing emitted_at to the Diagnostic

caada74

davidtwco approved these changes Nov 9, 2022

View reviewed changes

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 Nov 9, 2022

Manishearth mentioned this pull request Nov 9, 2022

Rollup of 12 pull requests #104207

Closed

Manishearth mentioned this pull request Nov 9, 2022

Rollup of 9 pull requests #104208

Closed

Manishearth mentioned this pull request Nov 9, 2022

Rollup of 9 pull requests #104215

Merged

bors merged commit 2206267 into rust-lang:master Nov 10, 2022

rustbot added this to the 1.67.0 milestone Nov 10, 2022

SLASHLogin deleted the rustc_codegen_llvm_diagnostics branch November 10, 2022 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate rustc_codegen_llvm to SessionDiagnostics #101005

Migrate rustc_codegen_llvm to SessionDiagnostics #101005

SLASHLogin commented Aug 25, 2022

rust-highfive commented Aug 25, 2022

oli-obk commented Aug 25, 2022

davidtwco left a comment

This comment has been minimized.

bors commented Aug 27, 2022

This comment has been minimized.

SLASHLogin commented Aug 30, 2022

davidtwco commented Aug 30, 2022 •

edited

Loading

davidtwco left a comment

davidtwco Aug 30, 2022

SLASHLogin Aug 30, 2022

SLASHLogin Aug 30, 2022

This comment was marked as resolved.

Noratrieb commented Oct 23, 2022

This comment has been minimized.

SLASHLogin commented Oct 30, 2022

rustbot commented Oct 31, 2022

davidtwco commented Oct 31, 2022

SLASHLogin commented Oct 31, 2022

davidtwco commented Nov 2, 2022

SLASHLogin commented Nov 9, 2022

SLASHLogin commented Nov 9, 2022

bors commented Nov 9, 2022

SLASHLogin commented Nov 9, 2022

rustbot commented Nov 9, 2022

davidtwco commented Nov 9, 2022

bors commented Nov 9, 2022

bors commented Nov 10, 2022

Migrate rustc_codegen_llvm to SessionDiagnostics #101005

Migrate rustc_codegen_llvm to SessionDiagnostics #101005

Conversation

SLASHLogin commented Aug 25, 2022

rust-highfive commented Aug 25, 2022

oli-obk commented Aug 25, 2022

davidtwco left a comment

Choose a reason for hiding this comment

This comment has been minimized.

bors commented Aug 27, 2022

This comment has been minimized.

SLASHLogin commented Aug 30, 2022

davidtwco commented Aug 30, 2022 • edited Loading

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco Aug 30, 2022

Choose a reason for hiding this comment

SLASHLogin Aug 30, 2022

Choose a reason for hiding this comment

SLASHLogin Aug 30, 2022

Choose a reason for hiding this comment

This comment was marked as resolved.

Noratrieb commented Oct 23, 2022

This comment has been minimized.

SLASHLogin commented Oct 30, 2022

rustbot commented Oct 31, 2022

davidtwco commented Oct 31, 2022

SLASHLogin commented Oct 31, 2022

davidtwco commented Nov 2, 2022

SLASHLogin commented Nov 9, 2022

SLASHLogin commented Nov 9, 2022

bors commented Nov 9, 2022

SLASHLogin commented Nov 9, 2022

rustbot commented Nov 9, 2022

davidtwco commented Nov 9, 2022

bors commented Nov 9, 2022

bors commented Nov 10, 2022

davidtwco commented Aug 30, 2022 •

edited

Loading