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

Implement new proc macro diagnostics API #83363

Closed
wants to merge 4 commits into from

Conversation

jhpratt
Copy link
Member

@jhpratt jhpratt commented Mar 22, 2021

See #54140 (comment) for details.

In this PR, I remove the existing MultiSpan trait, replacing it with the proposed Spanned trait. The Diagnostic API has been rebuilt from the base struct.

The Level enum still needs to be removed, as it's not used anywhere in the public API.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2021
@rust-log-analyzer

This comment has been minimized.

@jhpratt

This comment has been minimized.

@rustbot rustbot added A-proc-macros Area: Procedural macros S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2021
@jhpratt

This comment has been minimized.

@rustbot

This comment has been minimized.

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 22, 2021
@pickfire

This comment has been minimized.

@jhpratt

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jhpratt

This comment has been minimized.

@petrochenkov
Copy link
Contributor

This needs an API review from T-libs before a compiler implementation review.
r? @dtolnay

Also cc @matklad regarding incremental proc macros.
Some span-related methods are not stabilized because they allow proc macros to inspect absolute positions in files, maybe the diagnostic API has some similar hazards.

@jhpratt

This comment has been minimized.

@JohnCSimon
Copy link
Member

@jhpratt
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Nov 27, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Nov 27, 2022
@jhpratt
Copy link
Member Author

jhpratt commented Nov 27, 2022

I am ready to continue with it. There was just concerns regarding the exact API, which is unfortunate. It's an issue for T-libs-api, ultimately.

Thank you for the reminder of GitHub's quirk. I was aware of it previously, but a reminder never hurts.

@JohnCSimon JohnCSimon reopened this Nov 27, 2022
@JohnCSimon
Copy link
Member

@jhpratt ok I'll reopen it. We have a lot of dead PRs.

@jhpratt
Copy link
Member Author

jhpratt commented Nov 27, 2022

Oh, I understand 100%. It just needs a decision from T-libs-api to my recollection. I am more than happy to rebase and have this ready to merge if it's desired.

cc @rust-lang/libs-api — any chance of giving this another look?

@Amanieu Amanieu added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Dec 1, 2022
@m-ou-se m-ou-se assigned m-ou-se and unassigned yaahc Dec 13, 2022
@dtolnay dtolnay assigned dtolnay and unassigned m-ou-se Dec 13, 2022
@estebank
Copy link
Contributor

Hi, @rust-lang/libs! Would it be possible to get a review of this PR going? I would like to see us make progress in stabilizing (at least a part of) the Diagnostic API sometime before edition 2024 comes out.

@dtolnay
Copy link
Member

dtolnay commented Mar 22, 2023

I was supposed to look at this but syn 2-related work has been more important so far. Anyone else should feel free to weigh in with their feedback.

@jhpratt
Copy link
Member Author

jhpratt commented Mar 23, 2023

With regard to the implementation itself (given the team pinged), it's been so long that a rebase will be significant effort certainly involving nontrivial changes. It's more the API that I'd like to see agreed upon first — the implementation would naturally follow.

Given the duration, it may make sense to redo the implementation from scratch. Not certain on that, though.

Edit: For clarity, this is speculation on my end based on previous experience with old PRs. A rebase may in fact be straightforward.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 28, 2023

Is there an accurate API overview for this change? That'd make it a lot easier to discuss the proposed API. :)

@m-ou-se
Copy link
Member

m-ou-se commented Mar 28, 2023

It seems that this is the API introduced by this PR:

pub trait Spanned {
    type Iter: Iterator<Item = Span>;
    fn spans(self) -> Self::Iter;
    fn span(self) -> Span where Self: Sized;
    fn error(self, msg: &str) -> Diagnostic where Self: Sized;
    // no warning
    fn note(self, msg: &str) -> Diagnostic where Self: Sized;
}

impl<S: Spanned, I: IntoIterator<Item = S>> Spanned for I;
impl Spanned for Span;
impl Spanned for Group;
impl Spanned for Ident;
impl Spanned for Literal;
impl Spanned for Punct;
impl Spanned for TokenTree;

#[must_use]
struct Diagnostic {}

impl Diagnostic {
    pub fn error(message: &str) -> Self;
    // no warning
    pub fn note(message: &str) -> Self;
    pub fn with_help(mut self, message: &str) -> Self;
    pub fn with_note(mut self, message: &str) -> Self;
    pub fn mark(mut self, item: impl Spanned) -> Self;
    pub fn mark_all(mut self, item: impl Spanned) -> Self;
    pub fn label(mut self, item: impl Spanned, message: &str) -> Self;
    pub fn emit(self);
}

In this comment, it was noted that:

  • This API allows for diagnostics without a span, which might be fine.
  • We might want to change &str to Into<String> or something similar.
  • We probably want to add &mut self methods (add_help, add_note, etc.) too.
  • This doesn't support suggestions yet.
  • A lint mechanism would be useful for warnings, so they can be silenced with #[allow()].

@m-ou-se
Copy link
Member

m-ou-se commented Mar 28, 2023

I will say I'm personally not fully convinced this Spanned trait (in this form) is a good idea. Calling .error() and .note() on a Span doesn't seem unreasonable, but calling things like .error() and .note() on a Literal or on an iterator seems a bit surprising and maybe even confusing.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 28, 2023

The difference between mark and mark_all isn't very clear, as they both have the exact same signature. Does .mark([span_a, span_b]) not do the same as .mark_all([span_a, span_b])?

@m-ou-se
Copy link
Member

m-ou-se commented Mar 28, 2023

mark/mark_all don't seem to have an equivalent on rustc's internal Diagnostic api. What does/should they do exactly? In the implementation in this PR, they seem to add spans to MultiSpan::primary_spans, but in the description in this comment describes "secondary" spans, which sound more like the equivalent of an empty .label(span, "")?

@m-ou-se m-ou-se assigned m-ou-se and unassigned dtolnay Mar 28, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Mar 28, 2023

I found it useful to have a little overview of the current structure a Diagnostic in rustc itself. The current structure is roughly:

Diagnostic {
    level,
    message,
    code: Option<error or lint name>,
    primary_spans: [Span],
    span_labels: [(Span, String)],
    children: [
        SubDiagnostic {
            level,
            message,
            primary_spans: [Span],
            span_labels: [(Span, String)],
        }
    ],
    suggestions: [
        Suggestion {
            substitutions: [[(Span, String)]],
            message,
            style,
            applicability,
        }
    ],
}

(The actual type also supports localisation (with placeholders) for the strings, and styling for the messages.)

@m-ou-se
Copy link
Member

m-ou-se commented Mar 28, 2023

Esteban and I came up with an alternative idea. See this comment. Curious to hear what you think.

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label May 30, 2023
@Mark-Simulacrum
Copy link
Member

It looks to me like discussion on #54140 is/was ongoing since Mara's comment and there was positive reception to a different API shape than this PR currently implements. I'm going to go ahead and close as such. I think this can be reopened (or perhaps a new PR, to avoid running into github limitations on hiding comments to the extent possible) if there's a ready proposal for API shape that is positively received (not sure based on quick skim of the issue thread).

@jhpratt
Copy link
Member Author

jhpratt commented Jan 20, 2024

I'm not sure calling it "positive reception" is entirely fair, as there were significant questions raised, including some by myself that were never even remotely addressed.

Please don't take this personally, but this PR has been extremely frustrating. I opened it nearly three years ago, expecting it to be approved quite quickly as the general shape was agreed upon before I even began implementation. After multiple follow-ups, rebases, and trying to get the necessary reviews, it's just outright closed with what I consider a mischaracterization of people's feelings.

This is why people are hesitant to contribute to the standard library. It's not that technically challenging. It's that it's damn near impossible to get anything nontrivial done unless you're on a team, and joining teams has no apparent path. Even when things can get done, it typically takes at least a month to get even a preliminary review, with a similar timeframe for subsequent reviews. People want to do things, but the process is extraordinarily unwelcoming at every step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-proc-macros Area: Procedural macros S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.