Skip to content

Commit

Permalink
Auto merge of #93368 - eddyb:diagbld-guarantee, r=estebank
Browse files Browse the repository at this point in the history
rustc_errors: let `DiagnosticBuilder::emit` return a "guarantee of emission".

That is, `DiagnosticBuilder` is now generic over the return type of `.emit()`, so we'll now have:
* `DiagnosticBuilder<ErrorReported>` for error (incl. fatal/bug) diagnostics
  * can only be created via a `const L: Level`-generic constructor, that limits allowed variants via a `where` clause, so not even `rustc_errors` can accidentally bypass this limitation
  * asserts `diagnostic.is_error()` on emission, just in case the construction restriction was bypassed (e.g. by replacing the whole `Diagnostic` inside `DiagnosticBuilder`)
  * `.emit()` returns `ErrorReported`, as a "proof" token that `.emit()` was called
    (though note that this isn't a real guarantee until after completing the work on
     #69426)
* `DiagnosticBuilder<()>` for everything else (warnings, notes, etc.)
  * can also be obtained from other `DiagnosticBuilder`s by calling `.forget_guarantee()`

This PR is a companion to other ongoing work, namely:
* #69426
  and it's ongoing implementation:
  #93222
  the API changes in this PR are needed to get statically-checked "only errors produce `ErrorReported` from `.emit()`", but doesn't itself provide any really strong guarantees without those other `ErrorReported` changes
* #93244
  would make the choices of API changes (esp. naming) in this PR fit better overall

In order to be able to let `.emit()` return anything trustable, several changes had to be made:
* `Diagnostic`'s `level` field is now private to `rustc_errors`, to disallow arbitrary "downgrade"s from "some kind of error" to "warning" (or anything else that doesn't cause compilation to fail)
  * it's still possible to replace the whole `Diagnostic` inside the `DiagnosticBuilder`, sadly, that's harder to fix, but it's unlikely enough that we can paper over it with asserts on `.emit()`
* `.cancel()` now consumes `DiagnosticBuilder`, preventing `.emit()` calls on a cancelled diagnostic
  * it's also now done internally, through `DiagnosticBuilder`-private state, instead of having a `Level::Cancelled` variant that can be read (or worse, written) by the user
  * this removes a hazard of calling `.cancel()` on an error then continuing to attach details to it, and even expect to be able to `.emit()` it
  * warnings were switched to *only* `can_emit_warnings` on emission (instead of pre-cancelling early)
  * `struct_dummy` was removed (as it relied on a pre-`Cancelled` `Diagnostic`)
* since `.emit()` doesn't consume the `DiagnosticBuilder` <sub>(I tried and gave up, it's much more work than this PR)</sub>,
  we have to make `.emit()` idempotent wrt the guarantees it returns
  * thankfully, `err.emit(); err.emit();` can return `ErrorReported` both times, as the second `.emit()` call has no side-effects *only* because the first one did do the appropriate emission
* `&mut Diagnostic` is now used in a lot of function signatures, which used to take `&mut DiagnosticBuilder` (in the interest of not having to make those functions generic)
  * the APIs were already mostly identical, allowing for low-effort porting to this new setup
  * only some of the suggestion methods needed some rework, to have the extra `DiagnosticBuilder` functionality on the `Diagnostic` methods themselves (that change is also present in #93259)
  * `.emit()`/`.cancel()` aren't available, but IMO calling them from an "error decorator/annotator" function isn't a good practice, and can lead to strange behavior (from the caller's perspective)
  * `.downgrade_to_delayed_bug()` was added, letting you convert any `.is_error()` diagnostic into a `delay_span_bug` one (which works because in both cases the guarantees available are the same)

This PR should ideally be reviewed commit-by-commit, since there is a lot of fallout in each.

r? `@estebank` cc `@Manishearth` `@nikomatsakis` `@mark-i-m`
  • Loading branch information
bors committed Feb 25, 2022
2 parents 4e82f35 + b7e95de commit d4de1f2
Show file tree
Hide file tree
Showing 134 changed files with 1,497 additions and 1,143 deletions.
64 changes: 37 additions & 27 deletions compiler/rustc_borrowck/src/borrowck_errors.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
use rustc_errors::{struct_span_err, DiagnosticBuilder, DiagnosticId};
use rustc_errors::{struct_span_err, DiagnosticBuilder, DiagnosticId, ErrorReported};
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_span::{MultiSpan, Span};

impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
crate fn cannot_move_when_borrowed(&self, span: Span, desc: &str) -> DiagnosticBuilder<'cx> {
crate fn cannot_move_when_borrowed(
&self,
span: Span,
desc: &str,
) -> DiagnosticBuilder<'cx, ErrorReported> {
struct_span_err!(self, span, E0505, "cannot move out of {} because it is borrowed", desc,)
}

Expand All @@ -13,7 +17,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
desc: &str,
borrow_span: Span,
borrow_desc: &str,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let mut err = struct_span_err!(
self,
span,
Expand All @@ -32,7 +36,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
span: Span,
verb: &str,
desc: &str,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
struct_span_err!(
self,
span,
Expand All @@ -51,7 +55,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
old_loan_span: Span,
old_opt_via: &str,
old_load_end_span: Option<Span>,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let via =
|msg: &str| if msg.is_empty() { "".to_string() } else { format!(" (via {})", msg) };
let mut err = struct_span_err!(
Expand Down Expand Up @@ -99,7 +103,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
desc: &str,
old_loan_span: Span,
old_load_end_span: Option<Span>,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let mut err = struct_span_err!(
self,
new_loan_span,
Expand Down Expand Up @@ -132,7 +136,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
noun_old: &str,
old_opt_via: &str,
previous_end_span: Option<Span>,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let mut err = struct_span_err!(
self,
new_loan_span,
Expand Down Expand Up @@ -164,7 +168,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
old_opt_via: &str,
previous_end_span: Option<Span>,
second_borrow_desc: &str,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let mut err = struct_span_err!(
self,
new_loan_span,
Expand Down Expand Up @@ -200,7 +204,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
kind_old: &str,
msg_old: &str,
old_load_end_span: Option<Span>,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let via =
|msg: &str| if msg.is_empty() { "".to_string() } else { format!(" (via {})", msg) };
let mut err = struct_span_err!(
Expand Down Expand Up @@ -243,7 +247,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
span: Span,
borrow_span: Span,
desc: &str,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let mut err = struct_span_err!(
self,
span,
Expand All @@ -262,20 +266,20 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
span: Span,
desc: &str,
is_arg: bool,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let msg = if is_arg { "to immutable argument" } else { "twice to immutable variable" };
struct_span_err!(self, span, E0384, "cannot assign {} {}", msg, desc)
}

crate fn cannot_assign(&self, span: Span, desc: &str) -> DiagnosticBuilder<'cx> {
crate fn cannot_assign(&self, span: Span, desc: &str) -> DiagnosticBuilder<'cx, ErrorReported> {
struct_span_err!(self, span, E0594, "cannot assign to {}", desc)
}

crate fn cannot_move_out_of(
&self,
move_from_span: Span,
move_from_desc: &str,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
struct_span_err!(self, move_from_span, E0507, "cannot move out of {}", move_from_desc,)
}

Expand All @@ -287,7 +291,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
move_from_span: Span,
ty: Ty<'_>,
is_index: Option<bool>,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let type_name = match (&ty.kind(), is_index) {
(&ty::Array(_, _), Some(true)) | (&ty::Array(_, _), None) => "array",
(&ty::Slice(_), _) => "slice",
Expand All @@ -309,7 +313,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
&self,
move_from_span: Span,
container_ty: Ty<'_>,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let mut err = struct_span_err!(
self,
move_from_span,
Expand All @@ -327,7 +331,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
verb: &str,
optional_adverb_for_moved: &str,
moved_path: Option<String>,
) -> DiagnosticBuilder<'tcx> {
) -> DiagnosticBuilder<'tcx, ErrorReported> {
let moved_path = moved_path.map(|mp| format!(": `{}`", mp)).unwrap_or_default();

struct_span_err!(
Expand All @@ -346,7 +350,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
span: Span,
path: &str,
reason: &str,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
struct_span_err!(self, span, E0596, "cannot borrow {} as mutable{}", path, reason,)
}

Expand All @@ -357,7 +361,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
immutable_place: &str,
immutable_section: &str,
action: &str,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let mut err = struct_span_err!(
self,
mutate_span,
Expand All @@ -376,7 +380,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
&self,
span: Span,
yield_span: Span,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let mut err = struct_span_err!(
self,
span,
Expand All @@ -387,7 +391,10 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
err
}

crate fn cannot_borrow_across_destructor(&self, borrow_span: Span) -> DiagnosticBuilder<'cx> {
crate fn cannot_borrow_across_destructor(
&self,
borrow_span: Span,
) -> DiagnosticBuilder<'cx, ErrorReported> {
struct_span_err!(
self,
borrow_span,
Expand All @@ -400,7 +407,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
&self,
span: Span,
path: &str,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
struct_span_err!(self, span, E0597, "{} does not live long enough", path,)
}

Expand All @@ -410,7 +417,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
return_kind: &str,
reference_desc: &str,
path_desc: &str,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let mut err = struct_span_err!(
self,
span,
Expand All @@ -435,7 +442,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
closure_kind: &str,
borrowed_path: &str,
capture_span: Span,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
let mut err = struct_span_err!(
self,
closure_span,
Expand All @@ -454,11 +461,14 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
crate fn thread_local_value_does_not_live_long_enough(
&self,
span: Span,
) -> DiagnosticBuilder<'cx> {
) -> DiagnosticBuilder<'cx, ErrorReported> {
struct_span_err!(self, span, E0712, "thread-local variable borrowed past end of function",)
}

crate fn temporary_value_borrowed_for_too_long(&self, span: Span) -> DiagnosticBuilder<'cx> {
crate fn temporary_value_borrowed_for_too_long(
&self,
span: Span,
) -> DiagnosticBuilder<'cx, ErrorReported> {
struct_span_err!(self, span, E0716, "temporary value dropped while borrowed",)
}

Expand All @@ -467,7 +477,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
sp: S,
msg: &str,
code: DiagnosticId,
) -> DiagnosticBuilder<'tcx> {
) -> DiagnosticBuilder<'tcx, ErrorReported> {
self.infcx.tcx.sess.struct_span_err_with_code(sp, msg, code)
}
}
Expand All @@ -476,7 +486,7 @@ crate fn borrowed_data_escapes_closure<'tcx>(
tcx: TyCtxt<'tcx>,
escape_span: Span,
escapes_from: &str,
) -> DiagnosticBuilder<'tcx> {
) -> DiagnosticBuilder<'tcx, ErrorReported> {
struct_span_err!(
tcx.sess,
escape_span,
Expand Down
36 changes: 26 additions & 10 deletions compiler/rustc_borrowck/src/diagnostics/bound_region_errors.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use rustc_errors::DiagnosticBuilder;
use rustc_errors::{DiagnosticBuilder, ErrorReported};
use rustc_infer::infer::canonical::Canonical;
use rustc_infer::infer::error_reporting::nice_region_error::NiceRegionError;
use rustc_infer::infer::region_constraints::Constraint;
Expand Down Expand Up @@ -120,7 +120,11 @@ impl<'tcx, F, G> ToUniverseInfo<'tcx> for Canonical<'tcx, type_op::custom::Custo
trait TypeOpInfo<'tcx> {
/// Returns an error to be reported if rerunning the type op fails to
/// recover the error's cause.
fn fallback_error(&self, tcx: TyCtxt<'tcx>, span: Span) -> DiagnosticBuilder<'tcx>;
fn fallback_error(
&self,
tcx: TyCtxt<'tcx>,
span: Span,
) -> DiagnosticBuilder<'tcx, ErrorReported>;

fn base_universe(&self) -> ty::UniverseIndex;

Expand All @@ -130,7 +134,7 @@ trait TypeOpInfo<'tcx> {
cause: ObligationCause<'tcx>,
placeholder_region: ty::Region<'tcx>,
error_region: Option<ty::Region<'tcx>>,
) -> Option<DiagnosticBuilder<'tcx>>;
) -> Option<DiagnosticBuilder<'tcx, ErrorReported>>;

fn report_error(
&self,
Expand Down Expand Up @@ -188,7 +192,11 @@ struct PredicateQuery<'tcx> {
}

impl<'tcx> TypeOpInfo<'tcx> for PredicateQuery<'tcx> {
fn fallback_error(&self, tcx: TyCtxt<'tcx>, span: Span) -> DiagnosticBuilder<'tcx> {
fn fallback_error(
&self,
tcx: TyCtxt<'tcx>,
span: Span,
) -> DiagnosticBuilder<'tcx, ErrorReported> {
let mut err = tcx.sess.struct_span_err(span, "higher-ranked lifetime error");
err.note(&format!("could not prove {}", self.canonical_query.value.value.predicate));
err
Expand All @@ -204,7 +212,7 @@ impl<'tcx> TypeOpInfo<'tcx> for PredicateQuery<'tcx> {
cause: ObligationCause<'tcx>,
placeholder_region: ty::Region<'tcx>,
error_region: Option<ty::Region<'tcx>>,
) -> Option<DiagnosticBuilder<'tcx>> {
) -> Option<DiagnosticBuilder<'tcx, ErrorReported>> {
tcx.infer_ctxt().enter_with_canonical(
cause.span,
&self.canonical_query,
Expand All @@ -231,7 +239,11 @@ impl<'tcx, T> TypeOpInfo<'tcx> for NormalizeQuery<'tcx, T>
where
T: Copy + fmt::Display + TypeFoldable<'tcx> + 'tcx,
{
fn fallback_error(&self, tcx: TyCtxt<'tcx>, span: Span) -> DiagnosticBuilder<'tcx> {
fn fallback_error(
&self,
tcx: TyCtxt<'tcx>,
span: Span,
) -> DiagnosticBuilder<'tcx, ErrorReported> {
let mut err = tcx.sess.struct_span_err(span, "higher-ranked lifetime error");
err.note(&format!("could not normalize `{}`", self.canonical_query.value.value.value));
err
Expand All @@ -247,7 +259,7 @@ where
cause: ObligationCause<'tcx>,
placeholder_region: ty::Region<'tcx>,
error_region: Option<ty::Region<'tcx>>,
) -> Option<DiagnosticBuilder<'tcx>> {
) -> Option<DiagnosticBuilder<'tcx, ErrorReported>> {
tcx.infer_ctxt().enter_with_canonical(
cause.span,
&self.canonical_query,
Expand Down Expand Up @@ -288,7 +300,11 @@ struct AscribeUserTypeQuery<'tcx> {
}

impl<'tcx> TypeOpInfo<'tcx> for AscribeUserTypeQuery<'tcx> {
fn fallback_error(&self, tcx: TyCtxt<'tcx>, span: Span) -> DiagnosticBuilder<'tcx> {
fn fallback_error(
&self,
tcx: TyCtxt<'tcx>,
span: Span,
) -> DiagnosticBuilder<'tcx, ErrorReported> {
// FIXME: This error message isn't great, but it doesn't show up in the existing UI tests,
// and is only the fallback when the nice error fails. Consider improving this some more.
tcx.sess.struct_span_err(span, "higher-ranked lifetime error")
Expand All @@ -304,7 +320,7 @@ impl<'tcx> TypeOpInfo<'tcx> for AscribeUserTypeQuery<'tcx> {
cause: ObligationCause<'tcx>,
placeholder_region: ty::Region<'tcx>,
error_region: Option<ty::Region<'tcx>>,
) -> Option<DiagnosticBuilder<'tcx>> {
) -> Option<DiagnosticBuilder<'tcx, ErrorReported>> {
tcx.infer_ctxt().enter_with_canonical(
cause.span,
&self.canonical_query,
Expand All @@ -329,7 +345,7 @@ fn try_extract_error_from_fulfill_cx<'tcx>(
infcx: &InferCtxt<'_, 'tcx>,
placeholder_region: ty::Region<'tcx>,
error_region: Option<ty::Region<'tcx>>,
) -> Option<DiagnosticBuilder<'tcx>> {
) -> Option<DiagnosticBuilder<'tcx, ErrorReported>> {
let tcx = infcx.tcx;

// We generally shouldn't have errors here because the query was
Expand Down
Loading

0 comments on commit d4de1f2

Please sign in to comment.