Skip to content

Commit

Permalink
Auto merge of rust-lang#101986 - WaffleLapkin:move_lint_note_to_the_b…
Browse files Browse the repository at this point in the history
…ottom, 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 rust-lang#87678 <rust-lang#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 😢
  • Loading branch information
bors committed Oct 1, 2022
2 parents 277bb66 + b5b3ffe commit 744e397
Show file tree
Hide file tree
Showing 827 changed files with 3,369 additions and 3,209 deletions.
6 changes: 3 additions & 3 deletions compiler/rustc_const_eval/src/const_eval/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 11 additions & 0 deletions compiler/rustc_error_messages/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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]
Expand Down
24 changes: 0 additions & 24 deletions compiler/rustc_errors/src/diagnostic_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}
4 changes: 2 additions & 2 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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.
Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_hir_analysis/src/astconv/generics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
}

Expand Down
55 changes: 30 additions & 25 deletions compiler/rustc_hir_analysis/src/astconv/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down Expand Up @@ -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
},
);
}
Expand Down
77 changes: 41 additions & 36 deletions compiler/rustc_hir_analysis/src/check/cast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")]
Expand Down Expand Up @@ -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
},
);
}
Expand All @@ -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();
Expand All @@ -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
},
);
}
Expand All @@ -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
},
);
}
Expand Down
18 changes: 11 additions & 7 deletions compiler/rustc_hir_analysis/src/check/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
}
}

Expand Down Expand Up @@ -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();
},
);
}
Expand Down Expand Up @@ -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();
},
)
}
Expand Down
Loading

0 comments on commit 744e397

Please sign in to comment.