ACP: redesign `proc_macro_diagnostic`

[mejrs]

Proposal

Problem statement

The proc_macro_diagnostic feature has been stuck in limbo for years, with several unresolved design questions and implementation problems. To name a few:

• The current api makes it easy and convenient to "do the wrong thing", which is making a diagnostic that's not attached to a span
• dtolnays open pr does (and implies) various objectionable things regarding path resolution for lint names
• I dislike the current proc_macro::Diagnostic api. We don't do Into<String> bounds and the api feels like "too much" and error prone
• See also Tracking Issue: Procedural Macro Diagnostics (RFC 1566) rust#54140 (comment) for dtolnay's thoughts about the API.

Motivating examples or use cases

Emitting errors and lints from proc macros should be easy and first class.

Solution sketch

Remove the current API in favor of:

mod diagnostic {
    /// Attribute proc macro for declaring a lint.
    #[rustc_builtin_macro]
    pub macro proc_macro_lint($item:item) {
        /* compiler built-in */
    }
    
    #[derive(Copy, Clone)]
    pub struct Lint {
        // private
    }

    pub trait Diagnostic {
        // doc(hidden)/internal methods, can only be implemented by the macro
    }

    pub fn emit_error(span: Span, diag: impl Diagnostic);
    pub fn emit_lint(span: Span, lint: Lint, diag: impl Diagnostic);
    
    // Convenience impls for simple errors/lints.
    impl Diagnostic for String {}
    impl Diagnostic for &str {}
    
    /// Derive `Diagnostic` for structs and enums.
    #[rustc_builtin_macro(Diagnostic, attributes(diag, note, help, etc...))]
    pub macro Diagnostic($item:item) {
        /* compiler built-in */
    }
}

Lints are declared with the proc_macro_lint attribute macro:

#[proc_macro_lint(name = "namespace::lint_name", level = "Allow")]
static LINT: proc_macro::Lint;

The macro will create an initializer for Lint. An instance of this type must be used to emit a lint.

The supplied name must be of the form namespace::lint_name, with a single path separator. Both names must be valid identifiers, can only contain a..=z and _ and may not be keywords. It is an error to use a predefined tool name as the namespace name. Note that with register_tool it is possible to declare arbitrarily named tool lints (and attributes). As such detection of unknown lints is useless as a "wrong" lint name could just be an unknown tool lint and we have no way of checking that. Renaming or importing lint names seems similarly pointless to me and it is an implementation headache so I'd really like to not support it.

An optional lint level (Allow/Warn/Deny) can be supplied. The lint level defaults to Warn.

Users can control the lint level with the the lint check attributes. To name the lint in a lint check attribute, users must first register the namespace:

#![register_lint_tool(namespace)]

#[allow(namespace::lint_name)]
mod x {
    // ..
}

Simple example

For macro authors that just want to emit a simple message this api is as simple as possible:

#[proc_macro_lint(name = "namespace::no", level = "Deny")]
static DONT_DO_THAT: proc_macro::Lint;

#[proc_macro_attribute]
pub fn my_attr(attr: TokenStream, input: TokenStream) -> TokenStream {
    // ....
    proc_macro::diagnostic::emit_lint(span, DONT_DO_THAT, "this is really bad");
}

Advanced example

This api is focused around structured diagnostics. It looks similar to how structured diagnostics are implemented in rustc (See here for its documentation).

#[proc_macro_lint(name = "pyo3::deprecated", level = "Warn")]
static DEPRECATED: proc_macro::Lint;

#[derive(Diagnostic)]
#[diag("the option {name} has been deprecated")]
#[note("this option will be removed in pyo3 version {when}")]
pub(crate) struct DeprecatedOption {
    pub name: String,
    pub when: String,
}

#[proc_macro_attribute]
pub fn pyclass(attr: TokenStream, input: TokenStream) -> TokenStream {
    // ....
    proc_macro::diagnostic::emit_lint(
        span,
        DEPRECATED,
        DeprecatedOption {
            name: "what".into(),
            since: "0.30".into(),
        },
    );
}

I propose structured diagnostics over a builder api for several reasons:

• It encourages the user to structure their diagnostics similarly to rustc's diagnostics, hopefully creating a more seamless user experience.
• A macro based approach makes it easier to guide the macro author by emitting lints/errors/diagnostics.
• Splitting the diagnostic definition from main code paths, in my experience, results in cleaner and less error prone code (for example - from my experience converting rustc diagnostics to structured diagnostics - it's quite easy to have a bunch of spans all named span around and messing up spans in a diagnostic)
• It makes it easier to make changes across editions if necessary.

Structured diagnostics can be less flexible. There are errors in rustc which are hard to express as structured diagnostics because they are passed though many code paths, each of which add to the error in their own way. However proc macros have access to much less information than the compiler so this flexibility should rarely be necessary. People can also still build builder-style apis on top of a structured api (or the other way around).

Alternatives

We can do nothing. As a macro author you can create errors by returning compile_error! invocations. But those are crude (just a message) and they heavily incentivize returning a TokenStream (containing the compile_error) rather than continuing to parse and emit multiple errors. Similarly you can expand into items that trigger rustc lints (e.g. expand to a deprecated item, which in turn creates a message). Or you can just print to stdout.

We can stabilize the current api. I don't think anyone is really happy with it. I guess people can build nicer apis on top of it, though.

Links and related work

Proc macro diagnostics tracking issue rust-lang/rust#54140
The open (stuck) LintId rust-lang/rust#135432
HackMD version of this issue: https://hackmd.io/Kx0L8qzbTU-n7oV9dHIYzg?view

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
  the current

[programmerjake]

what happens if you want a note but only if some condition is met? or if you want a variable number of notes? are users forced to create duplicate definitions for all the variants?

[mejrs]

In rustc diagnostic structs you can do that like this:

#[derive(Diagnostic)]
#[diag("the main message)]
pub struct MyDiagnostic {
     #[note("maybe note")]
     talk_about_this: bool,
     #[note("also maybe note (with its own span)")]
     mention_that: Option<Span>,
     #[subdiagnostic]
     so_many_notes: Vec<MyNote>,
}

So it is possible to support all of that. I'm not sure exactly what shape that'll take - that's one of the things I'd like to experiment with.

[m-ou-se]

At the library team meetup in 2023, we discussed this design and were pretty happy with it. The main blocker since then has been mainly how to deal with warnings/lints and their interaction with #[allow] etc.

[m-ou-se]

@mejrs I noticed your GitHub profile says you live in Delft. If you have the time, you are very welcome to join us at the Hexcat office in the city centre some time to brainstorm a bit on this feature. We have a very big whiteboard and a small collection of Rustaceans who probably have useful thoughts. I think it might be useful to brainstorm a bit and bring the conclusions back to this thread. Ping me on Zulip if you want to drop by.

(Edit: I was informed by Jana that you have already visited our office before. Regardless, the offer still stands to brainstorm about the proc_macro_diagnostic feature some time!)

[mejrs]

I have! But you weren't there on that day 😞 I'm happy to drop by to discuss this.

[mejrs]

original op can be viewed here

Proposal

Problem statement

The proc_macro_diagnostic feature has been stuck in limbo for years, with several unresolved design questions and implementation problems. To name a few:

• The current api makes it easy and convenient to "do the wrong thing", which is making a diagnostic that's not attached to a span
• dtolnays open pr does (and implies) various objectionable things regarding path resolution for lint names
• I dislike the current proc_macro::Diagnostic api. We don't do Into<String> bounds and the api feels like "too much" and error prone
• See also Tracking Issue: Procedural Macro Diagnostics (RFC 1566) rust#54140 (comment) for dtolnay's thoughts about the API.

Motivating examples or use cases

Emitting errors and lints from proc macros should be easy and first class.

Solution sketch

Remove the current API in favor of:

mod diagnostic {
    /// Attribute proc macro for declaring a lint.
    #[rustc_builtin_macro]
    pub macro proc_macro_lint($item:item) {
        /* compiler built-in */
    }
    
    #[derive(Copy, Clone)]
    pub struct Lint {
        // private
    }

    pub trait Diagnostic {
        // doc(hidden)/internal methods, can only be implemented by the macro
    }

    pub fn emit_error(span: Span, diag: impl Diagnostic);
    pub fn emit_lint(span: Span, lint: Lint, diag: impl Diagnostic);
    
    // Convenience impls for simple errors/lints.
    impl Diagnostic for String {}
    impl Diagnostic for &str {}
    
    /// Derive `Diagnostic` for structs and enums.
    #[rustc_builtin_macro(Diagnostic, attributes(diag, note, help, etc...))]
    pub macro Diagnostic($item:item) {
        /* compiler built-in */
    }
}

Lints are declared with the proc_macro_lint attribute macro:

#[proc_macro_lint(name = "namespace::lint_name", level = "Allow")]
static LINT: proc_macro::Lint;

The macro will create an initializer for Lint. An instance of this type must be used to emit a lint.

The supplied name must be of the form namespace::lint_name, with a single path separator. Both names must be valid identifiers, can only contain a..=z and _ and may not be keywords. It is an error to use a predefined tool name as the namespace name. Note that with register_tool it is possible to declare arbitrarily named tool lints (and attributes). As such detection of unknown lints is useless as a "wrong" lint name could just be an unknown tool lint and we have no way of checking that. Renaming or importing lint names seems similarly pointless to me and it is an implementation headache so I'd really like to not support it.

An optional lint level (Allow/Warn/Deny) can be supplied. The lint level defaults to Warn.

Users can control the lint level with the the lint check attributes. To name the lint in a lint check attribute, users must first register the namespace:

#![register_lint_tool(namespace)]

#[allow(namespace::lint_name)]
mod x {
    // ..
}

Simple example

For macro authors that just want to emit a simple message this api is as simple as possible:

#[proc_macro_lint(name = "namespace::no", level = "Deny")]
static DONT_DO_THAT: proc_macro::Lint;

#[proc_macro_attribute]
pub fn my_attr(attr: TokenStream, input: TokenStream) -> TokenStream {
    // ....
    proc_macro::diagnostic::emit_lint(span, DONT_DO_THAT, "this is really bad");
}

Advanced example

This api is focused around structured diagnostics. It looks similar to how structured diagnostics are implemented in rustc (See here for its documentation).

#[proc_macro_lint(name = "pyo3::deprecated", level = "Warn")]
static DEPRECATED: proc_macro::Lint;

#[derive(Diagnostic)]
#[diag("the option {name} has been deprecated")]
#[note("this option will be removed in pyo3 version {when}")]
pub(crate) struct DeprecatedOption {
    pub name: String,
    pub when: String,
}

#[proc_macro_attribute]
pub fn pyclass(attr: TokenStream, input: TokenStream) -> TokenStream {
    // ....
    proc_macro::diagnostic::emit_lint(
        span,
        DEPRECATED,
        DeprecatedOption {
            name: "what".into(),
            since: "0.30".into(),
        },
    );
}

I propose structured diagnostics over a builder api for several reasons:

• It encourages the user to structure their diagnostics similarly to rustc's diagnostics, hopefully creating a more seamless user experience.
• A macro based approach makes it easier to guide the macro author by emitting lints/errors/diagnostics.
• Splitting the diagnostic definition from main code paths, in my experience, results in cleaner and less error prone code (for example - from my experience converting rustc diagnostics to structured diagnostics - it's quite easy to have a bunch of spans all named span around and messing up spans in a diagnostic)
• It makes it easier to make changes across editions if necessary.

Structured diagnostics can be less flexible. There are errors in rustc which are hard to express as structured diagnostics because they are passed though many code paths, each of which add to the error in their own way. However proc macros have access to much less information than the compiler so this flexibility should rarely be necessary. People can also still build builder-style apis on top of a structured api (or the other way around).

Alternatives

We can do nothing. As a macro author you can create errors by returning compile_error! invocations. But those are crude (just a message) and they heavily incentivize returning a TokenStream (containing the compile_error) rather than continuing to parse and emit multiple errors. Similarly you can expand into items that trigger rustc lints (e.g. expand to a deprecated item, which in turn creates a message). Or you can just print to stdout.

We can stabilize the current api. I don't think anyone is really happy with it. I guess people can build nicer apis on top of it, though.

Links and related work

Proc macro diagnostics tracking issue rust-lang/rust#54140
The open (stuck) LintId rust-lang/rust#135432
HackMD version of this issue: https://hackmd.io/Kx0L8qzbTU-n7oV9dHIYzg?view

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
  the current

We (me, @m-ou-se and @jdonszelmann) met today and discussed this. (Please let me know or edit this if I'm missing something)

@m-ou-se strongly felt we need a basic API and that a derive macro was too much, and that opinionated wrappers are best left for crates. That ended up looking a lot like what she wrote in rust-lang/rust#54140 (comment)

We discussed a stringly typed lint system versus the nameres system that rust-lang/rust#135432 implies. We all felt that doing nameres came with a lot of caveats:

• the implementation would be rather difficult and complex
• • lint nameres is a sizeable extension in rustc_resolve that we and many others are not comfortable with
• • name resolution in #[allow(..)] leads to the "resolving paths in attributes" problem
• macro authors can still write unsilenceable lints by simply not re-exporting the lint.
• we toyed with the idea of binding the lint name to the macro so you'd end up writing #[allow(macro_name::namespace_name::lint_name)]. Then you might end up writing #[allow(macro_name::*::lint_name)]. Does that imply #[allow(clippy::*)] should work? This is neat(?) but a lot of work and probably impossible to implement.

On the other hand, a stringly typed system has different tradeoffs

• no possibilities of renaming if you end up depending on multiple different versions of the same crate
• what if you use some not-serde crate that uses serde_derive?
• typos in lint names cannot be detected
• implementation is simple (we really like simplicity)
• issues related to it being stringly typed are probably quite rare

About the "static lint" thing:

#[proc_macro_lint(name = "namespace::no", level = "Deny")]
static DONT_DO_THAT: proc_macro::Lint;

• no real need for a static if we don't end up exporting it- can be something like declare_lint, either bound to a static/const or used inline.
• a static/const would be a nice place to hang documentation on. We didn't reach a conclusion on how this could fit together with rustdoc. Can docs on a static be inlined in the main crate's docs? How do tools like rust-analyzer get information about lints?

Do we need a Lint type?

• API can be like emit_lint(span, "namespace::lint_name", Level::Warn, msg: &str) -> Handle
• Lint name check would be a runtime check (that's compile-time for the user) and result in "proc macro panicked" errors. Could be a nasty gotcha if the lint is in some obscure code path and rarely emitted.
• Compile time check by forcing Lint creation through macro (be it either attribute on static or function macro) seems like the way to go.
• it would disallow creating lint names at runtime.
• we settled on something like declare_lint!(...).

We settled on that the handle system like in rust-lang/rust#54140 (comment) is probably the way to go. Take note of

Methods for adding subdiagnostics return a handle to the subdiagnostic instead of to the main diagnostic

which implies that things can sometimes render in unexpected ways (main op will get an example of this). We didn't like this but felt reluctantly okay with it:

• changing the api to deal with this would add significant api surface
• that api surface (e.g. actual builders/macro derives/etc) can be implemented in crates quite easily.
• most people are going to tuck this away in a function that always emits the same structure) and not run into it
• initially forbidding adding subdiagnostics to subdiagnostics would be annoying when we ever expand the api
• it's probably OK if we mention this in the documentation

We discussed multispans a bit but I haven't made up my mind up about how/if to integrate it now.

I'll update the OP later today.

[programmerjake]

The supplied name must be of the form namespace::lint_name, with a single path separator. Both names must be valid identifiers, can only contain a..=z and _ and may not be keywords.

you should allow digits too, an example you give is pyo3, other precedent is clippy's string_from_utf8_as_bytes

[ChayimFriedman2]

Is there any reason to disallow some valid Rust identifier? We only warn for non-snake-case when it isn't expected, why deviate from that here?

[dhardy]

Thanks for reviving this issue. I tentatively like the sketch above (even if derive(Diagnostic) is underspecified.

Sometimes it is necessary to attach a note with its own span. Both the above and @m-ou-se's simpler API appear to omit that. An example of mine (proc_macro_error crate syntax; attr is impl ToSpan):

emit_error!(
    attr, "multiple usages of #[collection] within a widget is not currently supported";
    note = coll_span => "previous usage of #[collection]";
    note = "write impls of fns Tile::child_indices, Tile::get_child and Widget::child_node instead";
);

How do tools like rust-analyzer get information about lints?

https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-db/src/generated/lints.rs is generated using https://github.com/rust-lang/rust-analyzer/blob/master/xtask/src/codegen/lints.rs.

Maybe requiring lints be defined in the root of a proc-macro crate (or a dependency of one) would help both r-a and the lint name check.

Should it be possible to cfg-gate/feature-gate a lint definition? I feel like this may be bad practice; it would complicate users #[allow(..)]-ing the lint or similar where it's only maybe applicable.

it would disallow creating lint names at runtime

I don't think we want that anyway?

[mejrs]

Sometimes it is necessary to attach a note with its own span.

That's what span_label does in rustc's api

it would disallow creating lint names at runtime

I don't think we want that anyway?

Right, that's a good thing. I don't want it.

Is there any reason to disallow some valid Rust identifier? We only warn for non-snake-case when it isn't expected, why deviate from that here?

Thanks for bringing this up. I think alphanumeric and _ (like crate_name: https://doc.rust-lang.org/nightly/reference/crates-and-source-files.html?highlight=crate_nam#r-crate.crate_name.restriction) should cover everything we want.

Anyway, I need to do more thinking about this. I'll just temporarily close it to get this off the agenda for now.

[tgross35]

(Updating close kind for now since this wasn't accepted as-is)