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

Emit useful error when attrs field is used and forward_attrs is missing #317

Closed
joshka opened this issue Jan 13, 2025 · 1 comment · Fixed by #318
Closed

Emit useful error when attrs field is used and forward_attrs is missing #317

joshka opened this issue Jan 13, 2025 · 1 comment · Fixed by #318

Comments

@joshka
Copy link

joshka commented Jan 13, 2025

I ran across an error ("Errors were already checked") using FromDeriveInput that seems a little difficult to understand and doesn't seem to have a good explanation anywhere. I narrowed it down to when args is included in the struct. Full repro:

use darling::FromDeriveInput;
use proc_macro::TokenStream;
use syn::{parse_macro_input, Attribute, Ident};

#[proc_macro_derive(Hello)]
pub fn derive_hello(input: TokenStream) -> TokenStream {
    let input = parse_macro_input!(input as syn::DeriveInput);
    let _args = HelloArgs::from_derive_input(&input);
    TokenStream::new()
}

#[derive(FromDeriveInput)]
struct HelloArgs {
    ident: Ident,
    attrs: Vec<Attribute>, // This is the problem
}

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_derive_hello() {
        let input = r#"
            #[derive(Hello)]
            struct World;
        "#;
        let input = syn::parse_str(input).unwrap();
        let args = HelloArgs::from_derive_input(&input).unwrap();
        assert_eq!(args.ident, "World");
    }
}
---- tests::test_derive_hello stdout ----
thread 'tests::test_derive_hello' panicked at src/lib.rs:12:10:
Errors were already checked
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::option::expect_failed
   3: core::option::Option<T>::expect
             at /Users/joshka/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/option.rs:933:21
   4: <darling_error::HelloArgs as darling_core::from_derive_input::FromDeriveInput>::from_derive_input
             at ./src/lib.rs:12:10
   5: darling_error::tests::test_derive_hello
             at ./src/lib.rs:30:20
   6: darling_error::tests::test_derive_hello::{{closure}}
             at ./src/lib.rs:24:27
   7: core::ops::function::FnOnce::call_once
             at /Users/joshka/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
   8: core::ops::function::FnOnce::call_once
             at /rustc/9fc6b43126469e3858e2fe86cafb4f0fd5068869/library/core/src/ops/function.rs:250:5

I'm not sure if this is a bug in darling, I'm assuming that I'm probably using this wrong, but the error message doesn't make it easy to understand what do next. It's also problematic as it doesn't really point back to the attrs field as being the problem. Consider adding extra info about what this means and what is missing from the struct / config to make it correct, or documenting this in the docs (the only hit for this error message is the line of code where it's omitted).

@TedDriggs
Copy link
Owner

TedDriggs commented Jan 17, 2025

The problem here is that you didn't declare #[darling(attributes(...))] or #[darling(forward_attrs(...))]. This scenario is rare enough that it's probably a warning or error; darling isn't going to put anything into the array since you didn't ask for any attributes.

The error message is bad because we're hitting an expect. Searching for that string in the code, we're failing to get the name of the attrs field here.

Running cargo expand, the problem becomes pretty obvious:

#[automatically_derived]
#[allow(clippy::manual_unwrap_or_default)]
impl ::darling::FromDeriveInput for HelloArgs {
    fn from_derive_input(
        __di: &::darling::export::syn::DeriveInput,
    ) -> ::darling::Result<Self> {
        let mut __errors = ::darling::Error::accumulator();
        let mut __fwd_attrs: ::darling::export::Vec<::darling::export::syn::Attribute> = ::alloc::vec::Vec::new();
        let mut attrs: ::darling::export::Option<_> = None;
        __errors.finish()?;
        ::darling::export::Ok(HelloArgs {
            ident: __di.ident.clone(),
            attrs: attrs.expect("Errors were already checked"),
        })
    }
}

We declare let mut attrs as None and then immediately call expect on the None value. Compare that with the code generated if we add #[darling(forward_attrs)] to the input struct:

#[automatically_derived]
#[allow(clippy::manual_unwrap_or_default)]
impl ::darling::FromDeriveInput for HelloArgs {
    fn from_derive_input(
        __di: &::darling::export::syn::DeriveInput,
    ) -> ::darling::Result<Self> {
        let mut __errors = ::darling::Error::accumulator();
        let mut __fwd_attrs: ::darling::export::Vec<::darling::export::syn::Attribute> = ::alloc::vec::Vec::new();
        let mut attrs: ::darling::export::Option<_> = None;
        use ::darling::ToTokens;
        for __attr in &__di.attrs {
            match ::darling::export::ToString::to_string(
                    &__attr.path().clone().into_token_stream(),
                )
                .as_str()
            {
                _ => __fwd_attrs.push(__attr.clone()),
            }
        }
        attrs = ::darling::export::Some(__fwd_attrs);
        __errors.finish()?;
        ::darling::export::Ok(HelloArgs {
            ident: __di.ident.clone(),
            attrs: attrs.expect("Errors were already checked"),
        })
    }
}

The enhancement here would be to add a check in darling_core::ast to emit an error when attribute forwarding is expected (based on the attrs field being set) but no forwarding was specified.

@TedDriggs TedDriggs changed the title "Errors were already checked" error Emit useful error when attrs field is used and forward_attrs is missing Jan 17, 2025
TedDriggs added a commit that referenced this issue Jan 17, 2025
Previously, this would result in a cryptic error due to an expect(...) failure if neither `attributes` nor `forward_attrs` were declared.

This change is technically breaking, as it was previously possible to declare this field without using `forward_attrs`; if `attributes` was used the `attrs` field would be initialized to an empty array.
That behavior was more likely to mask issues than be useful, but it's possible someone declared the field in order to manually populate it later.

Fixes #317
TedDriggs added a commit that referenced this issue Jan 17, 2025
Previously, this would result in a cryptic error due to an expect(...) failure if neither `attributes` nor `forward_attrs` were declared.

This change is technically breaking, as it was previously possible to declare this field without using `forward_attrs`; if `attributes` was used the `attrs` field would be initialized to an empty array.
That behavior was more likely to mask issues than be useful, but it's possible someone declared the field in order to manually populate it later.

Fixes #317
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