11

rustc_errors: let `DiagnosticBuilder::emit` return a "guarantee of emission...

 3 years ago
source link: https://github.com/rust-lang/rust/pull/93368
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Copy link

Member

eddyb commented on Jan 27

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 DiagnosticBuilders 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 (I tried and gave up, it's much more work than this PR),
    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


report

Recommend

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK