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

Give a friendly error for enums with fields #45

Merged
merged 3 commits into from May 30, 2021
Merged

Conversation

illicitonion
Copy link
Owner

Right now we generate invalid code, and the error message is pretty
inscrutable. Instead, generate a nice descriptive user-facing error.

Right now we generate invalid code, and the error message is pretty
inscrutable. Instead, generate a nice descriptive user-facing error.
Copy link
Collaborator

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

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

LGTM ✅

let fields = variant.fields;

match fields {
Fields::Named(_) | Fields::Unnamed(_) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tiny nit: since tuple variants don't really have fields / or rather, it may not be seen as such by some users, I'd replace the fields mention for Fields::Unnamed with something else. Maybe something like associated data / payloads, or even simpler: "only unit variants are supported". Since this is subjective, feel free to disregard this 🙂

Copy link
Owner Author

Choose a reason for hiding this comment

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

Great feedback - thanks!

Comment on lines 251 to 255
let mut span = variant.span();
// Sadly `Span::join` is a nightly-only API at the moment - see https://github.com/rust-lang/rust/issues/54725
if let Some(span_with_fields) = span.join(variant.fields.span()) {
span = span_with_fields;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW, if we were to use Error::new_spanned(variant over Error::new(variant.span() (the die! macro currently leads us to the latter; maybe we could try to feature a die_spanned_at variant for the former), then we'd have a correctly spanned error message, even on stable Rust. Indeed, new_spanned uses a hack to achieve this:

// imbued with the _start_ of the span
// vvvvvvvvvvvvvv
   compile_error! { "…" }
//                ^^^^^^^
//                imbued with the _end_ of the span

effectively yielding an error which does span over the whole range. Demo using new_spanned(variant vs. new(variant.span():

Screen Shot 2021-05-30 at 11 44 36

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ooh, TIL! This is amazing - done - thanks for the suggestion!

@illicitonion illicitonion merged commit d0ac4cd into main May 30, 2021
@illicitonion illicitonion deleted the error-on-fields branch May 30, 2021 15:28
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

Successfully merging this pull request may close these issues.

None yet

2 participants