Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support emitting warning/help/note diagnostics #16

Open
dhardy opened this issue Jan 23, 2024 · 9 comments
Open

Support emitting warning/help/note diagnostics #16

dhardy opened this issue Jan 23, 2024 · 9 comments

Comments

@dhardy
Copy link

dhardy commented Jan 23, 2024

This is unfortunately requires an unstable feature: see rust-lang/rust#54140

In other crates:

  • proc-macro-error emits such diagnostics on nightly but silently ignores them on stable
  • proc-macro2-diagnostics converts such diagnostics to errors when not on nightly. (Unfortunately it has a poor design for emitting a non-fatal diagnostic and continuing macro parsing/expansion.)
@dhardy
Copy link
Author

dhardy commented Jan 23, 2024

I see two approaches here:

  1. Accumulate non-fatal diagnostics somehow, perhaps using Emitter. This potentially enables conversion (though proc-macro-error's behaviour of ignoring non-fatal diagnostics on stable is slightly preferable as the default behaviour in my opinion).
  2. Emit immediately via Span::warning etc. when on nightly. In this case, an extension trait over proc_macro2::Span all that is needed, and technically this need not have anything to do with this library — but likely it is topical enough that it should be included.

@ModProg
Copy link
Owner

ModProg commented Jan 23, 2024

Yeah, this is something I want to reconsider, especially due to this being hopefully stabilized at some point.

One thing to consider would be to print the warning to stderr directly. This would not help IDE users but would show when running cargo build directly.

@ModProg
Copy link
Owner

ModProg commented Jan 23, 2024

#15 would be related for just improving errors using this as well.

@ModProg
Copy link
Owner

ModProg commented Jan 23, 2024

unsure about the api.

Something like

info!(span, "message")
warn!(span, "message")
not!(span, "message")

matching bail! could work

@ModProg
Copy link
Owner

ModProg commented Jan 23, 2024

I will at some point address this, but if someone wants to tackle this in the meantime, I'm open to suggestions and implementations.

@dhardy
Copy link
Author

dhardy commented Jan 23, 2024

Something like this maybe?

pub trait SpanDiagnostic {
    fn warning<T: Into<String>>(self, message: T) -> Diagnostic;
    // note, help ...
}
impl SpanDiagnostic for proc_macro2::Span {
    #[cfg(feature = "proc_macro_diagnostic")]
    fn warning<T: Into<String>>(self, message: T) -> Diagnostic {
        self.unwrap().warning(message)
    }

    #[cfg(not(feature = "proc_macro_diagnostic"))]
    fn warning<T: Into<String>>(self, message: T) -> Diagnostic {
        // ?
    }
}

This could easily be dropped later if proc_macro_diagnostic is stabilised and adopted into proc_macro2. But, admittedly, maybe this functionality should then be adopted into the proc_macro2 crate instead.

@dhardy
Copy link
Author

dhardy commented Jan 23, 2024

proc_macro2 is dtolnay's crate, and he tends to carefully consider what API should be added. Reading through his comments in the above-linked tracking issue, he favours this revision of the API, which it appears has yet to be implemented (haven't read the full thread). At the very least, dtolnay is aware of this.

@ModProg
Copy link
Owner

ModProg commented Jan 23, 2024

I think dtolnay will only add api to proc_macro2 where he expects stabilization, and diagnostics is probably still too vague.

The trait can definitely be added, but I think something more capable, e.g., with string formatting should be added as macros on top.

@dhardy
Copy link
Author

dhardy commented Jan 23, 2024

Sure, something like this could be added on top: warn!(span, "unused attribute: {}", attr.span())

Or, alternatively, something more complex (like I linked above or like proc_macro_error). I honestly don't care that much — simply having a facility for emitting warnings is the important part.

Eventually all these approaches would probably want to be changed anyway: a proper lint API should have some system for disabling certain lints and possibly also support attaching suggestions for rustfix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants