Discussion/brainstorming: Non-fatal errors

[TedDriggs]

darling has a simple pass-fail approach for turning the AST into receiver structs. All of the darling traits return Result<Self, darling::Error>. This is great for approachability, but it does mean there's not a good place to put non-fatal diagnostics.

Some concrete examples:

1. It would be great to support #[darling(deprecated = "...")], enabling someone to deprecate a field in their macro as easily as they would a regular method.
2. In derive_builder the conflicting visibility declarations don't need to block code generation. Ideally there would be one error for "A field cannot be both public and private" without a second compiler error "LoremBuilder does not exist"

It's possible that these scenarios are best handled outside the existing system - since they don't block parsing, they could be implemented as functions that take the Receiver and return a Vec<Diagnostic>. That "feels" wrong though:

1. Someone who's using darling for derivation shouldn't need to remember to also call some other method to avoid losing non-fatal diagnostics.
2. Composing together types that impl FromMeta should "just work", including emitting non-fatal diagnostics when appropriate. If the non-fatal diagnostics are gathered in a separate pass, the author will need to remember to visit every field when gathering diagnostics or they'll miss things.
3. Deferring non-fatal diagnostics requires holding spans in the receiver struct. An examination of crates using darling suggests this rarely happens, with String and bool being very common receiver field types. As a result, non-fatal diagnostics would not have access to good spans.

There seem to be two ways this "could" work:

1. darling passes around a context of some sort, and FromMeta impls can add diagnostics into that context.
2. The return type for the darling traits is changed to be something like: DarlingResult<T> { value: Option<T>, diagnostics: Vec<Diagnostic> }. A fatal error would cause value to be None.

Both of these have problems.

Approach 1 would likely require introduction of parallel traits: FromMetaContext, FromDeriveInputContext, etc. Those wouldn't compose, since existing derivations of FromMeta wouldn't pass the context to their children when they call FromMeta.

Approach 2 would mean an enormous breaking change to the entire darling ecosystem, and despite being a 0.x crate, I'm extremely reluctant - bordering on outright unwilling - to do that.

That leaves the after-the-fact traversal. Maybe there's a way with drop bombs to make that workable: Make #[darling::post_validation] be an attribute macro that implements a trait and injects a drop-bomb field to ensure the trait implementation is called?

[TedDriggs]

Would adding a struct like the one below solve this?

pub struct WithDiagnostics<T> {
    pub value: T,
    pub diagnostics: Vec<Error>,
}

impl<T: FromMeta> FromMeta for WithDiagnostics<T> {
    fn from_meta(meta: &Meta) -> Result<Self> {
        Ok(Self {
            value: FromMeta::from_meta(meta)?,
            diagnostics: vec![],
        })
    }
}

impl<T: ToTokens> ToTokens for WithDiagnostics<T> {
    fn to_tokens(&self, tokens: &mut TokenStream) {
        self.value.to_tokens(tokens);
        for diagnostic in &self.diagnostics {
            // TODO add errors to token stream
        }
    }
}

Pros

1. The change is very localized and opt-in; code that doesn't have non-fatal errors won't need any changes
2. The presence of non-fatal diagnostics is represented in the receiver struct's type, which feels more idiomatic than a hidden mutable context.
3. This design supports both field-level checks and struct-level post-validation. The #[darling(with = "...")] attribute lets someone write a function that isolates the field-level checks, while post-validation can be done by pushing diagnostics into the vec after the struct has been built (easily done with #[darling(map = "...")].

Cons

1. A lot of darling projects have the receiver struct and the "codegen" struct be different; to avoid losing diagnostics this would require supporting several combinators. That also raises the question of whether this needs to be generic over the type of Error so that &Error can be used in its stead when creating a WithDiagnostics that borrows from value.
2. This design doesn't help structs that want to derive FromMeta and have struct-internal non-fatal validations. Those would likely need to do something more involved where they add a field #[darling(skip)] diagnostics: Vec<Error> to themselves, then populate that using #[darling(map = "...")].
3. I would currently prefer to use darling::Error for the diagnostics; using proc_macro::Diagnostic wouldn't work well because right now darling_core doesn't have a hard dependency on proc_macro, and introducing a darling::Diagnostic type feels like it could quickly get confusing. However, I'm hesitant to call this struct WithErrors if it can contain things that aren't errors.

[TedDriggs]

@AlisCode I'm curious to get your input on this one; does your company have situations where you want to add warnings/notes/etc. without adding an error? Would a new util type like WithDiagnostics be a convenient way for you to do that?

[AlisCode]

In the case of the macro I've been refactoring, we don't exactly separate the concepts of "receiver structs" and "meaningful/stronger-typed structs". We have our "meaningful" structs implement darling traits, namely FromVariant.

Any validation problem on any of the variants of the enum is fatal, so I assume that no, optional diagnostics arent really a thing we need. Not for that specific macro, anyway :eyes: .