Move and rename SessionDiagnostic & SessionSubdiagnostic traits and macros

[JhonnyBillM]

After PR #101434, we want to:

• Move SessionDiagnostic to rustc_errors.
• Add emit_ methods that accept impl SessionDiagnostic to Handler.
• (optional) Rename trait SessionDiagnostic to DiagnosticHandler.
• (optional) Rename macro SessionDiagnostic to DiagnosticHandler.
• Update Rustc Dev Guide and Docs to reflect these changes. [Diagnostics] - Update docs to match renamed Diagnostics traits and macros rustc-dev-guide#1460

Now I am having build issues getting the compiler to build when trying to rename the macro.

See diagnostics errors and context when building.

error: diagnostics should only be created in `SessionDiagnostic`/`AddSubdiagnostic` impls
  --> compiler/rustc_attr/src/session_diagnostics.rs:13:10
   |
13 |   #[derive(DiagnosticHandler)]
   |            ^^^^^^^^^^^^^^^^^ in this derive macro expansion
   |
  ::: /Users/jhonny/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/synstructure-0.12.6/src/macros.rs:94:9
   |
94 | /         pub fn $derives(
95 | |             i: $crate::macros::TokenStream
96 | |         ) -> $crate::macros::TokenStream {
   | |________________________________________- in this expansion of `#[derive(DiagnosticHandler)]`
   |
note: the lint level is defined here
  --> compiler/rustc_attr/src/lib.rs:10:9
   |
10 | #![deny(rustc::diagnostic_outside_of_impl)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

And also this one:

error: diagnostics should only be created in `SessionDiagnostic`/`AddSubdiagnostic` impls
   --> compiler/rustc_attr/src/session_diagnostics.rs:213:32
    |
213 |         let mut diag = handler.struct_span_err_with_code(
    |                                ^^^^^^^^^^^^^^^^^^^^^^^^^

Note
Can't find where this message is coming from, because you can see in this experimental branch that I updated all errors and diags to say:
error: diagnostics should only be created in DiagnosticHandler/AddSubdiagnostic impls
and not:
error: diagnostics should only be created in SessionDiagnostic/AddSubdiagnostic impls

I tried building the compiler in different ways (playing with the stages etc), but nothing worked.

Question

Do we need to build or do something different when renaming a macro and identifiers?

For context, see experimental commit JhonnyBillM@f2193a9 where the macro and symbols are renamed, but it doesn't compile.

[rust-highfive]

r? @fee1-dead

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

[fee1-dead]

r? @davidtwco

[davidtwco]

This is great, left some ideas for how to fix the errors you're seeing in a comment below. I think it might be good if we named the derive Diagnostic (i.e. #[derive(Diagnostic)]) rather than DiagnosticHandler. I don't mind if the trait is named DiagnosticHandler, though maybe something like IntoDiagnostic might be more appropriate.

Right now (without this PR), we have the following...

Derive	Trait
#[derive(SessionDiagnostic)]	SessionDiagnostic
#[derive(SessionSubdiagnostic)]	AddToDiagnostic
#[derive(LintDiagnostic)]	DecorateLint

But maybe it would be more uniform as..

Derive	Trait
#[derive(Diagnostic)]	IntoDiagnostic
#[derive(Subdiagnostic)]	AddToDiagnostic
#[derive(LintDiagnostic)]	DecorateLint

[rustbot]

rustc_macros::diagnostics was changed

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

rustc_error_messages was changed

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

Some changes occurred in need_type_info.rs

cc @lcnr

[davidtwco]

LGTM, r=me (w/ rollup=never as this could easily conflict with other translation PRs) but I'd like it if you could squash some of your commits a little just so there are fewer that just correct previous commits in this PR.

[JhonnyBillM]

@davidtwco Updated commit history. Now it has 1 commit per rename, plus one with updates and fixes.

Thanks for the approval, I would like to confirm:

1. Are we ok with TargetDataLayoutErrorsWrapper in:
   1.1 rustc_session/src/errors.rs:48
   1.2 rustc_middle/src/ty/context.rs:1249
   1.3 rustc_session/src/config.rs:902

2. Updated lint_diagnostic_derive in rustc_macros/src/diagnostics/mod.rs:106 to https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-structs.html#reference. Is this ok?

[davidtwco]

@davidtwco Updated commit history. Now it has 1 commit per rename, plus one with updates and fixes.

Perfect!

1. Are we ok with TargetDataLayoutErrorsWrapper in:
   1.1 rustc_session/src/errors.rs:48
   1.2 rustc_middle/src/ty/context.rs:1249
   1.3 rustc_session/src/config.rs:902

I'm not sure why this is necessary. I'd prefer we avoid the wrapper if we could.

2. Updated lint_diagnostic_derive in rustc_macros/src/diagnostics/mod.rs:106 to https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-structs.html#reference. Is this ok?

This is fine.

[JhonnyBillM]

I'm not sure why this is necessary. I'd prefer we avoid the wrapper if we could.

We need it because of Coherence/Orphan rule. It wasn't needed before because the SessionDiagnostic protocol was defined in rustc_session, now that it isn't, neither the type nor the trait are defined in the crate.

But I am not sure if the compiler codebase handles this differently. Open to suggestions

[davidtwco]

I'm not sure why this is necessary. I'd prefer we avoid the wrapper if we could.

We need it because of Coherence/Orphan rule. It wasn't needed before because the SessionDiagnostic protocol was defined in rustc_session, now that it isn't, neither the type nor the trait are defined in the crate.

But I am not sure if the compiler codebase handles this differently. Open to suggestions

I think rustc_errors depends on rustc_target for some IntoDiagnosticArg impls, so you might be able to move this impl there?

[JhonnyBillM]

@davidtwco Great point. Is it possible to move this error in a follow up PR?

Because:

1. right now rustc_errors doesn't have a fluent file.
2. we would have to move the fluent strings.
3. I think we should have a separate file for these "general IntoDiagnostic impls" in rustc_errors instead of having them all in rustc_errors/src/diagnostics.rs

this would introduce many changes unrelated to the move+rename of the traits, and another PR could be more appropriate (for example, when porting rustc_errors, or just in a general clean up task).

[davidtwco]

Sure, we can leave that to a follow-up.

@bors r+ rollup=never (likely to conflict with other translation pull requests)

[rust-timer]

Finished benchmarking commit (9062b78): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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.

	mean1	range	count2
Regressions ❌ (primary)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²