Make diagnostics emitting independent of the happy code path

[oli-obk]

In our first @rust-lang/wg-diagnostics meeting on zulip we figured that a first start would be to create a new trait

trait AsError {
    /// ParseSess, Session or `TyCtxt`
    type Session;
    fn to_error(self, session: Self::Session) -> DiagnosticBuilder;
    fn emit(self) {
        self.to_error().emit()
    }
}

that is implemented for structs containing all relevant info for a diagnostic, e.g.

struct TypeError {
    expected: Ty,
    found: Ty,
    span: Span,
}

and then the code that currently constructs the diagnostic and emits it would just become

TypeError {
    expected,
    found,
    span: Span,
}.emit();

This issue has been assigned to @jumbatm via this comment.

[Manishearth]

The vast majority of these could probably be handled by a custom derive:

#[derive(AsError)]
#[error = "Cannot move {ty} out of borrow", span]
#[note = "First borrwed here", other_span]
#[code = E12345]
struct MoveOutOfBorrowError {
    ty: Ty,
    span: Span,
    other_span: Span,
}

(exact syntax is bikesheddable, of course)

[estebank]

At first I thought about how the derive macro would be constraining for some of the more complex errors, but looking at it I can see how it'd be more than appropriate for the great majority of errors we emit:

#[derive(AsError)]
#[code = E12345]
struct MoveOutOfBorrowError {
    ty: Ty,
    #[error = "cannot move {ty} out of borrow"]
    #[label = "cannot move out of borrow"]
    span: Span,
    #[label = "`{ty}` first borrowed here"]
    other_span: Span,
    #[suggestion(msg = "consider cloning here", code = "{opt_sugg}.clone()")]
    opt_sugg: Option<Span>,
}

[Manishearth]

Oh, tagging spans with this stuff is genius.

[Manishearth]

Hmm, it needs a little tweak:

#[derive(AsError)]
#[code = E12345]
struct MoveOutOfBorrowError {
    ty: Ty,
    #[error = "cannot move {ty} out of borrow"]
    #[label = "cannot move out of borrow"]
    span: Span,
    #[label = "`{ty}` first borrowed here"]
    other_span: Span,
    #[suggestion(msg = "consider cloning here", code = "{1}.clone()")]
    opt_sugg: Option<(Span, Ident)>,
}

[Manishearth]

So here we'd have to special case Ty, Span, and Option. Not too bad.

[Electron-libre]

This sounds like a great improvment in typeck maintainability.
(And probably a lot of work too)
It could be a good opportunity for me to pratice custom derives.

I think I can do this, It will just take time and trials.

What do you think ?

[Manishearth]

Go for it, I think. cc @estebank

The derive will basically have to specially notice Tys and Spans and some other stuff, though. Getting the suggestions working is tricky too.

[estebank]

I think we can iterate and disregard the more complex parts of the feature (IE, implement them manually in the code for now) and later pull them into the proc macro.

[Mark-Simulacrum]

@Electron-libre Are you still working on this? If so please @rustbot claim this issue, thanks!

[Electron-libre]

No sorry, my son is born ( 😍 ) few days after my last comment and then I forgot about this issue.
I will not invest time in OSS contributions the next months.
Someone else should pick this issue.

[Manishearth]

I and @yaahc may end up working on this

[Manishearth]

Starting to think about the design. Would this be implemented as a normal derive proc macro (using a syn dependency) or something more like the builtin libsyntax derives? I'm not sure if rustc having a syn dependency is a good idea.

If it's going to be a libsyntax thing, what x.py commands would y'all recommend for testing it for minimal cycle time? Ideally only libsyntax needs to be built to play with this. It's been a while since i poked at this stuff and the build system has changed.

cc @estebank

[bjorn3]

I think it should be in rustc_macros like other rustc internal derives.

[jumbatm]

@Manishearth @yaahc Did you end up / are you still intending on working on this? It's unassigned and I'd like to take this one, but I just want to make sure I'm not treading on any toes, first.

[Manishearth]

Go for it, I want to eventually work on this but if someone else gets there first that's fine

[jumbatm]

Alright, cheers.

@rustbot claim

[JohnTitor]

Triage: I'm going to release assignment due to inactivity.
@jumbatm If you're still interested in this, feel free to re-claim.
@rustbot release-assignment

[jumbatm]

I'm still actively working on this. Sorry I've been taking so long -- I'll hurry up and get a PR in soon.

@rustbot claim