Tracking Issue: Procedural Macro Diagnostics (RFC 1566)
SergioBenitez opened this issue · 104 comments
This is a tracking issue for diagnostics for procedural macros spawned off from #38356.
Overview
Current Status
- Implemented under
feature(proc_macro_diagnostic) - In use by Rocket, Diesel, Maud
Next Steps
- #44125
- Implement introspection methods (#52896)
- Implement multi-span support (#52896)
- Implement lint id for warnings (#135432)
- Document thoroughly
- Stabilize
Summary
The initial API was implemented in #44125 and is being used by crates like Rocket and Diesel to emit user-friendly diagnostics. Apart from thorough documentation, I see two blockers for stabilization:
-
Multi-Span Support
At present, it is not possible to create/emit a diagnostic via
proc_macrothat points to more than oneSpan. The internal diagnostics API makes this possible, and we should expose this as well.The changes necessary to support this are fairly minor: a
Diagnosticshould encapsulate aVec<Span>as opposed to aSpan, and thespan_methods should be made generic such that either aSpanor aVec<Span>(ideally also a&[Vec]) can be passed in. This makes it possible for a user to pass in an emptyVec, but this case can be handled as if noSpanwas explicitly set. -
Lint-Associated Warnings
At present, if a
proc_macroemits a warning, it is unconditional as it is not associated with a lint: the user can never silence the warning. I propose that we require proc-macro authors to associate every warning with a lint-level so that the consumer can turn it off.No API has been formally proposed for this feature. I informally proposed that we allow proc-macros to create lint-levels in an ad-hoc manner; this differs from what happens internally, where all lint-levels have to be known apriori. In code, such an API might look lIke:
val.span.warning(lint!(unknown_media_type), "unknown media type");
The
lint!macro might check for uniqueness and generate a (hidden) structure for internal use. Alternatively, the proc-macro author could simply pass in a string:"unknown_media_type".
I'd argue that support for associating warnings with lints should be a separate RFC, and shouldn't block moving forward with unsilenceable warnings (with the expectation that anything to associate warnings with lints would need to be an additional API)
Similarly, I'm not sure we actually need to address multi-span support before this API can be stabilized. The proposed change there involves changing some methods to be generic, which is considered a minor change under RFC #1105. It could also be done by changing Span itself to be an enum, rather than having a separate MultiSpan type
@sgrif I suppose the question is: should unsilenceable warnings be allowed at all? I can't think of a reason to remove this control from the end-user. And, if we agree that they shouldn't be allowed, then we should fix the API before stabilizing anything to account for this. I'd really rather not have four warning methods: warning, span_warning, lint_warning, lint_span_warning.
Similarly, I'm not sure we actually need to address multi-span support before this API can be stabilized.
Sure, but the change is so minor, so why not do it now? What's more, as much as I want this to stabilize as soon as possible, I don't think enough experience has been had with the current API to merit its stabilization. I think we should implement these two features, announce them broadly so others can play with them, gather feedback, and then stabilize.
It could also be done by changing Span itself to be an enum, rather than having a separate MultiSpan type.
Right, that works too.
I suppose the question is: should unsilenceable warnings be allowed at all? I can't think of a reason to remove this control from the end-user.
I think "having a thing we can ship" is a decent reason, but I also think an API that only supports error/help/note, but not errors is sufficiently useful to ship even without warnings. I'd support doing that if it meant we didn't block this on yet another API -- Mostly I just want to avoid having perfect be the enemy of good here.
Sure, but the change is so minor, so why not do it now?
Because we have a perfectly workable API that's being used in the wild right now that we could focus on stabilizing instead. Typically we always trend towards the more conservative option on this sort of thing, shipping an MVP that's forward compatible with extensions we might want in the future.
I don't think enough experience has been had with the current API to merit its stabilization.
So what needs to happen for that? Should we do a public call for testing? Definitely adding more docs is huge. I suppose it'd be good to see what serde looks like using this API as well.
So what needs to happen for that? Should we do a public call for testing? Definitely adding more docs is huge. I suppose it'd be good to see what serde looks like using this API as well.
Yeah, that's exactly what I was thinking.
Because we have a perfectly workable API that's being used in the wild right now that we could focus on stabilizing instead. Typically we always trend towards the more conservative option on this sort of thing, shipping an MVP that's forward compatible with extensions we might want in the future.
I don't think this is an eccentric proposal in any way. When folks play with this, they should have this feature. In any case, I'll be implementing this soon, unless someone beats me to it, as Rocket needs it.
Maybe it's too late (I'm lacking context here), but is there any hope of unifying proc-macro diagnostics with those emitted by the compiler itself? It seems sad and unmotivated to have two parallel implementations of diagnostics. (Rustc's diagnostics also have a suggestions API (albeit still somewhat in flux) that harbors a lot of promise given the new cargo fix subcommand that it would be nice for the Rocket/Diesel/&c. proc-macro world to also benefit from.)
@zackmdavis The API being exposed by the diagnostics API in proc_macro today is a refinement of the internal API; they're already quite unified, with minor differences to account for the context in which they are used. The implementation is a thin shell over the internal implementation.
In general, rustcs evolving needs and the proc_macro diagnostics API aim for stability prohibit the two from being identical. This is a good thing, however: rustc can experiment with unstable APIs as much as it wants without being concerned about stability while proc_macro authors can have a stable, concrete API to build with. Eventually, features from the former can makes their way into the latter.
Maud also uses the diagnostic API. It would benefit from both features described in the summary:
-
Multi-span support – Currently, the duplicate attribute check emits two separate diagnostics for each error. It would be cleaner to emit a single diagnostic instead.
-
Lint-associated warnings – We want to warn on non-standard HTML elements and attributes. But we also want to let the user silence this warning, for forward compatibility with future additions to HTML.
Is there any way to emit warning for arbitrary file? It could be usefull for macros that read additional data from external files (like derive(Template) in https://github.com/djc/askama ) .
If it's not possible, how problematic it is to add to Diagnostics something equivalent to :
fn new_raw<T: Into<String>>(start: LineColumn, end: LineColumn, file: &path::Path, level: Level, message: T) -> Diagnostic ?
Something I find confusing about the current nightly API: does Diagnostic::emit() return? (It appears to do so sometimes but not others; even for errors.)
Currently I must use unreachable!() after cases where I think emit should not return... and sometimes this results in internal error: entered unreachable code while at other times it does not (I can't spot a functional difference between the two cases, except for different spans being used).
In my opinion:
- either procedural macro functions should be revised to return a
Result(probably difficult to do now) - or there should be a documented, reliable way of aborting (i.e.
fn() -> !) with an error code (viapanic!,emit()or whatever; currentlypanic!is reliable but does not give a nice error message)
@dhardy Diagnostic::emit() should always return.
Okay, fair enough; not sure why I some panic messages sometimes and not others.
Then we could do with something that doesn't return; maybe Diagnostic::abort()?
I guess syn::parse::Error::to_compile_error and std::compile_error are what I was looking for.
Personally I'd prefer not exposing a Diagnostic::abort() method, because it encourages macro authors to bail out at the first error instead of collecting as many errors as possible.
The compiler will not proceed with type checking if your macro emits at least one error, so you can get away with returning nonsense (like TokenStream::empty()) in that case.
@macpp It's not possible today. I agree that something like this should exist, but much thought needs to be given to the API that's exposed. The API you propose, for instance, puts the onus of tracking line and column information on the user, and makes it possible to produce a Diagnostic where Rust would make it impossible to do so due to Span restrictions.
The API that I've considered is having a mechanism by which a TokenStream can be retrieved given a file name:
impl TokenStream {
fn from_source_file<P: AsRef<Path>>(path: P) -> Result<TokenStream>;
}This would make it possible to get arbitrary (but "well-formed") spans anywhere in the file. As a bonus, it means you can reuse libraries that work with TokenStream already, and implementing this function is particularly easy given that Rust uses something like it internally already. The downside is that you can only use this when the source contains valid Rust tokens, which in practice means that delimiters must be balanced, which seems like a sane restriction.
Well, of course if you deal with .rs files, then TokenStream::from_source_file is much better solution. However, i want to notice that this works only with valid rust files. To clarify, what i wanted to propose is api that allows me to emit warning for any kind of file - for example if my procedural macro reads some configuration from config.toml, then i want to be able to do something better than
panic("bad configuration in config.toml file: line X column Y")Unfortunately, i forgot about Span restrictions, so api that i proposed earlier is simply wrong for this use case :/
@macpp No, that would work for almost any kind of text file. A TokenStream is only beholden to matching delimiters; it need not necessarily be Rust. If it can be used as input to a macro, it would be parseable in this way. It would work just fine for Askama, for instance.
The downside is that you can only use this when the source contains valid Rust tokens, which in practice means that delimiters must be balanced, which seems like a sane restriction.
And that strings inside single quotes must contain a single codepoint, which eliminates a lot more things (including TOML files) that have arbitrary strings in single quotes.
Even the "delimiters must be balanced" requires that the file use the same idea of where a delimiter is and isn't valid. For example the source file must agree that a ") sequence does not end a previous ( because the Rust parser ) will treat it as part of a string.
In short, I don't have a problem with having a TokenStream::from_source_file, but I would not push this as a solution to the "diagnostics for arbitrary non-Rust source files" problem.
It's been quite a while since the last activity here. @SergioBenitez could you give a brief status update on what's holding back progress on this and if, which help is needed?
@regexident The issue text is up-to-date. The remaining tasks are to:
- Implement lint-associated warnings
- Document thoroughly
- Stabilize
Help would be appreciated on 1 and 2. Once implemented, we can discuss and move forward on 3.
There is currently a Pre-RFC on translating compiler output. If accepted, this may affect the Diagnostic API. Please consider this forward-compatibility issue before stabilization.
Doesn't look like any work has been done on this recently. @sgrif Are you still of the opinion that lint-associated warnings should be a separate RFC? If so, it should just be documentation and stabilizing, based on what @SergioBenitez said.
I haven't followed this closely enough to really have much of an opinion at this point. This sat for long enough that the ecosystem has hacked around it and recreated it with emitting compile_error!. I do think this is a significant enough API to have warranted an RFC in the first place (lint related or not)
Lint-Associated Warnings
At present, if aproc_macroemits a warning, it is unconditional as it is not associated with a lint: the user can never silence the warning. I propose that we require proc-macro authors to associate every warning with a lint-level so that the consumer can turn it off.
No API has been formally proposed for this feature. I informally proposed that we allow proc-macros to create lint-levels in an ad-hoc manner; this differs from what happens internally, where all lint-levels have to be known apriori. In code, such an API might look lIke:val.span.warning(lint!(unknown_media_type), "unknown media type");The
lint!macro might check for uniqueness and generate a (hidden) structure for internal use. Alternatively, the proc-macro author could simply pass in a string:"unknown_media_type".
Having only unknown_media_type could cause problems with semantic versioning and would make it very unintuitive to use.
For example a library author might use one proc-macro, where she/he wants to silence a lint called unknown_character. Later the author adds a new library, which also has a warning called unknown_character and the lint! macro would start to error. This would mean, that every new warning would require a major version bump, because it might break compilation? Silencing both lints is not an option in my opinion.
Therefore, I think it would be better to use namespaces, like it can be seen with rustfmt and clippy (#[allow(clippy::use_self)]).
The lint macro could default to the crate name and otherwise use the provided name:
macro_rules! lint {
($namespace:expr, $lint:literal) => {
println!("{}", concat!($namespace, "::", $lint));
};
($lint:literal) => {
println!("{}", concat!(env!("CARGO_PKG_NAME"), "::", $lint));
}
}
fn main() {
lint!("builder", "some_warning");
lint!("some_warning");
}Of course this does not prevent namespace conflicts entirely, but it makes it more unlikely and still allows for some flexibility.
Another option would be to do something like this:
#[proc_macro_derive(namespaces(builder))]Why is the current proposal to attach the name to the Span?
val.span.warning(lint!(unknown_media_type), "unknown media type");
I would much more prefer to have it in the constructor of a lint:
Diagnostic::new(Level::Warning(lint!("unknown_media_type")), "message");@Luro02 I think an even cleaner solution would be to make each lint declaration an item, so that namespaces can be handled by the module system (much like with the proc macros themselves).
I've reimplemented Diagnostic on stable - at least, the error-reporting part - in proc-macro-error. It delegates to proc_macro::Diagnostic on nightly and fallbacks to hand-made errors' render on stable.
If anybody is interested, you are welcome to check it out.
I'd be happy to try working on some of the implementation if it helps? This is a feature I think would be really useful, so if I can help I'd love to!
Given the tendency towards minimal subsets of features, is there any reason why this shouldn't be stabilized in its current form, perhaps excluding warnings? Having the ability to add errors with notes on a given span would still be incredibly useful.
Ping @m-ou-se — could we possibly get an FCP on stabilization for the subset I mentioned? From my understanding of the new process, this should happen before a stabilization PR. I'll create a PR when appropriate.
To be perfectly clear: what I'm proposing is stabilizing everything except Level::Warning. This would allow the most common use cases to take advantage of diagnostics, while avoiding the issue of unsilenceable warnings. I think the current MultiSpan support is the way to go.
@jhpratt I've nominated this issue for discussion in the next libs meeting (next wednesday).
(Temporarily removed the T-lang label so it doesn't show up as nominated on their agenda. We'll have to put it back before we start an FCP.)
Based on my experience using proc-macro-error, I have a few bits of feedback on the Diagnostic API that I think would be worth addressing before stabilization:
- Having
Diagnostic::newnot take a span feels like an anti-pattern. We should encourage users to pass in spans for better diagnostics. Perhaps renamespannedtonew, and the oldnewto e.g.call_site? proc-macro-errorlets you pass in anyT: ToTokens; it will extract the spans automatically from the tokens provided. It would be nice if we could do something similar here.- Perhaps we don't need to do anything, since a blanket
impl<T: ToTokens> MultiSpan for Tcan be added downstream.
- Perhaps we don't need to do anything, since a blanket
- It seems strange that
Diagnostic::childrenreturnsimpl Iterator<Item = Diagnostic>, when diagnostics themselves can only be nested two levels deep. Not a big deal though.
It seems strange that Diagnostic::children returns impl Iterator<Item = Diagnostic>, when diagnostics themselves can only be nested two levels deep. Not a big deal though.
This restriction could be lifted in the future. I think the json error format already allows it.
To be perfectly clear: what I'm proposing is stabilizing everything except Level::Warning. This would allow the most common use cases to take advantage of diagnostics, while avoiding the issue of unsilenceable warnings. I think the current MultiSpan support is the way to go.
From looking at the Diagnostic API, this looks like it would allow creating unsilenceable 'note' and 'help' messages via Diagnostic::new.
The Diagnostic API seemed to be missing span_suggestions, are we not looking into including suggestions API first?
@pickfire There's no reason that couldn't be done post-stabilization. Having something is better than nothing.
I have not gotten a chance to use this API yet at all, but I can give my impression based on skimming Diagnostic, MultiSpan, and Level. I think this is substantially different from what I would want to stabilize. 🙁
Some observations on how this API diverges from my understanding of diagnostics (keep in mind that I have no experience implementing rustc diagnostics, only reading rendered diagnostics as a library author, and not very closely at that):
-
The set of variants of
Levelmakes it confusing to me. In my view,ErrorandWarningare the same kind of thing, whileNoteandHelpare the same kind of thing, but error/warning are quite different kind of thing than note/help. Is there ever a case where all 4 of these would be equally valid to find? I think of an error or warning as a top-level diagnostic output, and a note or help as being an extra piece of information tacked on to a top-level output to fill in context or guidance respectively. For example an error might have 0 or more notes/helps attached to it; they'd render immediately below the error when emitted. -
As #54140 (comment) raises, unsilenceable notes and helps should not be a thing. In my view every diagnostic is either unsilenceable (error) or silenceable (warning), which implies that notes/helps should not be considered diagnostics in their own right by the API.
-
The
DiagnosticAPI seems too flexible in allowing kinds of things to be strung together. It's possible this just reflects how rustc diagnostics are currently stored internally (I haven't looked), but I am not sure that this would be what should be stably exposed to proc macros. For example attaching two errors as if the second error were a note/help attached to the first error?Diagnostic::new(Level::Error, "error1").span_error(Span::call_site(), "error2")-- in what case is this appropriate compared to emitting two errors independently? Apparently it currently renders as:error: error1 --> dev/main.rs:3:1 | 3 | test!(); | ^^^^^^^^ | error: error2 --> dev/main.rs:3:1 | 3 | test!(); | ^^^^^^^^ = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
as opposed to two separate errors:
error: error1 --> dev/main.rs:3:1 | 3 | test!(); | ^^^^^^^^ | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: error2 --> dev/main.rs:3:1 | 3 | test!(); | ^^^^^^^^ | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
-
It's not clear that
MultiSpancarries its weight in this API. WithDiagnosticalready being sort of a builder anyway, I think it would be better if multiple spans got attached instead in a builder-style way. -
It seems like the methods of
Diagnosticbias toward diagnostics without a span (errorvsspan_error) which does not seem like the right bias. Presumably we want practically every diagnostic to have spans? What is the use case for a diagnostic with no span? If we need for that to still be possible, first of all I think it shouldn't get the shorter method names as a niche use case, but also I wonder whether it shouldn't get any Diagnostic methods at all, and instead be handled via aSpanconstructor which makes a "mystery" span with no source location. That way all diagnostics could unconditionally have a span attached.
Counterproposal sketch:
impl Diagnostic {
// new top level diagnostic item
pub fn error(span: Span, message: impl Into<String>) -> Self;
pub fn warning(name: &Ident, span: Span, message: impl Into<String>) -> Self;
// attach context or guidance to a diagnostic
pub fn note(self, message: impl Into<String>) -> Self;
pub fn help(self, message: impl Into<String>) -> Self;
// add a highlighted span with message to the most recent of the above 4 calls
pub fn label(self, span: Span, message: impl Into<String>) -> Self;
pub fn emit(self);
}
impl Span {
pub fn mystery() -> Self; // or "unidentified", or "void", or ...
}@SergioBenitez thoughts? ^^
I think the fn label() in the above comment would be as a replacement for MultiSpan?
Generally speaking, I think that API would be more straightforward to end users, while still providing sufficient flexibility for the overwhelming majority of cases. I'd be interested in collaborating with others to implement that API (removing the existing one) if desired.
I wonder whether [the ability to create messages without spans] shouldn't get any Diagnostic methods at all, and instead be handled via a Span constructor which makes a "mystery" span with no source location.
Yeah, that was a concern I raised as well.
I think in practice the "mystery" span can be approximated by Span::call_site, so in the spirit of YAGNI I'd support not adding anything at all.
Standalone notes are used by miri for trace messages. In that case I think note is actually the most suited option. It is neither an error, nor a warning.
@SergioBenitez thoughts? ^^
Of course!
I've had the pleasure (pleasure?) of designing, re-designing, tinkering, and re-tinkering with this and related APIs in Rocket, rustc, and other compilers and compiler infrastructure. Indeed, the current proc-macro Diagnostic API is not ideal. I'd written a long(er!) response on why, and what we might fix, and so on, but I think the following examples might present a more cogent argument. Consider the following diagnostic emitted by rustc:
error: this enum takes 2 type arguments but only 1 type argument was supplied
--> src/main.rs:1:11
|
1 | fn f() -> Result<()> {
| ^^^^^^ -- supplied 1 type argument
| |
| expected 2 type arguments
|
note: enum defined here, with 2 type parameters: `T`, `E`
--> /lib/rustlib/src/rust/library/core/src/result.rs:241:10
|
241 | pub enum Result<T, E> {
| ^^^^^^ - -This diagnostic is impossible to emulate today in a proc-macro, and it would continue to be impossible with the suggested APIs and API removals above. This diagnostic requires the ability to associate a message of a given kind with one or more spans which may or may not have a label.
Here's what one API emitting this diagnostic might look like:
Diagnostic::error("this enum takes 2 type arguments but only 1 type argument was supplied")
.label(&result.ident, "expected 2 type arguments")
.label(&result.generics, "supplied 1 type arguemnt")
.with_note("enum defined here, with 2 type parameters: `T`, `E`")
.mark(&std_result.ident)
.mark_all(&std_result.generics)Here's another example rustc diagnostic:
error[E0106]: missing lifetime specifier
--> src/lib.rs:5:29
|
5 | fn bar(x: &str, y: &str) -> &str { }
| ---- ---- ^ expected named lifetime parameter
|
= help: this function's return type contains a borrowed value, but the signature does not say whether it is borrowed from `x` or `y`Here it is, again with the same API as before:
Diagnostic::error("missing lifetime specifier")
.label(&borrow, "expected named lifetime parameter")
.mark_all(&lifetimeless_borrows)
.with_help("this function's return type contains a borrowed value, but ..")The builder corresponds 1-to-1 with the diagnostic output as read from top-to-bottom.
A simple warning:
warning: unused variable: `hello`
--> src/lib.rs:1:6
|
1 | fn f(hello: u8) {
| ^^^^^
|
= note: `#[warn(crate::unused_variables)]` on by defaulthello.warning(lint!(unused_variable), "unused variable: `hello`")This API is derived from real-world usage in a compiler project emitting similarly intricate diagnostics.
The surface of the API is:
impl Diagnostic {
// Creates a new error diagnostic with the message `msg`.
fn error(msg: &str) -> Self;
// Creates a new warning diagnostic with the message `msg`.
fn warning(lint: &Lint, msg: &str) -> Self;
// Creates a new note diagnostic with the message `msg`.
fn note(msg: &str) -> Self;
// Adds a help message to the diagnostic.
fn with_help(self, msg: &str) -> Self;
// Adds a note message to the diagnostic.
fn with_note(self, msg: &str) -> Self;
// Adds one mark with the given `item.span()` to the last message. The mark
// is "primary" if no other mark or label has been applied to the previous
// message and "secondary" otherwise.
fn mark(self, item: impl Spanned) -> Self;
// Adds a spanned mark for every span in `item.spans()` to the last message.
// The marks are "primary" if no other mark or label has been applied to
// the previous message and "secondary" otherwise.
fn mark_all(self, item: impl Spanned) -> Self;
// Adds one spanned mark with a label `msg` with the given `item.span()` to
// the last message. The mark is "primary" if no other mark or label has
// been applied to the previous message and "secondary" otherwise.
fn label(self, item: impl Spanned, msg: &str) -> Self;
}Note that this keeps a top-level note() (rustc, miri, my compiler project, and Rocket use this, so there are use-cases) but removes top-level help, which I have not seen used and intuitively must be associated with a previous message. It also keeps the idea of a MultiSpan but generalized as Spanned. Here is Spanned:
trait Spanned {
type Iter: Iterator<Item = Span>;
fn spans(self) -> Self::Iter;
fn span(self) -> Span {
// A best-effort join. Skips any bad joins.
fn join_all(mut spans: impl Iterator<Item = Span>) -> Option<Span> {
let mut joined = spans.next()?;
for span in spans {
joined = match joined.join(span) {
Some(joined) => joined,
None => continue
};
}
Some(joined)
}
join_all(self.spans()).unwrap_or_else(Span::call_site)
}
fn error(self, msg: &str) -> Diagnostic {
Diagnostic::error(msg).mark(self.span())
}
fn warning(self, lint: &Lint, msg: &str) -> Diagnostic {
Diagnostic::warning(lint, msg).mark(self.span())
}
fn note(self, msg: &str) -> Diagnostic {
Diagnostic::note(lint, msg).mark(self.span())
}
}
impl<S: Spanned, I: IntoIterator<Item = S>> Spanned for I { /* .. */ }
impl Spanned for Span { /* .. */ }Simple diagnostics can continue to be created from a Span:
ident.span.error("unexpected identifier").emit()But can now also be created from Spanned values themselves:
ident.error("unexpected identifier").emit()Some remarks on this API:
-
&strcan beimpl Into<String>orimpl Into<Cow<'static, str>>. -
The
Spannedtrait removes the need to call.span()on every item. -
Spanned::{error, warning, note}methods can also be intrinsic methods onSpan, to avoid needing to import in the common case. -
The caller decides whether to use
Spanned::span()orSpanned::spans(), directly or indirectly, preventing ambiguity. -
It is possible to create span-less diagnostics. I think this is okay. I have not seen this cause any issues in
rustc, Rocket, or any other project, and removing this constraint greatly unifies and simplifies the API surface. -
We almost certainly want
&mut selfversions of most of these builder methods:fn add_help(&mut self, msg: &str); fn add_note(&mut self, msg: &str); fn add_mark(&mut self, item: impl Spanned); fn add_mark_all(&mut self, item: impl Spanned); fn add_label(&mut self, item: impl Spanned, msg: &str);
-
mark_allwould instead be calledmarks, followingrustc's internal diagnostic API naming. I thinkmark_all, or something like it, is helpfully more explicit and harder to mistype, but naming, as always, is up for debate. -
It would be nice to support
suggestions, i.e, things that are maybe fixable viarustfix, but the API would require even more thought than warnings. Nevertheless, I believe the API above can easily be extended to support suggestions, should a suitable API arise. -
A
lint!macro is introduced for declaring and registering lints. To prevent namespace collisions, warning lints should be scoped to the proc-macro crate much like proc-macros themselves. The end-user should be able to write any of the following:// attached to an item #[allow(rocket::unsupported_payload)] #[deny(rocket::unsupported_payload)] // applied to a module #![allow(rocket::unsupported_payload)] #![deny(rocket::unsupported_payload)]
Some attention must given to the API to define these lints. One simple solution, posed here, which should preserve enough flexibility for backwards-compatible future extensions, is to expose a new macro from
proc_macro,lint!(), that opaquely defines and registers a (statically known) lint which is then passed to anywarningbuilder:static UNSUPPORTED_PAYLOAD: &Lint = lint!(unsupported_payload); span.warning(UNSUPPORTED_PAYLOAD, "method does not typically support payloads"); // Alternatively, inline. span.warning(lint!(unsupported_payload), "method does not typically support payloads");
While I would consider the above a requirement for any stabilization, I believe we should take care to allow future backwards-compatible extensions to this idea. Some of these extensions might include
useing lints and allowing lint groups to be defined.use rocket::unsupported_payload; #[allow(unsupported_payload)]
static ALL: &Lint = define_lint_group!(all: UNSUPPORTED_PAYLOAD, MALFORMED_DYN_PARAM); #![allow(rocket::all)]
I am eagerly in favor of stabilizing an API resembling the above.
How difficult would lint-associated warnings be to implement? I've no idea how they're actually implemented internally.
I am on board with the API in #54140 (comment). Would anyone be willing to send an implementation?
Minus the lints (so really the warnings in general), I can give it a shot. I presume the existing Diagnostic et al. should be scrapped in the process?
Why do we need mark and mark_all? The proposed API in #54140 (comment) seemed to be missing span_error and span_warning so that means we cannot have child diagnostic which provides an error? Or maybe that is being solved by label since it may be confusing to have a child span having an error? Also, how do we force a Diagnostic to be used, can we have a warning like must_use or sort which requires the user to run .emit() in case they forget?
Maybe we can postpone the warning lint! as of now? Since we does not seemed to have discussed that in depth.
Regarding .error(), currently rustc have something like rustc --explain E0499, are we looking into something similar in the future where we could put a more application specific errors in details separately, either something like rustc --explain (or cargo-explain) or something like the clippy lists of errors?
Just put up a draft PR (#83363) for the proposed API. It'll probably fail initially, and still needs some more work (I haven't been able to test this locally), but it's a starting point.
Why do we need
markandmark_all?
mark marks one Span, specifically item.span(), while mark_all marks many Spans, specifically item.spans(). These are akin to span_label and span_labels in rustc.
The proposed API in #54140 (comment) seemed to be missing
span_errorandspan_warningso that means we cannot have child diagnostic which provides an error? Or maybe that is being solved bylabelsince it may be confusing to have a child span having an error?
The idea is that there is a single top-level message of kind error, warning, or note, and sub-level messages of kinds note and help that can be attached to these top-level messages to provide more context. Adding context that is itself a warning or error seems counterintuitive. I would be interested to know if there are any rustc diagnostics that to do this, and if so, why.
In any case, quite purposefully so, the API can easily be extended with with_warning and with_error. And you can still emit multiple errors and warnings, of course.
Also, how do we force a
Diagnosticto be used, can we have a warning likemust_useor sort which requires the user to run.emit()in case they forget?
A must_use would be great!
Regarding
.error(), currentlyrustchave something likerustc --explain E0499, are we looking into something similar in the future where we could put a more application specific errors in details separately, either something likerustc --explain(orcargo-explain) or something like the clippy lists of errors?
This seems like something that can be easily implemented outside of rustc or cargo. For instance, I could do:
Diagnostic::error("[E101] unknown media type")
.note("for more information about this error, visit https://rocket.rs/errors/E101")Libraries could make handling an error index and provide Diagnostic extension traits to make this easy as well.
Just for the record, I already placed #[must_use] on the initial constructors. I don't think it's necessary on all methods except .emit(), but it would of course be trivial to do so.
mark marks one Span, specifically item.span(), while mark_all marks many Spans, specifically item.spans(). These are akin to span_label and span_labels in rustc.
Just wondering, would it be good to have mark to be able to mark one or many spans? Or would that be confusing?
The idea is that there is a single top-level message of kind error, warning, or note, and sub-level messages of kinds note and help that can be attached to these top-level messages to provide more context. Adding context that is itself a warning or error seems counterintuitive. I would be interested to know if there are any rustc diagnostics that to do this, and if so, why.
Isn't it supposed to be only error and warning as top level message and note and help as sub-level messages? It may seemed useful but at the same time it could be confusing. Maybe @estebank can give some ideas how we use multiple top level messages, I believe that is being done as the API allows so, never checked on this.
Libraries could make handling an error index and provide Diagnostic extension traits to make this easy as well.
Ah, I didn't thought about that. Maybe we can add an example in the docs showing how one could make use of note to link to error index. It may be even cooler if it is built in to rustdoc, since rustdoc could lay out stuff by modules already, like https://docs.rs/dtolnay/0.0.9/dtolnay/, so I guess errors could even be lay out there for offline error index support.
Miri currently emits top-level note messages in some cases and so does Rocket.
Just wondering, would it be good to have
markto be able to mark one or many spans? Or would that be confusing?
I'm not sure how we'd do that. One major advantage of this new API is that we no longer have to call .span() every time we want to refer to a span of an existing item:
- Diagnostic::spanned_error(item.span(), "this item has an issue");
+ Diagnostic::error("this item has an issue").mark(&item);This works because Diagnostic::mark() calls Spanned::span(). To have mark() call span() or spans() would mean providing the method with additional information about which method to call, sacrificing the convenience and/or pithiness of the API. In addition, I do believe any such API would be far too implicit or error prone, one of the reasons I've chosen mark_all() (as opposed to, say, marks()) as the method name to mark many spans.
Isn't it supposed to be only
errorandwarningas top level message andnoteandhelpas sub-level messages? It may seemed useful but at the same time it could be confusing. Maybe @estebank can give some ideas how we use multiple top level messages, I believe that is being done as the API allows so, never checked on this.
As mentioned before, a top-level note is already in use by projects like Rocket, miri, rustc, and more, so removing support for top-level notes would leave an externally unbridgeable API gap.
Ah, I didn't thought about that. Maybe we can add an example in the docs showing how one could make use of
noteto link to error index. It may be even cooler if it is built in to rustdoc, since rustdoc could lay out stuff by modules already, like https://docs.rs/dtolnay/0.0.9/dtolnay/, so I guess errors could even be lay out there for offline error index support.
Given that this functionality can exist wholly outside of rustc, cargo, and rustdoc, I would personally advocate against inclusion of such support in Rust itself, especially as we seek to stabilize a minimal API.
Also, how do we force a
Diagnosticto be used, can we have a warning likemust_useor sort which requires the user to run.emit()in case they forget?
If this is a particular worry we could also make it so that emit is a free function that takes a closure that is required to return a Diagnostic, then always call the private emit method on that return value.
I'm still getting up to speed on these APIs but after an initial review of this thread and #83363 I'm surprised by the choice to continue to allow Diagnostics that don't have a Span.
It is possible to create span-less diagnostics. I think this is okay. I have not seen this cause any issues in rustc, Rocket, or any other project, and removing this constraint greatly unifies and simplifies the API surface.
I have encountered proc macros (structopt for example) that have had issues with diagnostics being emitted without associated spans, though I'm not confident that wasn't due to some unrelated issues with proc macros losing span info rather than footguns in the Diagnostic-like APIs it uses. Also, I'm not convinced that we cannot create an equally convenient API that forces a primary span to be provided at compile time. As a strawman proposal, I could imagine combining the solutions to accidentally not emitting with providing the span by moving the emit function to be a method on span.
// instead of this
ident.error("unexpected identifier").emit()
// we could require this
ident.emit(Diagnostic::error("unexpected identifier));On a related note, do we think it's important to require a primary span for child Diagnostics?
I've only used the current API for smaller projects, but personally:
I think the ability to provide span-less diagnostics is fine to leave in. I can imagine wanting to emit a warning about something the macro might notice not part of the code. EG if it's doing something with an external tool. I can't think of a span I'd want more than 'no span' for a case like that.
(As much less significant point, I find the idea of span.emit(Diagnostic::blah(...)) to look much worse than the trailing emit. I'm almost certain there exists an API that is elegant enough to fix that issue, but figured I'd mention it.)
We discussed this in today's @rust-lang/lang meeting. We confirmed that:
- T-lang only has a stake in suggesting that there should exist some way for proc macros to report diagnostics, and we do believe there should be; we have no open questions or concerns.
- The shape of the API is up to @rust-lang/libs-api, in consultation with compiler for what they can support.
For T-libs-api:
It's quite old at this point, but I've got a PR (#83363) open that implements the API proposed above. The implementation itself works (albeit needing a rebase after 14 months), but there was concern over having the ability to emit diagnostics without a span. What's the use case here (from a user's perspective, not the compiler)?
cc @SergioBenitez, who proposed that API
Is there any movement on this? The linked PR has now been open for over a year with vague concerns about an even better API. Personally I think the proposed API is fine, and that not attaching spans doesn't hurt to support. Mostly, I just would like some movement towards a path to stable.
There is no update. I will happily rebase and update the PR if and when it is desired to be merged.
Would any changes to the API require a separate RFC? I'm wondering why suggesting code in these diagnostics aren't supported. 🤔
Sorry if this has been covered already, but could it be possible to implement MultiSpan on TokenStream?
Looking at the state of things, I wonder if it wouldn't be worthwhile to stabilize a minimum set of these APIs: Opaque Span that can only be attained from an existing TokenStream and Diagnostic that can only be created and note/help attached to them, nothing more. There are changes happening on the rustc internal Diagnostic API, mainly around localization, but this basic API I'm proposing hasn't significantly changed since before 1.0 (only "major" changes I recall was when we changed it to accept MultiSpan instead of Span and Into<String> instead of String). For errors in particular we can already rely on panic and compile_error, as proc-macro-error does, but for warnings we have no mechanism, and the results are quite limited.
Stabilizing some of #![feature(proc_macro_span)] and #![feature(proc_macro_span_shrink)] would be very useful all on its own. One method I know there are workarounds for (when used in conjunction with compile_error!) is Span::join.
Diagnostic-wise, there's a PR that's been open for a couple years redesigning the public API for #![feature(proc_macro_diagnostics)], with input from Sergio Benitez and David Tolnay. I asked Josh about this just the other day, and he said the main issue is getting the right people in the meeting to discuss it. Personally, I am happy to join any meeting to discuss the API for diagnostics; just ping me wherever beforehand. At least for time, I'm already tracking the exact spans of the diagnostics and have decent error messages, but being able to avoid hacks and emit proper diagnostics would be significant.
I just had a long meeting with @estebank, in which we attempted to come up with the right API for diagnostics from proc macros.
This is what we came up with:
proc_macro::error(span, "message");(And the same for warning and note.)
Just a basic function call to produce a diagnostic. No Diagnostic:: or .emit() or anything like that.
However! It still supports the more advanced case of labels, subdiagnostics, etc., through the returned object (which can just be ignored in the simple case as above).
While the function immediately registers the diagnostic, it will only be emitted once the proc macro finishes. That means we can keep adding information to it. That can be done through the handle returned by the function:
let diag = proc_macro::error(span, "message");
diag.label(span1, "label 1");
diag.label(span2, "label 2");
// Or, equivalent:
proc_macro::error(span, "message")
.label(span1, "label 1")
.label(span2, "label 2");The object returned by error function is just a handle that can be used to add more information to the diagnostic.
Methods for adding subdiagnostics return a handle to the subdiagnostic instead of to the main diagnostic:
proc_macro::error(span, "message")
.label(span1, "label on the error")
.note("note") // This is a subdiagnostic.
.label(span2, "label on the note"); // This is a label on the subdiagnostic.This gives flexibility by allowing you to keep handles to both the main diagnostic and the subdiagnostic around:
let diag = proc_macro::error(span, "message");
let note = diag.note("note");
diag.label(span1, "label on the error");
note.label(span2, "label on the note");To allow for the kind of chaining proposed in this comment, the subdiagnostic handle can also be used to add more subdiagnostics:
proc_macro::error(span, "message")
.label(span, "label on the error")
.note("note 1")
.label(span1, "label on the first note")
.note("note 2") // Add a second subdiagnostic.
.label(span2, "label on the second note");To allow for multi-spans, the span arguments should be generic like impl Into<Spans> or impl Spans rather than just of type Spans:
proc_macro::error([span1, span2], "multiple span error!");Similarly, the message should be impl Into<DiagnosticMessage> or impl DiagnosticMessage rather than just &str or String to allow for styling, translation, and other possible extensions in the future. (These types or traits don't need to be stable.)
This design allows for a very easy way to do a partial stabilization of the absolute minimum: we can stabilize proc_macro::{error, warning, note} without stabilizing the handle type (or any of its methods). This allows for the most basic case of
proc_macro::warning(span, "warning!");but leaves the design of the methods for additional features (labels, subdiagnostics, suggestions, lints, ...) for later.
It also means that the most basic use case looks very simple and natural, just like a panic or a println: just a single error(span, "message") invocation to produce an error, without having to deal with things like .emit() or builder patterns etc. Keeping it as simple as possible is important to make it an attractive alternative over panic:
- panic!("missing attribute");
+ error(span, "missing attribute");So, this is the API interface that would be stabilized at first:
// crate proc_macro
pub fn error(span: impl Spans, message: impl Message) -> DiagnosticHandle;
pub fn warning(span: impl Spans, message: impl Message) -> DiagnosticHandle;
pub fn note(span: impl Spans, message: impl Message) -> DiagnosticHandle;
impl Message for String;
impl Message for &str;
impl Spans for Span;
impl<I: IntoIterator<Item = Span>> Spans for I;
impl Copy for DiagnosticHandle;
impl !Send for DiagnosticHandle;
impl !Sync for DiagnosticHandle;And this all remains unstable at first:
// This all still needs bikshedding
pub struct DiagnosticHandle { … }
pub trait Spans { … }
pub trait Messages { … }
impl DiagnosticHandle {
pub fn label(self, span: impl Spans, message: impl Message) -> Self;
pub fn note(self, message: impl Message) -> Self;
pub fn help(self, message: impl Message) -> Self;
// pub fn suggestion(…) ?
// pub fn lint(…) ?
// pub fn primary_span(…) ?
// … ?
}This "diagnostic handle" approach makes simple cases simple, is hard to misuse, and allows for more flexibility than the taking-self-by-value-builder pattern. (As a bonus, it fits very well in the proc macro bridge, which already works on the basis of handles to objects in the compiler.)
Thoughts? :)
If y'all think this is a good idea, I'll put turn this into a proper RFC so it doesn't get lost in the comments here. ^^
First impression is positive. The only concern I have is the lack of .emit(), which wouldn't permit lazy diagnostics too easily. Even if the diagnostic were "eager" by default with a method to permit it being lazy could be quite useful. I'm going to investigate my specific use cases, however.
As far as I can tell, that API is similar to the one I proposed in #54140 (comment) with the following changes:
- The static
Diagnosticmethods are now functions and require aSpan. - The former allows removing the
with_labels from the chaining methods. Diagnostic::emit()is removed in favor lazily emitting the diagnostic at some point later.- The functions and methods require you to call
.span()or.spans()yourself. - There's no differentiation between marking one span or multiple spans.
I think those are the only differences, unless I've missed something.
1 and 2 are a stylistic choice. I'd personally prefer static methods on Diagnostic purely from an aesthetic prospective, but removing the with_ prefix is also convenient. So it's a wash for me.
I find 3 particularly problematic, however. The order of diagnostics can be important, and it's unclear to me what order is being proposed: in order of initial construction, or in order of last modification? In either case, this seems like I'll need to do much more thinking about how/when diagnostics are emitted. Are diagnostic emitted if the proc-macro panics, for example? Furthermore, there are many places where I construct a diagnostic in case I need to emit it (usually to clone() and add more info as I go) but simply drop it without emitting it if I don't. This API makes that impossible.
Number 4 (which allows 5) is something I'd be greatly sad to lose. It's a pain to call .span() every single time I want to emit a diagnostic. I would much rather call error(expr, "this expression is invalid") or expr.error("this expression is invalid") than error(expr.span(), "this expression is invalid"). Furthermore, what does error(expr.many_span(), "this expression is invalid") do? In the API I propose, this is invalid as you can't mark more than one span as the initial span of the error, but this API makes that unclear and leads to the implementation needing to choose one span, which is likely a choice it cannot make.
I like this, simple and clear proposal. However if we're going with lazy diagnostics I would like to have a delay_span_bug function (probably with a better name).
The order of diagnostics can be important, and it's unclear to me what order is being proposed: in order of initial construction, or in order of last modification?
We discussed two alternatives: "registration order" (order of construction) or "drop order" (and just rely on an impl Drop for Diagnostic, but that approach could not trigger if a panic occurs).
The functions and methods require you to call .span() or .spans() yourself.
The signature for the functions all use new-types for both span and the message, instead of using the current Span and &str. This gives us flexibility to support things like rich text in descriptions and labels without breaking anyone mainly, but it also allows for anything that "impl Span" to be passed where the span is expected. If tokens and AST nodes implement Span, then you wouldn't have to call .span() yourself.
In the API I propose, this is invalid as you can't mark more than one span as the initial span of the error, but this API makes that unclear and leads to the implementation needing to choose one span, which is likely a choice it cannot make.
rustc's Diagnostic already accepts a MultiSpan in that place: the primary span is effectively Vec<Span> (as can be seen when you have too many formatting arguments in a format! formatting string.
Edit
As far as I can tell, that API is similar to the one I proposed in #54140 (comment)
@m-ou-se and I spent two hours talking about this subject. We referred back to that comment multiple times. I was initially pushing against that approach entirely, and only came around to it because it does have a certain mapping to what you end up seeing, but my initial preference was for something much more explicit about the shape (adding a SubDiagnostic struct), to make invalid states unrepresentable and avoid having incorrect mental models due to slightly misleading API semantics.
I am not in favor of making the constructors for diagnostics reside either on Span or AST nodes: it should either be a function or macro living in the proc_macro namespace.
As far as I can tell, that API is similar to the one I proposed in #54140 (comment) with the following changes:
There are some other differences. For example, with the API you proposed, you can't add labels to the main diagnostic after adding a subdiagnostic. (It'd be added to the last subdiagnostic. With the 'handle' style api, you can keep a handle to the main diagnostic around to add a label to it later.)
The order of diagnostics can be important, and it's unclear to me what order is being proposed:
It's just the order in which the proc_macro::error/warning/note functions are called, regardless of when additional details are added to them.
(As Esteban mentioned we briefly discussed the idea of emitting them on Drop, but we quickly realised that'd result in confusing situations.)
This API makes that impossible.
Not impossible, but it'd require you to keep the details for the diagnostic around yourself, rather than a type of the proc_macro crate providing that functionality for you.
Number 4 (which allows 5) is something I'd be greatly sad to lose. It's a pain to call .span() every single time I want to emit a diagnostic.
I think it's important that the proc_macro crate remains simple and minimal. That doesn't mean we should make it inconvenient on purpose, but it does mean we don't need to provide layers of extra convenience on top of the basic functionality. (Just like how we provide TokenStream, but not a full parser with convenient structs for all grammar elements, which is left to ecosystem crates such as syn.)
I think especially the proc_macro crate should optimize for simplicity and clarity (including in documentation). An API that implicitly calls .span() when passing token to something that needs a span might save some verbosity, but it complicates the documentation and makes code harder to follow for those not working with proc macros every day.
In the same way, I think that note(span, "abc"); for basic notes/warnings/errors is much prerrable over using Diagnostic::note("abc").mark(thing).emit();, even if there are some use cases where it might be useful to delay the .emit() call. I think it's important to keep simple things simple, and that we should be careful not to optimize only for the complex use cases.
(And for the complex use cases: it probably won't take long before there will be a nice crate on crates.io that makes it possible to define diagnostics as structs, just like in rustc and miette for example, built on top of the proc_macro diagnostics functionality.)
[..] but this API makes that unclear and leads to the implementation needing to choose one span, which is likely a choice it cannot make.
It doesn't have to choose. A diagnostic can have multiple primary spans. See #83363 (comment)
Not impossible, but it'd require you to keep the details for the diagnostic around yourself, rather than a type of the
proc_macrocrate providing that functionality for you.
I think it's important that the
proc_macrocrate remains simple and minimal. That doesn't mean we should make it inconvenient on purpose, but it does mean we don't need to provide layers of extra convenience on top of the basic functionality. (Just like how we provide TokenStream, but not a full parser with convenient structs for all grammar elements, which is left to ecosystem crates such assyn.)
Forcing the end user to either add a dependency or reinvent the wheel for something that can be trivially done (and is already implemented for the internal diagnostics) seems needlessly restrictive.
The default could be to eagerly emit, as that is likely what a majority of uses will be. However, having the ability to emit a diagnostic lazily on demand is something that more advanced uses cases will unquestionably want. I'll concede that I think proc_macro should have far more native tooling than it does (acknowledging that there would be tons of bikeshedding), and my belief of .emit() or similar is in line with that.
Even if there were a .lazy_error() method and similar, that would be more than sufficient for what I'd like to see.
More generally, if the diagnostic isn't being emitted on drop, how would the API even work? It would necessarily emit the diagnostic on .error(), but then you'd be adding labels, notes, etc. on a diagnostic that is already emitted, no?
There are some other differences.
Ah, I see now that the differences are rather significant, in fact. The methods for DiagnosticHandle in your comment don't have any receiver, so it wasn't clear to me what was going on (it's still not quite clear), but as far as I can understand, the proposal is that you have an opaque DiagnosticHandle to some part of a diagnostic which you can then add more parts to, and every time you do, you get another handle to that part.
impl DiagnosticHandle { pub fn label(span: impl Spans, message: impl Message) -> DiagnosticHandle; pub fn note(message: impl Message) -> DiagnosticHandle; pub fn help(message: impl Message) -> DiagnosticHandle; // pub fn suggestion(…) ? // pub fn lint(…) ? // pub fn primary_span(…) ? // … ? }
Is the handle Clone? Or Copy? Or do all of those methods take &self?
For example, with the API you proposed, you can't add labels to the main diagnostic after adding a subdiagnostic. (It'd be added to the last subdiagnostic. With the 'handle' style api, you can keep a handle to the main diagnostic around to add a label to it later.)
That's somewhat true. If all we have are the by-move builder methods, this would be true, but I did suggest that we have &mut methods, which would enable this. I don't think I've ever needed to do this, though. But I don't think my API makes it impossible, but less convenient.
I think it's important that the
proc_macrocrate remains simple and minimal. That doesn't mean we should make it inconvenient on purpose, but it does mean we don't need to provide layers of extra convenience on top of the basic functionality. (Just like how we provide TokenStream, but not a full parser with convenient structs for all grammar elements, which is left to ecosystem crates such assyn.)
To be fair, this is because there is no mechanism for Rust to expose an AST in a compatibility-safe manner. I don't think the existence of syn in Rust is a good thing. I think we'd all love to avoid the detriments that come with needing a crate like syn, including compile times.
Not impossible, but it'd require you to keep the details for the diagnostic around yourself, rather than a type of the
proc_macrocrate providing that functionality for you.
But this is really inconvenient. You're effectively asking users to duplicate the entire logical Diagnostic type to get this functionality in exchange for lazily emitting diagnostics, which I still find quite problematic.
In the same way, I think that
note(span, "abc");for basic notes/warnings/errors is much prerrable over usingDiagnostic::note("abc").mark(thing).emit();, even if there are some use cases where it might be useful to delay the.emit()call. I think it's important to keep simple things simple, and that we should be careful not to optimize only for the complex use cases.
The fair comparison would be note(span, "abc") vs. thing.note("abc").emit(), but even that isn't fair as the latter has clear semantics about when the note is emitted whereas the former does not. It's still not clear to me when I should expect the diagnostic to be emitted, or if there are cases in which it won't be emitted.
rustc'sDiagnosticalready accepts aMultiSpanin that place: the primary span is effectivelyVec<Span>(as can be seen when you have too many formatting arguments in aformat!formatting string.
Ah that's true. In fact, I implemented it in proc_macro. 😅
I am not in favor of making the constructors for diagnostics reside either on Span or AST nodes: it should either be a function or macro living in the proc_macro namespace.
I think that's fine, although I'd love to hear why you feel this way.
We discussed two alternatives: "registration order" (order of construction) or "drop order" (and just rely on an
impl Drop for Diagnostic, but that approach could not trigger if apanicoccurs).
Got it. Would diagnostics be emitted if a panic occurs? If so, this would mean that an in-progress diagnostic will be emitted even if it's not complete as this API provides no means by which rustc would know if a diagnostic is complete or not.
In short, I don't see what advantage lazily emitting diagnostics provides. It's really unclear to me when you should expect a diagnostic to be emitted, and if there are cases in which it won't be emitted.
Is the handle Clone? Or Copy?
They are Copy. (This is mentioned in my comment: "impl Copy for DiagnosticHandle".)
The methods for DiagnosticHandle in your comment don't have any receiver
Oh oops. Fixed.
It would necessarily emit the diagnostic on .error(), but then you'd be adding labels, notes, etc. on a diagnostic that is already emitted, no?
That's already explained in my comment: the diagnostics are emitted when the proc macro finishes executing. So, we can still add more information to diagnostics as long as the proc macro runs.
How often does one need lazy diagnostic functionality? Aren't most use cases easily restructured to work with eager diagnostics?
For lazy diagnostics, we need to decide between &mut self or self receivers. Taking self by (mut) reference makes it easy to miss .emit(), and taking self by value makes it annoying to change a mutable diagnostic variable/argument. (Which is why the rustc api chose for &mut.) And having methods for both gets messy. The copyable handle style api fixes/side steps these issues.
The main reason for keeping the stable surface of the proc_macro crate as minimal as possible is that what's stable will be stable forever. We don't really get to do a major version bump (unlike an ecosystem crate). (That's why I don't feel comfortable committing to having tokentree.error() or span.error() etc. What's convenient now might just get in the way in the future.)
Would diagnostics be emitted if a panic occurs? If so, this would mean that an in-progress diagnostic will be emitted even if it's not complete as this API provides no means by which rustc would know if a diagnostic is complete or not.
We can either not show any diagnostics if the proc macro panics, or just show everything until the panic. I prefer the latter. I don't think having an "incomplete diagnostic" is surprising or a problem, as it would work just the same as println!():
println!("a");
panic!();
println!("b");
// "a" shown, followed by a panic message.
// "b" not shown.let diag = error(span, "a");
panic!();
diag.note("b");
// error "a" shown, followed by a panic message.
// note "b" not shown.An error without a note or label is still useful. And it's directly followed by the "proc macro panicked" message that explains it went 💥.
Subjectively and personally, to me it doesn't really matter what gets emitted when it panics. I won't trust any of the output up until that point anyway given my experience with compilers. I'd rather see the crash fixed and getting complete output again before I trust whatever I see up until that point.
This is about when the proc macro panics, not when the entire compiler panics. Rustc catches proc macro panics and presents them as regular errors. Almost every proc macro currently uses panics to report errors as the diagnostics api isn't stable. It will take a while for the entire ecosystem to move to the diagnostics api, until which it is expected that errors will be reported as a mix of the diagnostics api and panics. As such I think it is important for panics to still cause diagnostics to be emitted.
In my view every diagnostic is either unsilenceable (error) or silenceable (warning), which implies that notes/helps should not be considered diagnostics in their own right by the API.
I forgot to mention that @estebank and I also discussed this. While I mostly agreed with the quote above, @estebank convinced me there are enough use cases for non-lint warnings/notes that we probably shouldn't block the warning and note diagnostic API on having a lint API. (For example, proc macros specifically used for their warnings, like a #[warn_when_using_casts] proc macro attribute. Or a proc macro that loads or writes external data/files, producing a note when something has been updated. Or simply using note diagnostics as a substitute for "println debugging". Or a proc macro with its own language/DSL with its own syntax for silencing warnings.)
We can always add a proc_macro::lint(…) function in the future to support opt-in/out lints that work with the #[allow()]/#[deny()]/etc. system, but that will have to come together with something (a macro?) to allow a proc macro crate to define lint names and their default reporting levels.
Almost every proc macro currently uses panics to report errors as the diagnostics api isn't stable.
that's becoming less true since a decent amount of proc macros now use syn::Result which translates to generating compile_error! rather than panicking.
But, they all still have to build a nice message with notes and help. Having diagnostics being stable lets all the proc macros have the same UI as rust compiler errors.
I know the discussion was earlier, but I do agree we shouldn't block this from going anywhere due to the lint API. I think we should start off with something basic like what Sergio suggested with a simple DiagnosticHandle.
Marking this as libs-api nominated to evaluate the current state of this API and consider stabilizing some or all of it.
Currently (to the best of my knowledge), proc macros don't have any way to emit warnings. They can kinda emit errors, via panic, but they can't emit multiple errors that way.
I do understand that proc_macro is something we have to maintain a stable API for, forever. (We can add a new crate, but we can't remove the old one.)
However, I think it's safe to say in general that the concept of "emit an error" or "emit a warning" is to some degree something we'll always want in some form, for as long as we have proc macros. We may wish to improve it in the future, but the core concept is fundamental to a compiler.
That doesn't mean we need to stabilize all of this API, exactly as it currently is. But, by way of example, I think the following bits could be safely stabilized as a very simple subset:
Span::errorandSpan::warning(leavingnoteandhelpunstable for now)DiagnosticandDiagnostic::emit(but not the rest ofDiagnostic, for now)
That would be just enough to emit errors and warnings, using a single Span. There's a lot more that proc macros may want in the future, but I think the above subset should be safe to commit to.
Thoughts?
cc @rust-lang/wg-macros ^^
They can kinda emit errors, via panic
It is possible to emit compiler_error!("foo") with the spans being set in a specific way to control what it points to. It's bit of a pain, admittedly.
As to the API, I think it would be worth revisiting the proposal from a few years back that I had a PR open to implement (that languished). Even if not stabilizing a large API all at once, there should at least be a general vision rather than simply stabilizing what happened to be submitted as a PR first. That's not to say that the API is bad, just that it should have more thought put into it.
It is possible to emit
compiler_error!("foo")with the spans being set in a specific way to control what it points to. It's bit of a pain, admittedly.
Fair enough. It sounds like there'd still be value in having a simple error/warning mechanism though.
As to the API, I think it would be worth revisiting the proposal from a few years back that I had a PR open to implement (that languished). Even if not stabilizing a large API all at once, there should at least be a general vision rather than simply stabilizing what happened to be submitted as a PR first. That's not to say that the API is bad, just that it should have more thought put into it.
Do you have a summary of that proposal's API surface?
Do you have a summary of that proposal's API surface?
Buried in this issue. See #54140 (comment) and the subsequent few comments. What Sergio proposed is what was implemented (but not merged) in #83363.
As far as I am aware, there hasn't been any meaningful discussion about the diagnostics API since then (the early 2023 comments).
@jhpratt Sergio's proposal seems reasonable to me, but it does have a substantial API surface. The followup version from Mara has a much smaller surface area, though I agree with Sergio's comment that we'd need to at least specify what order things get emitted in. (I would propose "in order of creation", for simplicity.)
I do think that deferring the Spanned concept seems reasonable to reduce API surface area. I agree that it'd be more ergonomic to not have to call .span(), but let's start with an MVP that we can get to the point of stabilization.
I also think it may make sense to make these functions on Span rather than (or in addition to) global functions in proc_macro. You have to have a span to call them, and I think on balance it's easier to write expr.span().error(...) rather than proc_macro::error(expr.span(), ...). @estebank had some concerns with that approach, though, and I'd like to know what those concerns were.
That said, I'd happily sign off on either approach, in the same spirit of starting with an MVP.
A copy of Mara's proposed API surface, for reference:
pub fn error(span: impl Spans, message: impl Message) -> DiagnosticHandle;
pub fn warning(span: impl Spans, message: impl Message) -> DiagnosticHandle;
pub fn note(span: impl Spans, message: impl Message) -> DiagnosticHandle;
impl Message for String;
impl Message for &str;
impl Spans for Span;
impl<I: IntoIterator<Item = Span>> Spans for I;
impl Copy for DiagnosticHandle;
impl !Send for DiagnosticHandle;
impl !Sync for DiagnosticHandle;I'd personally propose Diagnostic instead of DiagnosticHandle, and I'd like to have helper methods on either Span or Spans for the common case, but on balance I think that API would work as an MVP. 👍 for adding it, along with documentation stating that diagnostics are emitted in creation order.
Personally, I agree with @SergioBenitez's comments on Mara's proposal, and prefer the API @SergioBenitez proposal (although I care less about whether or not it is required to call emit() explicitly). (Also, I suspect the lint arg to Diagnostic::warning was not supposed to be there.)
There is one point of @SergioBenitez's proposal I dislike however: his Spanned trait is used for both single and multiple spans. Personally I would simplify it to something like syn::Spanned:
// alternative name: ToSpan
pub trait Spanned {
fn span(&self) -> Span;
}fn mark_all would thus become:
impl Diagnostic {
pub fn mark_all(self, item: impl Iterator<Item = impl Spanned>) -> Self;
}This leaves one gap in the API — joining spans. So lets address that separately:
impl Span {
pub fn join(&self, other: impl Spanned) -> Option<Span>; // existing unstable method with modified arg type
pub fn join_all(iter: impl Iterator<Item = impl Spanned>) -> Option<Span>;
pub fn extend(&self, iter: impl Iterator<Item = impl Spanned>) -> Option<Span>; // optional extra
}Note: the new Span methods could use Item = Span, though this seems unnecessarily restrictive.
Mostly though, I agree with @joshtriplett that it would be good to get some traction on this. The plan to stabilise only Diagnostic, Diagnostic::emit, Span::warning and Span::error makes sense, and is not incompatible with @SergioBenitez's proposal.
/bikeshedding
ToSpan would imply a fn(self) (consuming) method on first read, which I doubt is what you'd want.
I would be really happy to see process on this, as the current way to report errors and warnings for proc-macros is really not good.
I would like to add a few points as input from user of proc-macros:
proc macros don't have any way to emit warnings.
That's currently not true. You can generate a warning by using eprintln!(), but that one is not attached to a span. You also could use something like #[deprecated] on some internal generated type to get out a custom message, although that's then technically displayed as deprecated lint warning instead as proc macro warning. That written: I would see emitting warnings as the feature that would be enabled by exposing such an API. Emitting good errors is already somewhat, even if a bit complicated, possible, while emitting good warnings is just impossible.
That brings me to another point: Generating warnings from proc macros is likely very use-full for a lot of use-cases. Nevertheless people likely want to use #[allow]/#[deny] for these warnings, so maybe put these warnings by default into a "lint" group per proc-marco crate? So for diesel-derives you could do at some level (crate level, item level), #[allow(proc_macro::warnings::diesel_derives)] or something like that. This would downstream users allow to ignore or deny these kind of warnings for whatever reason.
I do think that deferring the Spanned concept seems reasonable to reduce API surface area. I agree that it'd be more ergonomic to not have to call .span(), but let's start with an MVP that we can get to the point of stabilization.
An important point to keep in mind here is that most crates are built on top of proc-macro2 and not on top of the plain proc-macro crate. The former provides it's own proc_macro2::Span type. By accepting impl Spanned instead of just proc_macro::Span it becomes much easier to pass in that span type instead.
When I suggested deferring Spanned/Spans, I mean that we can seal it for now and defer any implementations of it other than for Span. We should still keep the trait abstraction, so that we can extend it later.
Lints, lint groups, and allow/deny/forbid/etc for proc macros would be useful as well, but very much something we can defer from the initial implementation.
And yes, there are ways to get output to the user. I mean that there's no current way to get a well-integrated warning that's associated with the user's code.
I also think it may make sense to make these functions on Span rather than (or in addition to) global functions in proc_macro. You have to have a span to call them, and I think on balance it's easier to write expr.span().error(...) rather than proc_macro::error(expr.span(), ...). @estebank had some concerns with that approach, though, and I'd like to know what those concerns were.
I don't immediately recall what my concerns might have been then, but I'd guess it is about stabilizing an overly constrictive API that doesn't provide as many natural points of expansion (what would the api look like for using a MultiSpan or Vec<Span> + multiple notes + suggestion?), but at the same time I feel like token.span().error(""); is likely to fulfill the needs of most proc-macro developers and its constrained enough that we can commit to maintain it in perpetuity, even if we provide a more comprehensive 2.0 version in the future.
We discussed this in today's @rust-lang/libs-api meeting.
Several people in the meeting said that they didn't want to ship a design without a lint-style mechanism to associate warnings with a name (and namespace that name), allowing for the possibility of suppressing it in the future. (The suppression mechanism doesn't have to be finished before shipping, but the namespacing mechanism does.) Some in the meeting felt that warnings should require such a name or ID, rather than having it be optional.
We discussed two possible designs for naming and namespacing, in the meeting.
-
Declare one or more names at compile-time in the proc macro (e.g. as part of the
proc_macro_attributeattribute or similar). Possibly allow another crate to re-export those names (e.g.foore-exportingfoo_macros::my_lintasfoo::my_lint). Supportallow(foo::my_lint). Require the name in the warning API (and error if using an un-declared name). -
Handle names at runtime. Have the proc macro make a runtime call to set up warnings, giving a list of warning IDs. Note that this would require some mechanism to namespace the IDs; we didn't have an immediate idea for that in the meeting. (The issue was that people may not want to reference these by the specific proc macro name or crate name, because the proc macro name or crate name may be an implementation detail of some higher-level crate, e.g.
foovsfoo_macros.)
Either way, the only path forward that had consensus in the meeting was to have such an ID mechanism for lints, with a namespacing mechanism that ensured two different proc macros wouldn't have conflicting names.
@rustbot labels +T-lang +I-lang-nominated
In the libs-api call today, it was discussed how this feature may overlap with matters that concern lang. Certainly some particular designs, e.g. those that would add namespacing within allow(..) attributes, would be of direct concern to us.
But more broadly, we've been thinking about a number of seemingly-related namespacing concerns, e.g. how to namespace attributes applied to fields for derive macros, the tooling namespace, etc. We may want to think holistically about this, or to encourage designs that fall within whatever direction we take here. And of course, many matters of the proc macro API are inherently lang concerns, as they affect the specification of the language and fall outside of what could otherwise be written in stable Rust.
In the meeting, @dtolnay in particular mentioned too that he'd like to see lang involved here, and that the interesting question is perhaps who would be driving this to then present something to both teams.
So let's tag this (and any follow-on stabilization or other significant PRs) for lang along with libs-api, and then let's nominate it so we can discuss briefly to build context on this.
cc @rust-lang/lang
Here is a skeleton of design 1 and design 2 from #54140 (comment). (The semantics are more interesting than the exact spelling used.)
Static at compile-time of proc macro crate
Re-exportable in macro namespace (or a fourth one?), similar to the name resolution of macros. Documentable by rustdoc.
// foo_macros
#[proc_macro_warning]
static ambiguous_thing;
#[proc_macro_warning]
static ambiguities = [crate::ambiguous_thing];
#[proc_macro_derive(Foo)]
pub fn derive_foo(input: TokenStream) -> TokenStream {
if ... {
proc_macro::warning(crate::ambiguous_thing, span, "...");
}
}
// foo
pub use foo_macros::{ambiguities, ambiguous_thing, Foo};
pub trait Foo {...}
// downstream code
#![allow(foo::ambiguous_thing)]
use foo::Foo;
#[derive(Foo)]
...Static at compile-time of downstream crate
Not re-exportable, similar to the name resolution of inert attributes. Documented manually in crate-level markdown.
// foo_macros
#[proc_macro_derive(Foo)]
pub fn derive_foo(input: TokenStream) -> TokenStream {
setup_warnings();
if ... {
proc_macro::warning("foo::ambiguous_thing", span, "...");
}
}
fn setup_warnings() {
proc_macro::register_warning("foo::ambiguous_thing");
proc_macro::register_warning_group("foo::ambiguities", ["foo::ambiguous_thing"]);
}
// foo
pub use foo_macros::Foo;
pub trait Foo {...}
// downstream code (same as above)
#![allow(foo::ambiguous_thing)]
use foo::Foo;
#[derive(Foo)]
...#[proc_macro_warning]
static ambiguous_thing;
#[proc_macro_warning]
static ambiguities = [crate::ambiguous_thing];This static ambiguous_thing is a static with type LintId and a fresh value? No, that wouldn't work for static compile-time analysis.
Or is this a type-level construction? But then static amiguities makes no sense (unless it's supposed to have type [&dyn Lint], but then we're back to run-time).
Or something else entirely distinct from Rust's usual type model? This gets confusing.
fn setup_warnings() {
proc_macro::register_warning("foo::ambiguous_thing");
proc_macro::register_warning_group("foo::ambiguities", ["foo::ambiguous_thing"]);
}Why bother with this — why not auto-detect lint names? Is it only for lint groups?
There could be an automatic lint group for the proc-macro crate.
This seems like a considerable amount of complexity for a feature I would not expect to have much usage. Proc-macro warnings are only applicable to macro input which is somehow valid yet incorrect, which surely doesn't occur all that often. Further, I wouldn't expect such lints to have frequent false positives (like, say, Clippy lints).
Just a few quick reflections... If someone adds #![allow(xxx::yyy)], how do they distinguish between two different xxx namespaces with the same name? I can't see a nice way to do this which doesn't add lots of complication.
So I tend to feel that having runtime lint names and no sense of namespace identity or collision mitigation (behind string matching) is likely the simplest for the user, the smallest API and probably the most flexible option.
I also don't immediately see any practical problems with allowing different crates to have a namespace collision:
- I think in practice, accidental collisions would be quite unlikely.
- Trolling crates could try to impersonate other lints, but that would just harm their users. And if you've convinced a user to install your crate, you can troll them in much more malicious ways than impersonating a lint.
- The ability for multiple crates to share a namespace actually seems useful. For example, it would be nice if two different but related macro crates could share the same lint namespace. I've actually got a few use-cases in mind for this in one of my work projects, where we have three different proc macro projects for different specializations of the same idea, and would naturally both want to use the same namespace.
This could give some very simple APIs:
// (A) Simplest
proc_macro::warn(
"foo::bar",
span,
"My warning message",
);
// (B) Slightly more type-safe, but effectively still just a string.
// The lints could be defined centrally in a crate as const or static to avoid repetition.
proc_macro::warn(
proc_macro::LintId::new("foo::bar"),
span,
"My warning message",
);As for grouping, it wouldn't really make sense to define groups statically in this model, because they could clash...
But you could possibly support adding groups at emit time to this specific lint, by saying "emit foo::bar which should also considered in groups foo::group1 and foo::group2":
// (C) Option supporting groups
proc_macro::warn(
proc_macro::LintId::new("foo::specific").in_group("foo::group_1").in_group("foo::group_2"),
span,
"My warning message",
);And in terms of use-cases: I'm currently working on a library called preinterpret with a toolkit of simple, composable commands for code-generation (intended for use on its own or inside a declarative macro, and supports e.g. quote and paste functionality)... eventually I have aspirations that it could even be a go-to tool for writing declarative macros, but that's a little way off. In any case, I have been really interested in this topic - and also the span join API - for better error diagnostics.
For what it's worth, runtime lint name resolution would also work better with preinterpret, as users using preinterpret to build their declarative macros could use their own lint names... Something like this:
// This is just example preinterpret syntax, not anything I'm proposing for procedural macro APIs
macro_rules! emit_my_app_warning {
($message:literal) => {preinterpret!{
[!warning! { lint: "my_app::warning", message: $message, spans: [$message], sub_diagnostics: [] }]
}};
}Simultaneously responding to the previous 2 comments:
Why bother with this — why not auto-detect lint names? Is it only for lint groups?
I tend to feel that having runtime lint names […] is likely the simplest for the user, the smallest API and probably the most flexible option.
The issue with the approach @dhedey has shown, and the reason #54140 (comment) looks different than that, is you've only informed the compiler about a particular lint name if that lint is ever triggered.
Imagine the caller writes:
#![deny(foo::ambigous_thing)]
use foo::Foo;
#[derive(Foo)]
...Did you catch the typo?
The compiler needs to know whether an arbitrary scoped lint name refers to a valid thing without it being triggered, in order to report unknown_lints.
In my 2 designs (#54140 (comment)), the first one conveys perfect information about what lint names exist. The second one conveys perfect information (that's the reason for register_warning, @dhardy) if both of the following are true:
- The macro author correctly registers all their lints from the top of all their macros, including lints not reachable by that specific macro's implementation.
- The caller calls at least one macro. It would be silly to depend on a proc macro crate and not call any of its macros, but this can happen in awkward
cfgsituations where Cargo's[target.'cfg(…)'.dependencies]is underpowered. In such cases, rustc would need to report that your allow/warn/deny attribute is anunknown_lints, and you would be required to switch to:#[cfg_attr(…, deny(foo::…))]matching whatever condition corresponds to at least one of that crate's macros being called.
This
static ambiguous_thingis a static with typeLintIdand a fresh value? No, that wouldn't work for static compile-time analysis.
That's right. You would need to tell me more about why this can't work.
This:
#[proc_macro_warning]
static ambiguous_thing: LintId;
#[proc_macro_derive(Foo)]
pub fn derive_foo(input: TokenStream) -> TokenStream {...}would expand to something like:
static ambiguous_thing: LintId = proc_macro::LintId::__new("ambiguous_thing");
const _: () = {
#[rustc_proc_macro_decls]
#[used]
static _DECLS: &[proc_macro::bridge::client::ProcMacro] = &[
proc_macro::bridge::client::ProcMacro::custom_derive("Foo", crate::derive_foo), // same as today
proc_macro::bridge::client::ProcMacro::warning(crate::ambiguous_thing),
];
};This
static ambiguous_thingis a static with typeLintIdand a fresh value? No, that wouldn't work for static compile-time analysis.That's right. You would need to tell me more about why this can't work.
Resolving static ambiguous_thing as an item at compile-time is possible.
Resolving the contents of static ambiguities = [/* omitted */]; may be, but only since this is non-mutable. I don't think it actually matters though since from what I understand all usage happens after the proc-macro is compiled.
@dtolnay mentioned above that the semantics were more important than "the exact spelling", but I would have found the example a little more comprehensible had more effort been paid to typing above. Thus, I'd propose the following to keep within Rust's existing type models:
// The attribute macro here supplies a definition (and, if necessary, registers the lint identifier with the compiler).
#[proc_macro_warning]
static ambiguous_thing: LintId;
// The attribute macro *might* be required to register the lint group identifier, but it might also be completely unnecessary here.
#[proc_macro_warning]
static ambiguities: &[LintId] = &[crate::ambiguous_thing];The group might be better typed as follows if we had support for [LintId; _] (see #85077):
#[proc_macro_warning]
static ambiguities: [LintId; _] = [crate::ambiguous_thing];In particular, I don't think #[proc_macro_warning] should supply the typing information, even if this would be less to write.
// (A) Simplest
proc_macro::warn(
"foo::bar",
span,
"My warning message",
);
I think we should omit the name of the proc_macro from the lint id (so bar rather than foo::bar here). Crates can be renamed on use and in that case I would expect the lint namespace to be renamed as well.