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

Add derive macro for specifying diagnostics using attributes. #75138

Merged
merged 3 commits into from
Sep 8, 2020

Conversation

jumbatm
Copy link
Contributor

@jumbatm jumbatm commented Aug 4, 2020

Introduces #[derive(SessionDiagnostic)], a derive macro for specifying structs that can be converted to Diagnostics using directions given by attributes on the struct and its fields. Currently, the following attributes have been implemented:

  • #[code = "..."] -- this sets the Diagnostic's error code, and must be provided on the struct iself (ie, not on a field). Equivalent to calling code.
  • #[message = "..."] -- this sets the Diagnostic's primary error message.
  • #[label = "..."] -- this must be applied to fields of type Span, and is equivalent to span_label
  • #[suggestion(..)] -- this allows a suggestion message to be supplied. This attribute must be applied to a field of type Span or (Span, Applicability), and is equivalent to calling span_suggestion. Valid arguments are:
    • message = "..." -- this sets the suggestion message.
    • (Optional) code = "..." -- this suggests code for the suggestion. Defaults to empty.

suggestionalso comes with other variants: #[suggestion_short(..)], #[suggestion_hidden(..)] and #[suggestion_verbose(..)] which all take the same keys.

Within the strings passed to each attribute, fields can be referenced without needing to be passed explicitly into the format string -- eg, #[error = "{ident} already declared"] will set the error message to format!("{} already declared", &self.ident). Any fields on the struct can be referenced in this way.

Additionally, for any of these attributes, Option fields can be used to only optionally apply the decoration -- for example:

#[derive(SessionDiagnostic)]
#[code = "E0123"]
struct SomeKindOfError {
    ...
    #[suggestion(message = "informative error message")]
    opt_sugg: Option<(Span, Applicability)>
    ...
}

will not emit a suggestion if opt_sugg is None.

We plan on iterating on this macro further; this PR is a start.

Closes #61132.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2020
Comment on lines 835 to 842
struct_span_err!(
tcx.sess,
f.span,
E0124,
"field `{}` is already declared",
f.ident
)
.span_label(f.span, "field already declared")
.span_label(prev_span, format!("`{}` first declared here", f.ident))
.emit();
#[derive(SessionDiagnostic)]
#[code = "E0124"]
struct FieldAlreadyDeclared {
field_name: String,
#[error = "field `{field_name}` is already declared"]
#[label = "field already declared"]
span: Span,
#[label = "`{field_name}` first declared here"]
prev_span: Span,
}

tcx.sess.emit_err(FieldAlreadyDeclared {
field_name: f.ident.to_string(),
span: f.span,
prev_span,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an existing diagnostic I've replaced, which demonstrates an actual usage of the derive macro.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @rust-lang/wg-diagnostics have a look at this epic PR

@jumbatm I'm a bit unsure about the "type checks" in general. I have commented where I think they can be removed in favour of other schemes, but e.g. the Option example is a bit more complex. We may be able to work around this by creating a MaybeOption trait that just doesn't do anything for None:

trait MaybeOption<T> {
    fn maybe_do(self, f: impl FnOnce(T));
}

impl<T> MaybeOption<T> for T {
    fn maybe_do(self, f: impl FnOnce(T)){
        f(self)
    }
}

impl<T> MaybeOption<T> for Option<T> {
    fn maybe_do(self, f: impl FnOnce(T)) {
        if let Some(this) = self {
            f(this)
        }
    }
}

src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved
src/librustc_typeck/collect.rs Outdated Show resolved Hide resolved
src/librustc_macros/src/session_diagnostic.rs Outdated Show resolved Hide resolved
src/librustc_macros/src/session_diagnostic.rs Outdated Show resolved Hide resolved
src/librustc_macros/src/session_diagnostic.rs Outdated Show resolved Hide resolved
src/librustc_macros/src/session_diagnostic.rs Outdated Show resolved Hide resolved
src/librustc_macros/src/session_diagnostic.rs Outdated Show resolved Hide resolved
return Ok((span, applicability));
}
}
throw_span_err!(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just expect a specific trait to be implemented for these fields (and give that trait an on_unimplemented attribute for a targetted diagnostic). Then you can invoke ThatTrait::get_span(#binding) and ThatTrait::get_applicability. ThatTrait could be implemented for various combinations of Span and Applicability or the lack of the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all in favour of removing the name-based type checks in favour of just letting custom traits do the heavy lifting. However, doing this, the emitted error points at the #[derive(SessionDiagnostic)], even with quote_spanned! (as with the other case):

https://github.com/jumbatm/rust/blob/0254a1831fd926207b51bc87cd9deb3de7e5ee5e/src/test/ui-fulldeps/session-derive-errors.stderr#L164-L171

On having an on_unimplemented method: wouldn't I need to generate code which calls this method, meaning I'd be emitting the customised diagnostic at runtime? Using compile_error! wouldn't do the trick, either, that would always cause a compile error (regardless of the flow at runtime).

src/librustc_macros/src/session_diagnostic.rs Outdated Show resolved Hide resolved
src/librustc_macros/src/session_diagnostic.rs Outdated Show resolved Hide resolved
@estebank
Copy link
Contributor

estebank commented Aug 4, 2020

Haven't looked at this yet, but it will be interesting to be able to allow us to impl the derived trait by hand in cases where we want more advanced behavior, like changing the output or wording depending on span ordering to give an example.

@bors
Copy link
Contributor

bors commented Aug 15, 2020

☔ The latest upstream changes (presumably #73851) made this pull request unmergeable. Please resolve the merge conflicts.

@jumbatm jumbatm force-pushed the session-diagnostic-derive branch 3 times, most recently from 414ea88 to ebec567 Compare August 27, 2020 10:39
@oli-obk
Copy link
Contributor

oli-obk commented Aug 27, 2020

Ok, this looks ready enough to me, I think we should iterate further on it, but we should merge it now and iterate in future PRs. Can you move it out of draft and squash the commits?

@jumbatm jumbatm marked this pull request as ready for review August 28, 2020 04:26
@jumbatm
Copy link
Contributor Author

jumbatm commented Aug 28, 2020

Sure thing. This should be good to go once green.

@bors
Copy link
Contributor

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 7, 2020

@bors r+

ok, let's get this rolling. Thanks for your awesome work!

@bors
Copy link
Contributor

bors commented Sep 7, 2020

📌 Commit 5956254 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 7, 2020
…r=oli-obk

Add derive macro for specifying diagnostics using attributes.

Introduces `#[derive(SessionDiagnostic)]`, a derive macro for specifying structs that can be converted to Diagnostics using directions given by attributes on the struct and its fields. Currently, the following attributes have been implemented:
- `#[code = "..."]` -- this sets the Diagnostic's error code, and must be provided on the struct iself (ie, not on a field). Equivalent to calling `code`.
- `#[message = "..."]` -- this sets the Diagnostic's primary error message.
- `#[label = "..."]` -- this must be applied to fields of type `Span`, and is equivalent to `span_label`
- `#[suggestion(..)]` -- this allows a suggestion message to be supplied. This attribute must be applied to a field of type `Span` or `(Span, Applicability)`, and is equivalent to calling `span_suggestion`. Valid arguments are:
    - `message = "..."` -- this sets the suggestion message.
    - (Optional) `code = "..."` -- this suggests code for the suggestion. Defaults to empty.

`suggestion`also  comes with other variants: `#[suggestion_short(..)]`, `#[suggestion_hidden(..)]` and `#[suggestion_verbose(..)]` which all take the same keys.

Within the strings passed to each attribute, fields can be referenced without needing to be passed explicitly into the format string -- eg, `#[error = "{ident} already declared"] ` will set the error message to `format!("{} already declared", &self.ident)`. Any fields on the struct can be referenced in this way.

Additionally, for any of these attributes, Option fields can be used to only optionally apply the decoration -- for example:

```rust
#[derive(SessionDiagnostic)]
#[code = "E0123"]
struct SomeKindOfError {
    ...
    #[suggestion(message = "informative error message")]
    opt_sugg: Option<(Span, Applicability)>
    ...
}
```
will not emit a suggestion if `opt_sugg` is `None`.

We plan on iterating on this macro further; this PR is a start.

Closes rust-lang#61132.

r? @oli-obk
@bors
Copy link
Contributor

bors commented Sep 8, 2020

⌛ Testing commit 5956254 with merge 71569e4...

@bors
Copy link
Contributor

bors commented Sep 8, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 71569e4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 8, 2020
@bors bors merged commit 71569e4 into rust-lang:master Sep 8, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 8, 2020
@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make diagnostics emitting independent of the happy code path
8 participants