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

Unsure if with_span within FromMeta is working #110

Closed
chipsenkbeil opened this issue Dec 30, 2020 · 7 comments
Closed

Unsure if with_span within FromMeta is working #110

chipsenkbeil opened this issue Dec 30, 2020 · 7 comments
Assignees
Labels

Comments

@chipsenkbeil
Copy link
Contributor

I'm porting my custom parsing of attributes over to darling and one such case is marking a field in the form:

struct MyEnt {
    #[ent(field)]
    field1: u32,

    #[ent(field(indexed))]
    field2: u32,

    #[ent(field(indexed, mutable))]
    field3: u32,

    #[ent(field(mutable))]
    field4: u32,
}

FromMeta derive attempt

Originally, I had this implementation:

#[derive(FromDeriveInput)]
struct Ent {
    // ...
    data: ast::Data<(), EntField>,
}

#[derive(FromField)]
#[darling(attributes(ent))]
struct EntField {
    // ...
    #[darling(default, rename = "field")]
    attr: Option<FieldAttr>,
}

#[derive(FromMeta)]
struct FieldAttr {
    indexed: bool,
    mutable: bool,
}

Manual implementation

But darling wouldn't support a standalone word of #[ent(field)], so I needed to implement it myself and check for each of the possible attributes:

Screen Shot 2020-12-30 at 3 04 32 PM

Not pointing to location of error

I'm using trybuild to compare my old output to the new before saving it. For the example below, I can't seem to get it to be more specific like I used to have.

Screen Shot 2020-12-30 at 3 14 34 PM

Screen Shot 2020-12-30 at 3 11 20 PM

@TedDriggs
Copy link
Owner

I’m traveling today but will take a look tomorrow. Do you have a public repo I can look at? If not, no worries

@chipsenkbeil
Copy link
Contributor Author

chipsenkbeil commented Dec 30, 2020

I just landed the unfinished migration to darling so you can take a look. The entity and entity_macros crates haven't been published outside of 0.0.0 to reserve space, but you can check out the examples in the macros lib.rs to see what I'm going for.

This is the file that has some of the code above: https://github.com/chipsenkbeil/entity-rs/blob/main/entity_macros/src/derive/ent/struct/data/internal.rs

I've been piecing together how to use parts of darling from reported issues and reading the source, so I may be approaching some of it incorrectly.

@TedDriggs
Copy link
Owner

TedDriggs commented Jan 4, 2021

I'm not seeing the exact error you shared when I run cargo +nightly test --all, though I am seeing some other errors from trybuild.

That said, after looking at your code my working theory is that this line and/or this line is causing the problem. darling::Error isn't directly convertible to syn::Error because darling::Error can contain multiple errors, each of which may or may not have an associated Span.

Referencing the darling docs because my own memory on this is a bit fuzzy, it appears the right way to turn these into compile errors is to use the darling::Error::write_errors method, when features = ["diagnostics"] is enabled for darling in Cargo.toml.

I started trying to test this theory, but there are a lot of references to syn::Error in the code that made it hard for me to do so without a sizable change. You could switch to using darling::Error pervasively (which would have the side benefit of letting you collect all the errors on a user's input then returning them at once, e.g. these), or you could make a custom error enum which allowed darling::Error as the value of one of its branches.

@TedDriggs TedDriggs self-assigned this Jan 4, 2021
@chipsenkbeil
Copy link
Contributor Author

@TedDriggs, sorry, I had to move forward after giving you that link, so code moved around a bit. I also switched to having the UI tests only run on nightly, but the problem I was seeing was with stable. The code still runs on stable (minimum version 1.45.0), but these tests are now limited to nightly as that's what I've seen others using trybuild do.

As it turns out, I was trying to use the darling Custom error type to populate a specific message and span. From there, after running from_derive_input, I wanted convert the darling error to the string message and use its span when building a syn::Error.

As you mentioned, there can be more than one darling error. My hope was to be able to grab the first darling error and use its span for a more specific location, but from what I could see if the error code there is no way to get a span back out once it's inserted.

So my result is wrapping the darling errors in a single syn error that uses the display output from darling as its message and the entire derive input's span as the error's span: https://github.com/chipsenkbeil/entity-rs/blob/main/entity_macros/src/derive/ent/struct/data/mod.rs#L100

@TedDriggs
Copy link
Owner

If you do that then you'll definitely get the unwanted behavior described in your original issue report. Going from syn::Error into darling::Error instead (via darling::Error::custom) would avoid losing that information, since it would allow darling to retain all the separate spans and errors up to the point at which they need to be emitted.

It doesn't appear there's a problem in darling here, so I'm going to close this issue. That said, it might make sense to improve documentation on why darling::Error doesn't convert cleanly to syn::Error and why it doesn't expose its spans. Would you have seen a section in the darling::Error docs if one existed?

@chipsenkbeil
Copy link
Contributor Author

chipsenkbeil commented Jan 4, 2021

If you do that then you'll definitely get the unwanted behavior described in your original issue report. Going from syn::Error into darling::Error instead (via darling::Error::custom) would avoid losing that information, since it would allow darling to retain all the separate spans and errors up to the point at which they need to be emitted.

It doesn't appear there's a problem in darling here, so I'm going to close this issue. That said, it might make sense to improve documentation on why darling::Error doesn't convert cleanly to syn::Error and why it doesn't expose its spans. Would you have seen a section in the darling::Error docs if one existed?

If there was documentation for it, I believe I would have seen it. I've been reading through the documentation available and filling in gaps by reading through issues and code. E.g. I don't believe that #[darling(multiple)] is documented, or if it is I did not find it. But reading an issue, I saw it mentioned. And I believe one example or test also used it.

More documentation on this matter is definitely a positive in my eyes! 😄 Any guidance that can be captured regarding ways to approach this would also be helpful. Personally, I'd already written my code with bubbling up syn::Error in mind, so I wanted to avoid having to rewrite it all to use darling::Error. I don't know if I'm in the minority or not with this opinion, but addressing it in some way would be great.

@TedDriggs
Copy link
Owner

Cool, in that case I'll put a little guide in the error. I need to look at trybuild more closely; it didn't exist when I started, and as a result some of the tests and docs for this are less robust than I'd like.

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

No branches or pull requests

2 participants