Require #[must_use] on Diagnostic structs to prevent future bugs

[Enselic]

This will prevent bugs like #118972 at compile time.

Also see #118973 where this was originally discussed.

This PR handles

• Diagnostic structs

We should also begin to require #[must_use] on

• Diagnostic enums
• Subdiagnostic structs
• Subdiagnostic enums

but let's wait with that until we know we want this at all.

r? @michaelwoerister (who maybe want to delegate this to someone else)

cc @rust-lang/wg-diagnostics

Note: Review this PR commit-by-commit since one of the commits is repetitive but big...

Alternatives

We could maybe turn the Diagnostic derive macro into an attribute macro so that it can add #[must_use] automatically behind the scenes. But I am personally not against the clarity of an explicit #[must_use] attribute.

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

[michaelwoerister]

Thanks, @Enselic!

I'm happy to do the review -- but I'd like confirmation from @rust-lang/wg-diagnostics that they are generally OK with the approach. A quick summary:

• @Enselic, discovered that certain errors where never emitted because the diagnostic struct was never actually emitted (see here)
• @klensy had the idea of adding #[must_use] to these structs in order to catch cases like that.
• This PR makes the Diagnostic derive macro emit an error if it does not find #[must_use]. That is, the attribute has to be added manually but the macro makes sure one cannot forget it.

Does that sound OK?

[estebank]

@michaelwoerister lets go for it.

[nnethercote]

Requiring a mandatory line of boilerplate on 1,300+ structs seems suboptimal to me :(

[compiler-errors]

I agree with @nnethercote. I don't think that this is maintainable, tbh, and it seems like it will immediately begin regressing as people add more derive structs without the attr. (edit: nvm on that last point)

[bors]

☔ The latest upstream changes (presumably #119190) made this pull request unmergeable. Please resolve the merge conflicts.

[michaelwoerister]

One alternative would be to turn the derive macro into an attribute macro and make that add #[must_use].

[michaelwoerister]

I don't think that this is maintainable

I don't quite agree on the maintainability aspect, by the way. As far as I can tell, one would only come into contact with this when adding a new diagnostic struct and the error message gives a simple instruction on what to do. I think the approach is acceptable.

But yes, requiring the user to add boilerplate code is indeed not exactly optimal 🙂

[Enselic]

One alternative would be to turn the derive macro into an attribute macro and make that add #[must_use].

Let's explore that idea a bit more. It is not clear to me exactly how we would achieve this. Let's take this struct as an example:

#[derive(Diagnostic, Clone, Copy)]
#[diag(ast_lowering_underscore_expr_lhs_assign)]
pub struct UnderscoreExprLhsAssign {
    #[primary_span]
    #[label]
    pub span: Span,
}

what would this struct definition look like with the attribute macro approach?

[estebank]

@Enselic I believe that derive macros can't add annotations to the annotated item, only expand to new items, while proc macro attributes can fully rewrite the item they are applied to, including adding new attributes.

[Enselic]

I might explore the proc macro attribute approach later.

Since this PR is not actionable before someone has explored the proc macro attribute approach, I will close this PR for now.

I would like to thank everyone who provided feedback so far 👍

[michaelwoerister]

The attribute macro approach should be rather similar. The main difference is that attribute macros will expect that the item given in the input token stream will be re-emitted (possibly modified) along with any additional items (e.g. "derived" impls).

Then the example above would look like:

#[diag(ast_lowering_underscore_expr_lhs_assign)]
#[derive(Clone, Copy)] // `Diagnostic` is removed, this is handled by `#[diag]` now
pub struct UnderscoreExprLhsAssign {
    #[primary_span]
    #[label]
    pub span: Span,
}

The macro (now #[diag]) would still get the same token stream as input and would still have more or less the same token stream as output, just with input token stream (+ #[must_use]) prepended.

[michaelwoerister]

In the example above, I moved the #[diag] attribute above the #[derive]. This way, derive should already see the modified version of the struct (although it should not make a difference in this case).