Rust: Invert control in `struct_lint_level`

[Centril]

See https://github.com/rust-lang/rust/pull/67755#discussion_r362239628.

cc @rust-lang/wg-diagnostics

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

[jumbatm]

I'd like to give this a go.

@rustbot claim

[jumbatm]

Regarding the original suggestion of having struct_lint_level & friends take an Fn(&mut DiagnosticBuilder<'_>, which would require adding fn set_message(&mut self) to DiagnosticBuilder, how would you feel about introducing a lightweight proxy type which stores contextual information for the diagnostic and can only be turned into a DiagnosticBuilder by providing a message? That is:

pub struct DiagnosticBuilder {
    // Keep DiagnosticBuilder the way it currently is ...
}

// But introduce a new proxy type which stores just enough information that methods like
// struct_lint_level can set up things like level and span.
pub struct PendingDiagnosticBuilder<'a> {
    handler: &'a Handler,
    level: Level,
}

impl PendingDiagnosticBuilder<'_> {
    // Only accessible internally to rustc_errors.
    crate fn new(
        handler: &'a Handler,
        level: Level,
        span: Option<MultiSpan>,
    ) -> PendingDiagnosticBuilder<'a> {
        PendingDiagnosticBuilder { handler, level }
    }

    // Only method publicly accessible -- forces the user of the API to explicitly provide a
    // message the same way the API currently does.
    pub fn into_diagnostic_builder(self, msg: &str) -> DiagnosticBuilder {
        DiagnosticBuilder::new(handler, level, msg)
    }
}

// struct_lint_level & co get their signature changed to something like this:
pub fn struct_lint_level<'a>(
    sess: &'a Session,
    lint: &'static Lint,
    level: Level,
    src: LintSource,
    span: Option<MultiSpan>,
    set_message: impl Fn(PendingDiagnosticBuilder<'_>) -> DiagnosticBuilder<'_>,
) -> DiagnosticBuilder<'a> {
    // ...
}

// then, when calling struct_lint_level in another crate:
rustc::lint::struct_lint_level(session, lint, src, span, |pending_diagnostic| {
    pending_diagnostic.into_diagnostic_builder(format!(
        "Some diagnostic message which does string formatting: {}",
        something_expensive_to_display
    ))
});

// the API enforces that the caller provides a message. Because `PendingDiagnosticBuilder::new` and
// `DiagnosticBuilder::new` can't be called outside the crate, users of the API can't create their
// own, erroneous DiagnosticBuilder to return, either.

I feel like this is worth it, because it achieves the original goal of preventing formatting of lint diagnostics that don't end up emitted, but makes it so the public API stops you from forgetting to add a diagnostic message, which I feel like would be too easy to do with defaulting to "" and hoping the caller remembers to set the message in the closure they provide. Of course, there would be nothing stopping the caller from providing an empty message, but that's something a caller can do currently anyway, and at least it would be explicit this way. What do you think?

Also, I know PendingDiagnosticBuilder isn't the greatest name, but couldn't bring myself to name it DiagnosticBuilderBuilder.

[Centril]

The point of the impl Fn(&mut DiagnosticBuilder<'_>) wasn't just to set the message, but rather to allow any sort of change to the builder such as e.g. .span_label. If we're going with a proxy type, which seems fine to me, it would be better to have:

// defined where `struct_lint_level` is:
struct LintDiagnosticBuilder<'a, 'b>(&'a mut DiagnosticBuilder<'b>);

impl<'a, 'b> LintDiagnosticBuilder<'a, 'b> {
    fn build(self, msg: &str) -> &'a mut DiagnosticBuilder<'b> {
        let LintDiagnosticBuilder(diag) = self);
        diag.set_message(msg);
        diag
    }
}

Also, note that struct_lint_level should return (), not DiagnosticBuilder<'_>. That is, we would now have:

pub fn struct_lint_level<'a>(
    sess: &'a Session,
    lint: &'static Lint,
    level: Level,
    src: LintSource,
    span: Option<MultiSpan>,
    decorate: impl Fn(LintDiagnosticBuilder<'_>),
) {
    // ...
}

cc @estebank @oli-obk @Mark-Simulacrum

[Mark-Simulacrum]

It might be a good idea to avoid using impl there (or at least internally cast to &dyn or so quickly), just to avoid possible codegen bloat from duplicated struct_lint_level, since it's quite large.

[estebank]

would require adding fn set_message(&mut self) to DiagnosticBuilder

I don't think this is necessary today.

[jumbatm]

I see. Originally, when I posted that, I thought the only way to set the message was on construction, which I thought was a deliberate design decision to ensure a diagnostic's message couldn't accidentally overwritten. I realised later that Diagnostic already has a set_primary_message, which DiagnosticBuilder can already access via DerefMut to Diagnostic.

I was having some lifetime issues earlier, trying to have decorate return the DiagnosticBuilder that's unwrapped on call to build(...) to prove that a message gets provided. Went as far as to have decorate be for<'a, 'b>, but it makes it makes decorate pretty unergonomic at the callsite when passing in a closure, because lifetime parameters can't explicitly named and related on the closure itself. See here. Instead of doing this, I'm now just planning to not have decorate return anything, and just trust the caller provided a message. Let me know if you have any opinions or suggestions about this.

I have a long weekend now, so I'm aiming to get a PR up to close this issue by tonight or tomorrow night.

edit: The set_message I was looking at was from libproc_macro Diagnostic, but rustc_errors's Diagnostic _does_ have a set_primary_message which does what I need anyway.