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

Create an extended version of syn::Meta that accepts paths and expressions in value position. #1704

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

agausmann
Copy link

As discussed in #368 and #1490, there is a desire for supporting a larger set of syntaxes in attributes, including the ability to pass paths and expressions as values.

Right now, attribute parsing uses syn::Meta, but this only allows for literals in value position. This means that any other desired syntax must be wrapped in string literals (e.g. #[serde(default = "defaults::one")]). The long-term goal is to eventually replace such string literals with paths, and to be able to support other syntax elements in value position, such as expressions, for new kinds of options like default_value.

The approach that I'm taking is to duplicate and extend syn::Meta. I haven't bothered with changing the names or the basic structure as it seems to be otherwise sufficient, and this allows us to use it as a drop-in replacement with minimal changes. The main difference is that lit: Lit in MetaNameValue is replaced with lit: MetaValue, where MetaValue is an enum that allows types other than Lit to be parsed in its place.

@agausmann
Copy link
Author

agausmann commented Jan 4, 2020

Where I'm at now: I've replaced all uses of syn::Meta with the extended version, and it now supports path parsing. The only test failures are UI mismatches due to the different high-level parsing logic and also due to a new message "expected path or literal" in value position.

Adding support for expressions isn't as straightforward. First, syn::Expr will require syn's "full" feature. I don't know what impact this will have on compile times, but it's worth considering whether that's acceptable. (EDIT: I was mistaken, it is available with "derive", which is in our current feature set.) Second, there is ambiguity between paths, literals and expressions, as paths and literals are also valid expressions (e.g. std::mem::replace being an expression returning a function pointer). What I could do is completely replace MetaValue with Expr; though it might not be semantically correct to parse paths as expressions in all cases (e.g. #[serde(with = some::module)], it is still possible to match against the parsed expression and extract the path.

@agausmann agausmann marked this pull request as ready for review January 4, 2020 03:19
@agausmann
Copy link
Author

agausmann commented Jan 4, 2020

I'm having second thoughts about exactly specifying the type of values in MetaNameValue. It might make more sense to simply store it as a TokenStream and have a method like fn parse_value::<T: Parse>(&self) -> Result<T>. This is simultaneously the most flexible and most specific option, as it allows the value to be any kind of syntax element, and that type can be determined by each option in its own parsing code. We would be able to parse many other things like where clauses, types, and lifetimes.

This will also result in better diagnostic error messages for the end-user. If the type were specified within the structure of MetaNameValue, then it would also have to be parsed that early, and the error message would have to be as generic as possible, as nothing is known about what specific type is expected until the MetaNameValue is passed to an option for further parsing.

This pattern already exists in other places (e.g. syn::Attribute::parse_args), so I think it's worth trying out.

@agausmann
Copy link
Author

agausmann commented Jan 4, 2020

I had some issues with ambiguity when parsing values as TokenStreams, as they will parse every remaining token, including the punctuating commas. I'm looking into ways to resolve that, but I'm not very familiar with the proc macro ecosystem; feel free to add suggestions if you have any! My current plan is to make a custom parsing function for Punctuated that splits the token stream before parsing the individual nodes.

Example:

error: expected serde tag attribute to be a string: `tag = "..."`
   --> test_suite/tests/test_gen.rs:604:19
    |
604 |     #[serde(tag = "t", content = "c")]
    |                   ^^^^^^^^^^^^^^^^^^

@agausmann
Copy link
Author

Ready for discussion / review

@agausmann
Copy link
Author

agausmann commented Jan 6, 2020

I've been working on the next step, modifying the parsing frontend to accept inline syntax, you can see progress on my branch inline_values.

Something I ran into is an ambiguity with regard to where clauses. Since they use commas as punctuation, you can't write something like #[serde(bound = E: Clone, F: Clone)]. They would have to be written as separate invocations of the bound option, The other possibility I'm considering is allowing the where clause to be delimited with parentheses, so it would be written as #[serde(bound = (E: Clone, F: Clone))], but this definitely needs some discussion.

@agausmann
Copy link
Author

agausmann commented May 4, 2020

@dtolnay any feedback? There are a few UI failures but they don't seem to be any harder to understand, though a bit more verbose.

@dtolnay
Copy link
Member

dtolnay commented May 6, 2020

Hi @agausmann, I am still interested in this change but I haven't gotten a chance to take a look yet. It is still definitely on the list (https://triage.rust-lang.org/notifications?user=dtolnay).

@pksunkara
Copy link

I did a brief review, looks good to me. But I don't know the internals of serde that well to forsee any future problems.

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

Successfully merging this pull request may close these issues.

None yet

3 participants