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

Allow string literals for meta items of type syn::Expr #229

Open
TedDriggs opened this issue Apr 27, 2023 · 11 comments
Open

Allow string literals for meta items of type syn::Expr #229

TedDriggs opened this issue Apr 27, 2023 · 11 comments

Comments

@TedDriggs
Copy link
Owner

Right now, when darling encounters a string literal for a field of type syn::Expr, it will attempt to parse the string contents and put that result into the struct. This means it's impossible to have a field that takes an arbitrary expression that may happen to be a string literal.

Proposed Fix

None yet; please comment on this issue if you're relying on the current behavior or want to rely on the behavior described above.

@milesj
Copy link

milesj commented May 15, 2023

I believe I'm running into this right now. I have these attributes:

#[setting(default = "foo.json")]
#[setting(default = "foo with. many values!")]

But they are being parsed as Expr::Field and Expr::Path respectively, instead of Expr::Lit.

Edit: Looks like this is by design?

/// For backwards-compatibility to versions of `darling` based on `syn` 1,
I wish we could turn this off.

@milesj
Copy link

milesj commented May 16, 2023

Been thinking about this a bit more. What if there was another derive, like FromAttributesStrict, that preserved string literals. This would be backwards compatible.

For context, I'm using Expr to define default values using default attr meta: https://github.com/moonrepo/schematic/blob/master/crates/config/tests/macros_test.rs#L46 Because of this string problem, I have to add 2 additional meta fields, default_str and default_fn.

@TedDriggs
Copy link
Owner Author

I'm leaning towards a solution that uses #[darling(with="...")] to choose a parsing function with the desired handling of string literals.

I don't want to forbid quotation marks in places where they don't cause ambiguity; if nothing else, as long as serde has quotation marks around its paths I'm going to keep typing them by reflex, and breaking that unnecessarily would annoy maintainers and users of darling-powered macros. That means the impl FromMeta for syn::Path would continue to be indifferent about quotation marks.

I think that syn::Expr is unique in having this problem: syn::Lit should be fine since it was previously valid in this position in syn 1, so we didn't have this issue of needing to "smuggle" content with quotation marks.

@milesj how about we aim to introduce a helper function in 0.20.1 that you can use to get unstuck, and then continue exploring the problem space to find the best long-term default?

@milesj
Copy link

milesj commented May 18, 2023

I'm cool with whatever approach allows me to have a single default field.

I've gone back and forth on whether to use strings for paths (like serde), or use actual paths, and landed on paths because it has better editor integration. For example, you can hover to see the signature, and click through to go to source. I wish more macros would follow suit.

Screenshot 2023-05-18 at 11 03 42 AM

@TedDriggs
Copy link
Owner Author

@milesj can you try these out for your use-case and make sure they work as-expected?

use syn::{Expr, Meta};

use crate::{Error, FromMeta};

pub fn preserve_str_literal(meta: &Meta) -> crate::Result<Expr> {
    match meta {
        Meta::Path(_) => return Err(Error::unsupported_format("path").with_span(meta)),
        Meta::List(_) => return Err(Error::unsupported_format("list").with_span(meta)),
        Meta::NameValue(nv) => Ok(nv.value.clone()),
    }
}

pub fn parse_str_literal(meta: &Meta) -> crate::Result<Expr> {
    match meta {
        Meta::Path(_) => return Err(Error::unsupported_format("path").with_span(meta)),
        Meta::List(_) => return Err(Error::unsupported_format("list").with_span(meta)),
        Meta::NameValue(nv) => {
            if let Expr::Lit(expr_lit) = &nv.value {
                Expr::from_value(&expr_lit.lit)
            } else {
                Ok(nv.value.clone())
            }
        }
    }
}

The expectation here would be that you add #[darling(with = "preserve_str_literal")] above your Expr field, and then it would behave the way you're asking for.

@milesj
Copy link

milesj commented May 22, 2023

@TedDriggs Looks to work 🎉

moonrepo/schematic#19

I did have to wrap the return in Option though for types to pass.

@TedDriggs
Copy link
Owner Author

TedDriggs commented May 22, 2023

@milesj ugh, that's kind of annoying - I don't really want to make people have to create one of these per wrapper type they want (e.g. Result, Option, Vec, etc).

Edit: This seems to be okay, since the with function composes nicely with the map = "..." option:

#[darling(with = "darling::util::parse_expr::preserve_str_literal", map = "Some")]

@milesj
Copy link

milesj commented May 23, 2023

Just chiming in that it's been working great. Thanks for the quick turn around 👍

@greyblake
Copy link
Contributor

At the moment I am rewriting nutype parsing logic to use darling and I stumble upon this in a few different places.
There are places where I have to take a dynamic value and generate it back and it does not work well at the moment.

@TedDriggs
Copy link
Owner Author

@greyblake have you tried the with approach described above?

@greyblake
Copy link
Contributor

greyblake commented Jul 10, 2023

@TedDriggs Thanks, I overlooked that.

So having this

pub fn preserve_str_literal(meta: &Meta) -> darling::Result<Option<Expr>> {
    match meta {
        Meta::Path(_) => return Err(darling::Error::unsupported_format("path").with_span(meta)),
        Meta::List(_) => return Err(darling::Error::unsupported_format("list").with_span(meta)),
        Meta::NameValue(nv) => Ok(Some(nv.value.clone())),
    }
}

and

    #[darling(with = "preserve_str_literal")]
    pub default: Option<Expr>,

Works together as I wish, string literals are correctly parsed 👍

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

No branches or pull requests

3 participants