search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.62k stars 12.48k forks source link

Tracking Issue: Procedural Macro Diagnostics (RFC 1566) #54140

Open SergioBenitez opened 5 years ago

SergioBenitez commented 5 years ago

This is a tracking issue for diagnostics for procedural macros spawned off from https://github.com/rust-lang/rust/issues/38356.

Overview

Current Status

Next Steps

Summary

The initial API was implemented in https://github.com/rust-lang/rust/pull/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:

  1. Multi-Span Support

    At present, it is not possible to create/emit a diagnostic via proc_macro that points to more than one Span. The internal diagnostics API makes this possible, and we should expose this as well.

    The changes necessary to support this are fairly minor: a Diagnostic should encapsulate a Vec<Span> as opposed to a Span, and the span_ methods should be made generic such that either a Span or a Vec<Span> (ideally also a &[Vec]) can be passed in. This makes it possible for a user to pass in an empty Vec, but this case can be handled as if no Span was explicitly set.

  2. Lint-Associated Warnings

    At present, if a proc_macro emits 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".

sgrif commented 5 years ago

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)

sgrif commented 5 years ago

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

SergioBenitez commented 5 years ago

@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.

sgrif commented 5 years ago

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.

SergioBenitez commented 5 years ago

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.

sgrif commented 5 years ago

Should we be going through the RFC process for additions to the API here? (I noticed that Diagnostic itself actually never got an RFC)

On Tue, Sep 11, 2018 at 3:26 PM Sergio Benitez notifications@github.com wrote:

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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rust-lang/rust/issues/54140#issuecomment-420430972, or mute the thread https://github.com/notifications/unsubscribe-auth/ABdWK2Gpoc6vTWz8fKpJmVaGysf9w7-Uks5uaCpsgaJpZM4WkOKO .

zackmdavis commented 5 years ago

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.)

SergioBenitez commented 5 years ago

@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.

lambda-fairy commented 5 years ago

Maud also uses the diagnostic API. It would benefit from both features described in the summary:

  1. 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.

  2. 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.

macpp commented 5 years ago

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 ?

dhardy commented 5 years ago

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:

SergioBenitez commented 5 years ago

@dhardy Diagnostic::emit() should always return.

dhardy commented 5 years ago

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.

lambda-fairy commented 5 years ago

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.

SergioBenitez commented 5 years ago

@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.

macpp commented 5 years ago

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 :/

SergioBenitez commented 5 years ago

@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.

Arnavion commented 5 years ago

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.

regexident commented 5 years ago

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?

SergioBenitez commented 5 years ago

@regexident The issue text is up-to-date. The remaining tasks are to:

  1. Implement lint-associated warnings
  2. Document thoroughly
  3. Stabilize

Help would be appreciated on 1 and 2. Once implemented, we can discuss and move forward on 3.

kennytm commented 5 years ago

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.

jhpratt commented 4 years ago

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.

sgrif commented 4 years ago

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)

Luro02 commented 4 years ago

  1. Lint-Associated Warnings At present, if a proc_macro emits 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");
}

playground%3B%0A%20%20%20%20%7D%3B%0A%20%20%20%20(%24lint%3Aliteral)%20%3D%3E%20%7B%0A%20%20%20%20%20%20%20%20println!(%22%7B%7D%22%2C%20concat!(env!(%22CARGO_PKG_NAME%22)%2C%20%22%3A%3A%22%2C%20%24lint))%3B%0A%20%20%20%20%7D%0A%7D%0A%0Afn%20main()%20%7B%0A%20%20%20%20lint!(%22builder%22%2C%20%22some_warning%22)%3B%0A%20%20%20%20lint!(%22some_warning%22)%3B%0A%7D)

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");
lambda-fairy commented 4 years ago

@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).

CreepySkeleton commented 4 years ago

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.

teymour-aldridge commented 3 years ago

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!

jhpratt commented 3 years ago

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.

jhpratt commented 3 years ago

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.

m-ou-se commented 3 years ago

@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.)

lambda-fairy commented 3 years ago

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:

bjorn3 commented 3 years ago

It seems strange that Diagnostic::children returns impl Iterator, 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.

Aaron1011 commented 3 years ago

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.

jhpratt commented 3 years ago

@m-ou-se What was the outcome of the discussion?

pickfire commented 3 years ago

The Diagnostic API seemed to be missing span_suggestions, are we not looking into including suggestions API first?

jhpratt commented 3 years ago

@pickfire There's no reason that couldn't be done post-stabilization. Having something is better than nothing.

m-ou-se commented 3 years ago

What was the outcome of the discussion?

That we should probably just ask @dtolnay's opinion (who wasn't in the meeting this time).

@dtolnay What's your opinion here?

dtolnay commented 3 years ago

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. :slightly_frowning_face:

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):

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 ...
}
jhpratt commented 3 years ago

@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.

lambda-fairy commented 3 years ago

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.

bjorn3 commented 3 years ago

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.

https://github.com/rust-lang/miri/blob/277b59c0b373be0cd1c4448a7539329fa44a04ae/src/diagnostics.rs#L165

https://github.com/rust-lang/miri/blob/277b59c0b373be0cd1c4448a7539329fa44a04ae/src/diagnostics.rs#L280

SergioBenitez commented 3 years ago

@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 default
hello.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:

I am eagerly in favor of stabilizing an API resembling the above.

jhpratt commented 3 years ago

How difficult would lint-associated warnings be to implement? I've no idea how they're actually implemented internally.

dtolnay commented 3 years ago

I am on board with the API in https://github.com/rust-lang/rust/issues/54140#issuecomment-802701867. Would anyone be willing to send an implementation?

jhpratt commented 3 years ago

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?

pickfire commented 3 years ago

Why do we need mark and mark_all? The proposed API in https://github.com/rust-lang/rust/issues/54140#issuecomment-802701867 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?

jhpratt commented 3 years ago

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.

SergioBenitez commented 3 years ago

Why do we need mark and mark_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_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?

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 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?

A must_use would be great!

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?

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.

jhpratt commented 3 years ago

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.

pickfire commented 3 years ago

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.