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

new chapter with examples of diagnostic translation PRs #1621

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tshepang
Copy link
Member

I found other examples overwhelming, as they tend to handle large parts of rustc crates, so decided to point to these more simple ones I created.

@JohnTitor JohnTitor added the S-waiting-on-review Status: this PR is waiting for a reviewer to verify its content label Mar 4, 2023
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Thanks for this. I think we could use this as an opportunity to provide a bit more "motivation" for the various parts of migrating these diagnostic structs if we add more information. Left a bunch of nits..

src/diagnostics/making-diagnostics-translatable.md Outdated Show resolved Hide resolved
src/diagnostics/making-diagnostics-translatable.md Outdated Show resolved Hide resolved
ecx.struct_span_err(span, "proc-macro derive produced unparseable tokens").emit();
```

> Note that `ecx.struct_span_err` is the [Session::struct_span_err].
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> Note that `ecx.struct_span_err` is the [Session::struct_span_err].
> Note that `ecx.struct_span_err` calls [Session::struct_span_err].

This should also probably link to ecx.struct_span_err (https://doc.rust-lang.org/nightly/nightly-rustc/rustc_expand/base/struct.ExtCtxt.html#method.struct_span_err) so that users can attest this relationship themselves.

Copy link
Member Author

@tshepang tshepang Mar 7, 2023

Choose a reason for hiding this comment

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

yeah, that should have been ultimately calls...

- Replace the code above with this:

```rust
ecx.sess.emit_err(errors::ProcMacroDeriveTokens { span });
Copy link
Member

Choose a reason for hiding this comment

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

Should point to emit_err in rustdocs. Also, useful to point out that if the user wants to make a diagnostic, but not emit it, they can use create_err.

Copy link
Member Author

Choose a reason for hiding this comment

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

tough balance between adding useful info and avoiding the overwhelm... I should find a way to fit create_err in the reference

ecx.sess.emit_err(errors::ProcMacroDeriveTokens { span });
```

- Create the above type, `errors::ProcMacroDeriveTokens`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Create the above type, `errors::ProcMacroDeriveTokens`,
- Add a diagnostic struct, `errors::ProcMacroDeriveTokens`,

Referring to the [general comments](#general-comments) section above,
follow these changes:

- Replace the code above with this:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Replace the code above with this:
- Replace the calls to `struct_span_err` and `emit` with:

Comment on lines +57 to +60
#[derive(Diagnostic)]
#[diag(expand_proc_macro_derive_tokens)]
pub struct ProcMacroDeriveTokens {
#[primary_span]
Copy link
Member

Choose a reason for hiding this comment

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

I think it may be useful to motivate the three important parts of this:

  • derive(Diagnostic)
  • diag(FLUENT_SLUG)
  • and primary_span

Maybe why we need these three components, what other components are common, etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

I more wanted to have people see the pattern, than do wall-of-text which can overwhelm. The more complete details are in the reference chapters (earlier in the guide).

@tshepang tshepang added S-waiting-on-author Status: this PR is waiting for additional action by the OP and removed S-waiting-on-review Status: this PR is waiting for a reviewer to verify its content labels Jul 7, 2023
@spastorino
Copy link
Member

@compiler-errors do you think this is fine to merge now?.

@jieyouxu
Copy link
Member

jieyouxu commented Nov 2, 2024

Update: cf rust-lang/rust#132181 (diag infra currently in limbo, no longer mandatory usage, but examples/clarifications will still be very helpful)

@jieyouxu jieyouxu added A-diagnostics Area: diagnostics A-translation Area: diagnostics translation and other translations T-compiler Relevant to compiler team labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: diagnostics A-translation Area: diagnostics translation and other translations S-waiting-on-author Status: this PR is waiting for additional action by the OP T-compiler Relevant to compiler team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants