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 for custom diagnostics in Error #224

Closed
AlisCode opened this issue Mar 7, 2023 · 5 comments · Fixed by #225
Closed

Support for custom diagnostics in Error #224

AlisCode opened this issue Mar 7, 2023 · 5 comments · Fixed by #225

Comments

@AlisCode
Copy link

AlisCode commented Mar 7, 2023

Hello! Great work on this crate.

First of all, I am aware of #175 being opened. I just don't know if this is the same issue, as the other one mentions non-fatal diagnostics, so I'm creating a new one. Feel free to close this if non-applicable.

I can see that there's support for diagnostics through a feature. Namely, did_you_mean will show alternatives as a help diagnostic.

I've been writing proc macros at work for a custom use case, and wanted to refactor said macro to use darling as we believe it brings value (easier maintainance, better readability). The macro currently has custom diagnostics, implemented through a custom error enum + a match block that leverags proc_macro_error to display error diagnostics. This is important to us, because it immediately clarifies what's wrong when working with our own macros.

One issue that's preventing us from refactoring with darling is the lack of custom diagnostics - I'm speaking about the possibility to attach a custom diagnostic to a darling Error like proc_macro_error does. The API to attach custom diagnostics could for example look like this :

Error::custom("My custom error").with_diagnostic("note", "My note on the macro usage").with_diagnostic("help", "Try doing this instead")

Which would then display the following error at compile-time :

error: My custom error
  --> my_project/my_file.rs:3:5
   |
13 |     FooBar { value: String },
   |     ^^^^^^
   |
   = note: My note on the macro usage
   = help: Try doing this instead

In terms of implementation, this would probably require adding a Vec of custom diagnostics at the Error level. I dont know how feasible this is, but I could have a go at implementing it if there's interest ?

@TedDriggs
Copy link
Owner

TedDriggs commented Mar 7, 2023

This sounds like a great proposal.

I'd recommend emulating the proc_macro API as far as having error, span_error, etc. methods - that avoids needing to expose any sort of enum for levels.

Some open questions:

  • What happens when a user invokes one of these on an error whose kind is ErrorKind::multiple? I'm leaning towards cloning the child diagnostics into each flattened error; that seems the least surprising and most useful behavior.
  • What is the behavior when the diagnostics feature is disabled? Are the methods cfg-gated, are they exposed but noops, or are they exposed and we try to pack the help lines into the limited message space we get without full diagnostics support? I've gone with making all the methods feature-gated, so that only people who opt into using nightly and this feature will see those APIs. I don't want to people to use these believing that they're more stable than they actually are.

TedDriggs added a commit that referenced this issue Mar 8, 2023
@TedDriggs TedDriggs mentioned this issue Mar 8, 2023
3 tasks
@TedDriggs
Copy link
Owner

@AlisCode I've put up #225 with a WIP on this. I'm not sure how to test this reliably; ideally we wouldn't have spurious CI failures due to bad nightlies, but there would be something that checks this periodically to make sure it's not broken. If you have any ideas there, they'd be much appreciated.

@AlisCode
Copy link
Author

AlisCode commented Mar 9, 2023

Thanks for the good work! I'll review the PR and give feedback.

ideally we wouldn't have spurious CI failures due to bad nightlies

I don't think that this is such a big concern because despite diagnostics being unstable, I haven't seen much changes on the feature (or its reliability thereof) in a long time. And the people working on it are probably very careful because diagnostics are actually used a lot. But anyway, I'll just assume that if you think this is a concern, you've had experiences with something similar in the past.

If you really think that's a big issue, one solution would be to pin the nightly version to a specific toolchain in the CI. Say today's for a start, because it seems to work fine. I see that the CI on this repo uses dtolnay/rust-toolchain, so from the documentation, this seems easy enough :

image

The concern I see with this is that the targetted nightly used for CI may be out of sync with the end user's. So maybe the workspace itself needs to be pinned to the same nightly using rust-toolchain.toml ? I have no strong opinion here.

@AlisCode
Copy link
Author

AlisCode commented Mar 9, 2023

Also, sorry for not answering your open questions. I agree with both of your answers to them, though :)

TedDriggs added a commit that referenced this issue Mar 9, 2023
When using the `diagnostics` feature, crate consumers can add custom
error, warning, note, and help messages to `Error` instances and have
those appear in the compiler's output.

Fixes #224
TedDriggs added a commit that referenced this issue Mar 9, 2023
When using the `diagnostics` feature, crate consumers can add custom
error, warning, note, and help messages to `Error` instances and have
those appear in the compiler's output.

Fixes #224
TedDriggs added a commit that referenced this issue Mar 9, 2023
When using the `diagnostics` feature, crate consumers can add custom
error, warning, note, and help messages to `Error` instances and have
those appear in the compiler's output.

Fixes #224
TedDriggs added a commit that referenced this issue Mar 9, 2023
When using the `diagnostics` feature, crate consumers can add custom
error, warning, note, and help messages to `Error` instances and have
those appear in the compiler's output.

Fixes #224
@TedDriggs
Copy link
Owner

This has been published as part of 0.14.4

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

Successfully merging a pull request may close this issue.

2 participants