Pass tokens that failed to parse as Expr to FromMeta::from_verbatim

[nik-rev]

This PR adds a new FromMeta::from_invalid_expr which accepts an arbitrary TokenStream as an input. This can be used to parse arbitrary code that fails to parse as a syn::Expr. For example, this failed to parse previously:

#[method(vis = pub(crate))]

You used to have to use quotation marks for that:

#[method(vis = "pub(crate)")]

Because pub(crate) is not a valid expression. Now, it will call the from_invalid_expr method, passing it the TokenStream corresponding to the tokens pub(crate) because we failed to parse pub(crate) as an expression

Also, syn::Type, syn::Type*, syn::Visibility and syn::WhereClause don't require quotation marks anymore

Closes #378

[TedDriggs]

I am very impressed with this change, but also very nervous.

My big fear is that this will somehow degrade darling errors in common cases, such as imbalanced parentheses, brackets, or braces. I don't know exactly how that would manifest, since until now I haven't looked closely at the error experiences when parsing into the syn::Meta.

I would be interested to see some before/after comparisons of the errors you get from darling with these changes in place.

Option A: Add more compile-fail tests

This is the higher-effort option. Add some tests like trying to put a Type that's not an Expr in the RHS position of a Meta, and commit the generated errors. Then reapply this change and regenerate the error messages, so that this PR can show the change in the errors.

Option B: Provide some examples loose in the PR comments

Since we've not had any previous expectations about behavior here, just showing some examples of how it changes would be good for assuaging my immediate fears. We can then add some tests later that capture the behavior in more detail.

[nik-rev]

I found a way to implement this feature without making the errors any worse

My big fear is that this will somehow degrade darling errors in common cases

The initial version of this PR just disregarded the error completely

But I have found an approach that's essentially the best of both worlds. Instead of ignoring the error, we store it, and then:

• If FromMeta::from_verbatim is not implemented, then the original syn::Error from parsing the expression is surfaced to the user
• If FromMeta::from_verbatim is implemented, the users error gets rendered

Representation

When we come across an invalid expression:

#[args(tag = <br />, foo = 10)]
             ^^^^^^

• We take all of the input until the comma (,) and store it.
• We take the syn::Error that we got from trying to parse this input, convert it into a TokenStream (with syn::Error::into_token_stream()), and store it.

We get something like this:

let input = quote! {
    <br />
};
let error = quote! {
    compile_error! { "invalid expression" }
};

We must store the above 2 arbitrary TokenStreams in a single syn::Expr. The first idea is to use Expr::Verbatim:

Verbatim(TokenStream)

We could store our input and error, each in their own TokenTree::Group:

Expr::Verbatim(quote! { (#input) (#error) })

We could then parse this to get our input and error back.

There are 2 problems with that approach:

• It's totally possible that syn::Expr::parse() creates Expr::Verbatim, for example if a new Rust version releases that adds a new kind of expression, and syn doesn't want to update its major version just yet (because e.g adding a new variant would be a breaking change) - syn can parse this into Expr::Verbatim
• Doing it this way requires that we .clone() the entire stream, unconditionally. So anytime someone would write something that doesn't parse into a syn::Expr, all the tokens would be cloned - because a &TokenStream is a completely opaque object, it cannot be inspected at all. You must call .into_iter() to extract those errors and inputs, which requires ownership.

That's why I opted for a different representation:

let input = quote! {
    <br />
};
let error = quote! {
    compile_error! { "invalid expression" }
};

Expr::Macro(parse_quote! {
    #[__darling_expr_verbatim(
        #error
    )]
    __darling_expr_verbatim! {
        #input
    }
})

Inside of FromMeta::from_expr, if we get this representation, we will extract the input and error from it, without having to do any .clone()ing:

fn from_expr(expr: &Expr) -> Result<Self> {
    match *expr {
        // ...
        _ => {
            if let Some((input, error)) = expr_verbatim::decode(expr) {
                // `input` is the tokens we failed to parse, and `error` is
                // the `TokenStream` corresponding to the rendered `syn::Error` that we got
                // when we failed to parse `input` as an expression
            } else {
                Err(Error::unexpected_expr_type(expr))
            }
        }
    }
}

That expr_verbatim::decode only returns Ok if the representation is exactly what I described above.

Once we have this input and the error, we call FromMeta::from_verbatim, which has the following signature:

fn from_verbatim(tokens: &TokenStream) -> Option<Result<Self>>;

If this method returns Some(..), this type implementing FromMeta supports arbitrary input, and can return an arbitrary error.

If this method returns None, as is the default implementation - then this does does not support arbitrary input, and we surface the expression error back to the user:

if let Some((input, error)) = expr_verbatim::decode(expr) {
    if let Some(handled) = Self::from_verbatim(input) {
        input
    } else {
        Err(Error::from_syn_rendered(error.clone()))
    }
} else {
    Err(Error::unexpected_expr_type(expr))
}

That Error::from_syn_rendered takes rendered compile_error! invocations like these:

compile_error! { "unexpected expr" }
compile_error! { "something went wrong" }

And parses them into a darling::Error

[TedDriggs]

We must store the above 2 arbitrary TokenStreams in a single syn::Expr.

Why do we have this restriction?

If we failed to parse to an Expr, we call from_verbatim with the offending TokenStream in lieu of ever calling from_expr. I don't understand when we'd end up needing to pack this back into an expression.

Note that in general, darling is willing to endure some clones for ergonomics (note that the core trait FromMeta forces clones to retain parsed data).

[nik-rev]

We must store the above 2 arbitrary TokenStreams in a single syn::Expr

Why do we have this restriction?

Because these arbitrary tokens are parsed inside of parse_meta_name_value_after_path:

fn parse_meta_name_value_after_path<'a>(
    path: Path,
    input: ParseStream<'a>,
) -> syn::Result<MetaNameValue> {

This type requires the value to be an Expr:

pub struct MetaNameValue {
    pub path: Path,
    pub eq_token: Token![=],
    pub value: Expr,
}

We try to parse the RHS into an expression here:

match input.parse() {
    Ok(expr) => expr,
    // This isn't a valid expression, it might be something like `pub(in crate::module)`
    // so we want to save that and let the user parse it (e.g. into a [`syn::Visibility`]).
    //
    // For more details, see docs of `darling::util::decode_if_verbatim`
    Err(err) => {
        let error: TokenStream = err.into_compile_error();
        let verbatim_input: TokenStream = ...;

        expr_verbatim::encode(verbatim_input, Some(error))
    }
}

This expression (the whole match) must evaluate to an Expr, and we want to store both the error and the input TokenStreams in that singular Expr.

If we failed to parse to an Expr, we call from_verbatim with the offending TokenStream in lieu of ever calling from_expr. I don't understand when we'd end up needing to pack this back into an expression.

In order to call from_verbatim, we must first return from that parse_meta_name_value_after_path function. We can't return from it if we don't construct a syn::Expr.

After all, we are basically trying to support something like this:

#[html(tag = <br />)]
             ^^^^^^

To do that we need to return a MetaNameValue from parse_meta_name_value_after_path like this:

MetaNameValue {
    path: parse_quote!(tag),
    eq_token: Token![=],
    value: parse_quote!(<br />),
}

But since <br /> is not a valid expression, we store its TokenStream in a custom way. We also need to store the error which means storing 2 arbitrary TokenStreams

Note that in general, darling is willing to endure some clones for ergonomics (note that the core trait FromMeta forces clones to retain parsed data).

In this case there won't be an ergonomic advantage to storing this in Expr::Verbatim over Expr::Macro. We will still need to have these 2 functions:

/// Encodes an arbitrary stream of tokens + error stream into an `syn::Expr`
pub(crate) fn encode(input: TokenStream, error: Option<TokenStream>) -> Expr

/// Decodes a darling verbatim expression into the user's input + error that describes
/// why the expression failed to parse
pub fn decode(maybe_verbatim: &syn::Expr) -> Option<(&TokenStream, &TokenStream)>

The only thing that will be different is internal representation of that Expr

[TedDriggs]

I have general concerns about this, and think we may need to discuss pros and cons of different approaches before continuing review of a specific PR.

[TedDriggs]

This current design feels dangerous.

Up until now, parsing has not produced a syn::Meta that would output different tokens than one parsed by syn itself. All previous discussions have been limited to changes around whether keywords are considered idents, which didn't affect output code.

This seems to change that; attempting to output the Meta could now produce tokens that are different than the input and that won't compile.

I fear this may push us to having a darling::ast::Meta so we can store this information in a safer way. That's a huge breaking change though.

[nik-rev]

I fear this may push us to having a darling::ast::Meta so we can store this information in a safer way. That's a huge breaking change though.

What about if we add a new variant to NestedMeta? This should break people who do a full match on NestedMeta but the impact should be minimal.

There are 700 usages of NestedMeta on GitHub: https://github.com/search?q=%2Fdarling%3A%3Aast%3A%3ANestedMeta%2F&type=code

I have opened about 40 of those and I didn't find a single match on a NestedMeta that would be broken by adding a new variant. Typical usages look like this:

        let VisibilityAttrInternal {
            public,
            private,
            vis: explicit,
        } = VisibilityAttrInternal::from_list(items)?;

        let Inner {
            array,
            count,
            nul_terminated,
            opaque,
            socket,
        } = Inner::from_list(items)?;

       let args = darling::ast::NestedMeta::parse_meta_list(input.into())
            .map_err(|e| syn::Error::new(proc_macro2::Span::call_site(), e))?;

        let values = items
            .iter()
            .map(|item| match item {
                darling::ast::NestedMeta::Meta(meta) => PickArgs::from_meta(meta),
                _ => Err(darling::Error::unexpected_type("non meta").with_span(item)),
            })
            .collect::<darling::Result<Vec<PickArgs>>>()?;

Most usages of NestedMeta out there seem to just pass it to another function. If any pattern match it, they're usually in it for the NestedMeta::Meta and don't do a comprehensive match

I believe if we added a new variant we could represent this in a safe way, without causing too much of a headache for people. If we replaced syn::Meta that would be a huge change because it would need to be updated in all 1.1k places that it is used on GitHub. However, adding a new variant to NestedMeta might only require a few dozen updates

We can add a variant like this:

pub enum NestedMeta {
    Meta(syn::Meta),
    Lit(syn::Lit),
    VerbatimMetaNameValue {
        path: Path,
        eq_token: Token![=],
        // the original input
        input: TokenStream,
        // error that resulted from trying to parse into an Expr
        error: syn::Error
    }
}

We can of course make that an opaque object like VerbatimMetaNameValue(VerbatimMetaNameValue) and then provide accessor methods on it. We also won't need to render/parse the syn::Error and can just directly store it then turn into darling::Error

[TedDriggs]

Given this code:

#[derive(FromField)]
#[darling(attributes(outer))]
struct Field {
    thing: Thing
}

#[derive(FromMeta)]
struct Thing {
    arg: usize,
    inner: Inner,
}

#[derive(FromMeta)]
struct Inner {
    other: syn::LitStr,
    vis: syn::Visibility
}

For the input:

#[outer(thing(
    arg = 4,
    inner(
        other = "Hello",
        vis = pub(crate)
    )
))]

If I'm understanding correctly, our version of parsing a Meta would use syn::Expr::Verbatim where syn's parsing would've produced an error, but we won't retain that error initially.

Later on, when we call Inner::from_meta, that will see the value is a list and call Inner::from_list having just parsed the items into NestedMeta. This is the moment at which we produced the syn::Error that we actually hold onto.

Question 1: What function of FromMeta does that call, and what is the parameter it receives? I think we need the NestedMetaVerbatimNameValue struct so it can be the parameter type for this new from_invalid_meta method.

Question 2: In this example, there is no comma after the vis; will we stop consuming tokens in the right place?

[nik-rev]

If I'm understanding correctly, our version of parsing a Meta would use syn::Expr::Verbatim where syn's parsing would've produced an error, but we won't retain that error initially.

Right now, we do retain the error as a TokenStream when parsing the NestedMeta, there's no way around that if we want good error messages. We first have syn::Error, then we call error.into_token_stream() to turn that into a bunch of compile_error!("...") invocations (the TokenStream)

With the approach I mentioned above (adding new variant to NestedMeta), we store the syn::Error directly in the field, without needing to do any kind of "encoding"

Later on, when we call Inner::from_meta, that will see the value is a list and call Inner::from_list having just parsed the items into NestedMeta. This is the moment at which we produced the syn::Error that we actually hold onto.

Yes.

Meta::List contains this TokenStream:

(
    other = "Hello",
    vis = pub(crate)
)

Which is then parsed into the following 2 NestedMetas:

NestedMeta::Meta(Meta::NameValue(MetaNameValue {
    path: parse_quote(other),
    value: parse_quote!("Hello"),
}))
NestedMeta::VerbatimNameValue(NestedMetaVerbatimNameValue {
    path: parse_quote!(vis),
    input: quote!(pub(crate)),
    error: <syn error from Expr::parse(input)>
})

Question 1: What function of FromMeta does that call, and what is the parameter it receives? I think we need the NestedMetaVerbatimNameValue struct so it can be the parameter type for this new from_invalid_meta method.

I like the name FromMeta::from_invalid_expr, maybe we can call the variant NestedMeta::InvalidNameValueExpr, it can receive the NestedMetaInvalidNameValueExpr as a parameter:

struct NestedMetaInvalidNameValueExpr {
        path: Path,
        eq_token: Token![=],
        // the original input
        input: TokenStream,
        // error that resulted from trying to parse into an Expr
        error: syn::Error
}

We have FromMeta::from_expr that strictly deals with valid expressions, so FromMeta::from_invalid_expr will deal with invalid expressions

I believe this is better than from_invalid_meta or from_invalid_meta_name_value because the name part of the NestedMeta is valid (e.g. name = ) it is the value part which is not (the RHS is invalid):

vis = pub(crate)
^^^^^ valid
      ^^^^^^^^^^ invalid

Question 2: In this example, there is no comma after the vis; will we stop consuming tokens in the right place?

Yes

This is the logic responsible for that:

fn eat_until_comma<'c>(
    cursor: StepCursor<'c, '_>,
) -> syn::Result<(TokenStream, syn::buffer::Cursor<'c>)> {
    let mut rest = *cursor;
    let mut ts = TokenStream::new();
    while let Some((tt, next)) = rest.token_tree() {
        match tt {
            TokenTree::Punct(punct) if punct.as_char() == ',' => {
                break;
            }
            tt => {
                ts.extend([tt]);
                rest = next
            }
        }
    }
    Ok((ts, rest))
}

This eats every token until it encounters a comma (the break), or end of input (while let aborts)

[TedDriggs]

This is sounding good: Existing crate users won't need to make any changes to preserve current behavior, calling ToTokens will always produce back the input we got, and we can unblock things like types and visibilities in RHS positions.

[nik-rev]

I've just pushed a new iteration of the design. as discussed, it adds a new variant to NestedMeta without forcing us to resort to hacks.

[TedDriggs]

We are closing in.

[TedDriggs]

This would probably benefit from additional tests to capture error behaviors, but I am content with merging the changes as-is and adding those tests in another PR.