Auto merge of #101986 - WaffleLapkin:move_lint_note_to_the_bottom, r=…

…estebank

Move lint level source explanation to the bottom

So, uhhhhh

r? `@estebank`

## User-facing change

"note: `#[warn(...)]` on by default" and such are moved to the bottom of the diagnostic:
```diff
-   = note: `#[warn(unsupported_calling_conventions)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #87678 <#87678>
+   = note: `#[warn(unsupported_calling_conventions)]` on by default
```

Why warning is enabled is the least important thing, so it shouldn't be the first note the user reads, IMO.

## Developer-facing change

`struct_span_lint` and similar methods have a different signature.

Before: `..., impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>)`
After: `..., impl Into<DiagnosticMessage>, impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()>`

The reason for this is that `struct_span_lint` needs to edit the diagnostic _after_ `decorate` closure is called. This also makes lint code a little bit nicer in my opinion.

Another option is to use `impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>) -> DiagnosticBuilder<'a, ()>` altough I don't _really_ see reasons to do `let lint = lint.build(message)` everywhere.

## Subtle problem

By moving the message outside of the closure (that may not be called if the lint is disabled) `format!(...)` is executed earlier, possibly formatting `Ty` which may call a query that trims paths that crashes the compiler if there were no warnings...

I don't think it's that big of a deal, considering that we move from `format!(...)` to `fluent` (which is lazy by-default) anyway, however this required adding a workaround which is unfortunate.

## P.S.

I'm sorry, I do not how to make this PR smaller/easier to review. Changes to the lint API affect SO MUCH 😢

compiler/rustc_const_eval/src/const_eval/error.rs

@@ -233,10 +233,10 @@ impl<'tcx> ConstEvalErr<'tcx> {
                 rustc_session::lint::builtin::CONST_ERR,
                 hir_id,
                 tcx.span,
+                message,
                 |lint| {
-                    let mut lint = lint.build(message);
-                    finish(&mut lint, Some(err_msg));
-                    lint.emit();
+                    finish(lint, Some(err_msg));
+                    lint
                 },
             );
             ErrorHandled::Linted

compiler/rustc_error_messages/src/lib.rs

@@ -358,6 +358,17 @@ impl<S: Into<String>> From<S> for DiagnosticMessage {
     }
 }
 
+/// A workaround for "good path" ICEs when formatting types in disables lints.
+///
+/// Delays formatting until `.into(): DiagnosticMessage` is used.
+pub struct DelayDm<F>(pub F);
+
+impl<F: FnOnce() -> String> From<DelayDm<F>> for DiagnosticMessage {
+    fn from(DelayDm(f): DelayDm<F>) -> Self {
+        DiagnosticMessage::from(f())
+    }
+}
+
 /// Translating *into* a subdiagnostic message from a diagnostic message is a little strange - but
 /// the subdiagnostic functions (e.g. `span_label`) take a `SubdiagnosticMessage` and the
 /// subdiagnostic derive refers to typed identifiers that are `DiagnosticMessage`s, so need to be

compiler/rustc_errors/src/diagnostic.rs

@@ -1,6 +1,6 @@
 use crate::snippet::Style;
 use crate::{
-    CodeSuggestion, DiagnosticMessage, EmissionGuarantee, Level, LintDiagnosticBuilder, MultiSpan,
+    CodeSuggestion, DiagnosticBuilder, DiagnosticMessage, EmissionGuarantee, Level, MultiSpan,
     SubdiagnosticMessage, Substitution, SubstitutionPart, SuggestionStyle,
 };
 use rustc_ast as ast;
@@ -209,7 +209,12 @@ pub trait AddToDiagnostic {
 #[rustc_diagnostic_item = "DecorateLint"]
 pub trait DecorateLint<'a, G: EmissionGuarantee> {
     /// Decorate and emit a lint.
-    fn decorate_lint(self, diag: LintDiagnosticBuilder<'a, G>);
+    fn decorate_lint<'b>(
+        self,
+        diag: &'b mut DiagnosticBuilder<'a, G>,
+    ) -> &'b mut DiagnosticBuilder<'a, G>;
+
+    fn msg(&self) -> DiagnosticMessage;
 }
 
 #[must_use]

compiler/rustc_errors/src/diagnostic_builder.rs

@@ -642,27 +642,3 @@ macro_rules! struct_span_err {
 macro_rules! error_code {
     ($code:ident) => {{ $crate::DiagnosticId::Error(stringify!($code).to_owned()) }};
 }
-
-/// Wrapper around a `DiagnosticBuilder` for creating lints.
-pub struct LintDiagnosticBuilder<'a, G: EmissionGuarantee>(DiagnosticBuilder<'a, G>);
-
-impl<'a, G: EmissionGuarantee> LintDiagnosticBuilder<'a, G> {
-    #[rustc_lint_diagnostics]
-    /// Return the inner `DiagnosticBuilder`, first setting the primary message to `msg`.
-    pub fn build(mut self, msg: impl Into<DiagnosticMessage>) -> DiagnosticBuilder<'a, G> {
-        self.0.set_primary_message(msg);
-        self.0.set_is_lint();
-        self.0
-    }
-
-    /// Create a `LintDiagnosticBuilder` from some existing `DiagnosticBuilder`.
-    pub fn new(err: DiagnosticBuilder<'a, G>) -> LintDiagnosticBuilder<'a, G> {
-        LintDiagnosticBuilder(err)
-    }
-}
-
-impl<'a> LintDiagnosticBuilder<'a, ErrorGuaranteed> {
-    pub fn forget_guarantee(self) -> LintDiagnosticBuilder<'a, ()> {
-        LintDiagnosticBuilder(self.0.forget_guarantee())
-    }
-}

compiler/rustc_errors/src/lib.rs

@@ -30,7 +30,7 @@ use rustc_data_structures::stable_hasher::StableHasher;
 use rustc_data_structures::sync::{self, Lock, Lrc};
 use rustc_data_structures::AtomicRef;
 pub use rustc_error_messages::{
-    fallback_fluent_bundle, fluent, fluent_bundle, DiagnosticMessage, FluentBundle,
+    fallback_fluent_bundle, fluent, fluent_bundle, DelayDm, DiagnosticMessage, FluentBundle,
     LanguageIdentifier, LazyFallbackBundle, MultiSpan, SpanLabel, SubdiagnosticMessage,
     DEFAULT_LOCALE_RESOURCES,
 };
@@ -374,7 +374,7 @@ pub use diagnostic::{
     AddToDiagnostic, DecorateLint, Diagnostic, DiagnosticArg, DiagnosticArgFromDisplay,
     DiagnosticArgValue, DiagnosticId, DiagnosticStyledString, IntoDiagnosticArg, SubDiagnostic,
 };
-pub use diagnostic_builder::{DiagnosticBuilder, EmissionGuarantee, LintDiagnosticBuilder};
+pub use diagnostic_builder::{DiagnosticBuilder, EmissionGuarantee};
 use std::backtrace::Backtrace;
 
 /// A handler deals with errors and other compiler output.

compiler/rustc_hir_analysis/src/astconv/generics.rs

@@ -649,9 +649,8 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
                     LATE_BOUND_LIFETIME_ARGUMENTS,
                     args.args[0].hir_id(),
                     multispan,
-                    |lint| {
-                        lint.build(msg).emit();
-                    },
+                    msg,
+                    |lint| lint,
                 );
             }
 

compiler/rustc_hir_analysis/src/astconv/mod.rs

@@ -2010,30 +2010,35 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
         tcx.check_stability(item.def_id, Some(hir_ref_id), span, None);
 
         if let Some(variant_def_id) = variant_resolution {
-            tcx.struct_span_lint_hir(AMBIGUOUS_ASSOCIATED_ITEMS, hir_ref_id, span, |lint| {
-                let mut err = lint.build("ambiguous associated item");
-                let mut could_refer_to = |kind: DefKind, def_id, also| {
-                    let note_msg = format!(
-                        "`{}` could{} refer to the {} defined here",
-                        assoc_ident,
-                        also,
-                        kind.descr(def_id)
-                    );
-                    err.span_note(tcx.def_span(def_id), &note_msg);
-                };
+            tcx.struct_span_lint_hir(
+                AMBIGUOUS_ASSOCIATED_ITEMS,
+                hir_ref_id,
+                span,
+                "ambiguous associated item",
+                |lint| {
+                    let mut could_refer_to = |kind: DefKind, def_id, also| {
+                        let note_msg = format!(
+                            "`{}` could{} refer to the {} defined here",
+                            assoc_ident,
+                            also,
+                            kind.descr(def_id)
+                        );
+                        lint.span_note(tcx.def_span(def_id), &note_msg);
+                    };
 
-                could_refer_to(DefKind::Variant, variant_def_id, "");
-                could_refer_to(kind, item.def_id, " also");
+                    could_refer_to(DefKind::Variant, variant_def_id, "");
+                    could_refer_to(kind, item.def_id, " also");
 
-                err.span_suggestion(
-                    span,
-                    "use fully-qualified syntax",
-                    format!("<{} as {}>::{}", qself_ty, tcx.item_name(trait_did), assoc_ident),
-                    Applicability::MachineApplicable,
-                );
+                    lint.span_suggestion(
+                        span,
+                        "use fully-qualified syntax",
+                        format!("<{} as {}>::{}", qself_ty, tcx.item_name(trait_did), assoc_ident),
+                        Applicability::MachineApplicable,
+                    );
 
-                err.emit();
-            });
+                    lint
+                },
+            );
         }
         Ok((ty, kind, item.def_id))
     }
@@ -3079,15 +3084,15 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
                     BARE_TRAIT_OBJECTS,
                     self_ty.hir_id,
                     self_ty.span,
+                    msg,
                     |lint| {
-                        let mut diag = lint.build(msg);
-                        diag.multipart_suggestion_verbose(
+                        lint.multipart_suggestion_verbose(
                             "use `dyn`",
                             sugg,
                             Applicability::MachineApplicable,
                         );
-                        self.maybe_lint_blanket_trait_impl(&self_ty, &mut diag);
-                        diag.emit();
+                        self.maybe_lint_blanket_trait_impl(&self_ty, lint);
+                        lint
                     },
                 );
             }

compiler/rustc_hir_analysis/src/check/cast.rs

@@ -33,7 +33,7 @@ use super::FnCtxt;
 use crate::hir::def_id::DefId;
 use crate::type_error_struct;
 use hir::def_id::LOCAL_CRATE;
-use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder, ErrorGuaranteed};
+use rustc_errors::{struct_span_err, Applicability, DelayDm, DiagnosticBuilder, ErrorGuaranteed};
 use rustc_hir as hir;
 use rustc_infer::traits::{Obligation, ObligationCause, ObligationCauseCode};
 use rustc_middle::mir::Mutability;
@@ -754,19 +754,25 @@ impl<'a, 'tcx> CastCheck<'tcx> {
         } else {
             ("", lint::builtin::TRIVIAL_CASTS)
         };
-        fcx.tcx.struct_span_lint_hir(lint, self.expr.hir_id, self.span, |err| {
-            err.build(&format!(
-                "trivial {}cast: `{}` as `{}`",
-                adjective,
-                fcx.ty_to_string(t_expr),
-                fcx.ty_to_string(t_cast)
-            ))
-            .help(&format!(
-                "cast can be replaced by coercion; this might \
-                                   require {type_asc_or}a temporary variable"
-            ))
-            .emit();
-        });
+        fcx.tcx.struct_span_lint_hir(
+            lint,
+            self.expr.hir_id,
+            self.span,
+            DelayDm(|| {
+                format!(
+                    "trivial {}cast: `{}` as `{}`",
+                    adjective,
+                    fcx.ty_to_string(t_expr),
+                    fcx.ty_to_string(t_cast)
+                )
+            }),
+            |lint| {
+                lint.help(format!(
+                    "cast can be replaced by coercion; this might \
+                     require {type_asc_or}a temporary variable"
+                ))
+            },
+        );
     }
 
     #[instrument(skip(fcx), level = "debug")]
@@ -1074,12 +1080,12 @@ impl<'a, 'tcx> CastCheck<'tcx> {
                 lint::builtin::CENUM_IMPL_DROP_CAST,
                 self.expr.hir_id,
                 self.span,
-                |err| {
-                    err.build(&format!(
-                        "cannot cast enum `{}` into integer `{}` because it implements `Drop`",
-                        self.expr_ty, self.cast_ty
-                    ))
-                    .emit();
+                DelayDm(|| format!(
+                    "cannot cast enum `{}` into integer `{}` because it implements `Drop`",
+                    self.expr_ty, self.cast_ty
+                )),
+                |lint| {
+                    lint
                 },
             );
         }
@@ -1090,12 +1096,11 @@ impl<'a, 'tcx> CastCheck<'tcx> {
             lint::builtin::LOSSY_PROVENANCE_CASTS,
             self.expr.hir_id,
             self.span,
-            |err| {
-                let mut err = err.build(&format!(
+            DelayDm(|| format!(
                     "under strict provenance it is considered bad style to cast pointer `{}` to integer `{}`",
                     self.expr_ty, self.cast_ty
-                ));
-
+                )),
+            |lint| {
                 let msg = "use `.addr()` to obtain the address of a pointer";
 
                 let expr_prec = self.expr.precedence().order();
@@ -1114,22 +1119,22 @@ impl<'a, 'tcx> CastCheck<'tcx> {
                         (cast_span, format!(").addr(){scalar_cast}")),
                     ];
 
-                    err.multipart_suggestion(msg, suggestions, Applicability::MaybeIncorrect);
+                    lint.multipart_suggestion(msg, suggestions, Applicability::MaybeIncorrect);
                 } else {
-                    err.span_suggestion(
+                    lint.span_suggestion(
                         cast_span,
                         msg,
                         format!(".addr(){scalar_cast}"),
                         Applicability::MaybeIncorrect,
                     );
                 }
 
-                err.help(
+                lint.help(
                     "if you can't comply with strict provenance and need to expose the pointer \
                     provenance you can use `.expose_addr()` instead"
                 );
 
-                err.emit();
+                lint
             },
         );
     }
@@ -1139,24 +1144,24 @@ impl<'a, 'tcx> CastCheck<'tcx> {
             lint::builtin::FUZZY_PROVENANCE_CASTS,
             self.expr.hir_id,
             self.span,
-            |err| {
-                let mut err = err.build(&format!(
-                    "strict provenance disallows casting integer `{}` to pointer `{}`",
-                    self.expr_ty, self.cast_ty
-                ));
+            DelayDm(|| format!(
+                "strict provenance disallows casting integer `{}` to pointer `{}`",
+                self.expr_ty, self.cast_ty
+            )),
+            |lint| {
                 let msg = "use `.with_addr()` to adjust a valid pointer in the same allocation, to this address";
                 let suggestions = vec![
                     (self.expr_span.shrink_to_lo(), String::from("(...).with_addr(")),
                     (self.expr_span.shrink_to_hi().to(self.cast_span), String::from(")")),
                 ];
 
-                err.multipart_suggestion(msg, suggestions, Applicability::MaybeIncorrect);
-                err.help(
+                lint.multipart_suggestion(msg, suggestions, Applicability::MaybeIncorrect);
+                lint.help(
                     "if you can't comply with strict provenance and don't have a pointer with \
                     the correct provenance you can use `std::ptr::from_exposed_addr()` instead"
                  );
 
-                err.emit();
+                lint
             },
         );
     }

compiler/rustc_hir_analysis/src/check/check.rs

@@ -48,9 +48,13 @@ pub(super) fn check_abi(tcx: TyCtxt<'_>, hir_id: hir::HirId, span: Span, abi: Ab
             .emit();
         }
         None => {
-            tcx.struct_span_lint_hir(UNSUPPORTED_CALLING_CONVENTIONS, hir_id, span, |lint| {
-                lint.build("use of calling convention not supported on this target").emit();
-            });
+            tcx.struct_span_lint_hir(
+                UNSUPPORTED_CALLING_CONVENTIONS,
+                hir_id,
+                span,
+                "use of calling convention not supported on this target",
+                |lint| lint,
+            );
         }
     }
 
@@ -510,10 +514,10 @@ fn check_static_inhabited<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) {
             UNINHABITED_STATIC,
             tcx.hir().local_def_id_to_hir_id(def_id),
             span,
+            "static of uninhabited type",
             |lint| {
-                lint.build("static of uninhabited type")
+                lint
                 .note("uninhabited statics cannot be initialized, and any access would be an immediate error")
-                .emit();
             },
         );
     }
@@ -1437,17 +1441,17 @@ pub(super) fn check_transparent<'tcx>(tcx: TyCtxt<'tcx>, sp: Span, adt: ty::AdtD
                 REPR_TRANSPARENT_EXTERNAL_PRIVATE_FIELDS,
                 tcx.hir().local_def_id_to_hir_id(adt.did().expect_local()),
                 span,
+                "zero-sized fields in `repr(transparent)` cannot contain external non-exhaustive types",
                 |lint| {
                     let note = if non_exhaustive {
                         "is marked with `#[non_exhaustive]`"
                     } else {
                         "contains private fields"
                     };
                     let field_ty = tcx.def_path_str_with_substs(def_id, substs);
-                    lint.build("zero-sized fields in repr(transparent) cannot contain external non-exhaustive types")
+                    lint
                         .note(format!("this {descr} contains `{field_ty}`, which {note}, \
                             and makes it not a breaking change to become non-zero-sized in the future."))
-                        .emit();
                 },
             )
         }