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

Prototype for support of custom keywords #480

Closed
wants to merge 1 commit into from

Conversation

kureuil
Copy link
Contributor

@kureuil kureuil commented Sep 4, 2018

First prototype for support of custom keywords with syn's new parsing
API. Documentation and tests aren't present for now as I'm mainly
reaching for feedback.

This patch introduces a new Keyword trait, a new macro custom_keyword!
and exposes the existing TokenMarker enum. The Keyword trait
automatically implements the Token trait, making it possible to peek on
custom keywords (this is why I had to make TokenMarker public).

The custom macro generates a structure storing an Ident and implementing
the Keyword and Parse traits. A function with the same name as the
structure is also generated in order to use it like any predefined
keyword.

Unresolved questions:

  • Making TokenMarker public is the only way I found to allow custom_keyword to implement the Peek trait. Is it possible and/or dangerous ?
  • Via its blanket impls, Keyword is highjacking the fact that the Token trait is currently sealed. Could this have implications that I'm not foreseeing ?

Ref: #476

// Example usage, parsing `key = "value"`
custom_keyword!(key);

struct KeyPair {
    key: key,
    equals: Token![=],
    value: syn::LitStr,
}

impl syn::parse::Parse for KeyPair {
    fn parse(input: syn::parse::ParseStream) -> syn::parse::Result<Self> {
        Ok(KeyPair {
            key: input.parse()?,
            equals: input.parse()?,
            value: input.parse()?,
        })
    }
}
// More complex example parsing either:
// - `key_str = "literal string"`
// - `key_bool = true` or `key_bool = false`
custom_keyword!(key_str);
custom_keyword!(key_bool);

enum Pairs {
    Str { key: key_str, equals: Token![=], value: syn::LitStr },
    Bool { key: key_bool, equals: Token![=], value: syn::LitBool }
}

impl syn::parse::Parse for Pairs {
    fn parse(input: syn::parse::ParseStream) -> syn::parse::Result<Self> {
        let lookahead = input.lookahead1();
        if lookahead.peek(key_str) {
            Ok(Pairs::Str {
                key: input.parse()?,
                equals: input.parse()?,
                value: input.parse()?,
            })
        } else if lookahead.peek(key_bool) {
            Ok(Pairs::Bool {
                key: input.parse()?,
                equals: input.parse()?,
                value: input.parse()?,
            })
        } else {
            Err(lookahead.error())
        }
    }
}

First prototype for support of custom keywords with syn's new parsing
API. Documentation and tests aren't present for now as I'm mainly
reaching for feedback.

This patch introduces a new Keyword trait, a new macro custom_keyword!
and exposes the existing TokenMarker enum. The Keyword trait
automatically implements the Token trait, making it possible to peek on
custom keywords (this is why I had to make TokenMarker public).

The custom macro generates a structure storing an Ident and implementing
the Keyword and Parse traits. A function with the same name as the
structure is also generated in order to use it like any predefined
keyword.
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this is broadly the right approach. Some suggestions:

  • Please do not add any public APIs other than custom_keyword!. Everything underneath is an implementation detail. This should also make your life easier when it comes to documentation.

  • Please make custom keywords act as much as possible like builtin keywords. If you look at
    Token![return] for example there is a public span field and several useful trait impls.

#[macro_export]
macro_rules! custom_keyword {
($ident:ident) => {
custom_keyword_internal!({ pub(in self) } $ident);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the default pub(in self) rather than the usual default empty visibility? Actually is there any difference between empty visibility and in self?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is pub(in self) because of an older version of the custom_keyword_internal! macro, and I forgot to update this call, it can be safely removed. According to https://doc.rust-lang.org/rust-by-example/mod/visibility.html, empty visibility and in self seem to have the same effect.

@kureuil
Copy link
Contributor Author

kureuil commented Sep 5, 2018

As an update: I worked a little on this and mostly struggled to come up with an acceptable solution. I'm starting to run out of ideas and I probably won't get to finish this before you release 0.15 so feel free to close this PR if you find a solution in the meantime.

Please do not add any public APIs other than custom_keyword!. Everything underneath is an implementation detail. This should also make your life easier when it comes to documentation.

By public API, do you mean 1) items exposed by the crate and visible in the generated documentation, or 2) an item exposed by the crate regardless of their documentation visibility ? Because I cannot see how it would be possible to hook into the Parse & Peek parsing APIs without introducing at least one new trait.

Please make custom keywords act as much as possible like builtin keywords. If you look at
Token![return] for example there is a public span field and several useful trait impls.

Regarding the span field: this was the route I envisionned too initially, however the syn crate is not re-exporting proc_macro2's Span, which meant either have users explicitely depend on it (but that would be poor UX IMO), or have syn expose it but I wasn't sure about adding a new public API.

Regarding the extra traits impls: because of the way #[cfg] attributes works, there would be one macro implementation for each combination of features. I tried searching how to avoid this code duplication, but couldn't find what I was searching for.

@dtolnay
Copy link
Owner

dtolnay commented Sep 6, 2018

Ah, to clarify: if something is hidden from docs, we don't need to consider it public API. I like to put a short // Not public API. comment to make it super clear like here, but generally something hidden is only considered public API if somewhere else in rustdoc the reader is told to use it.

I would be on board with re-exporting Span as syn::Span. I have written code where that would have been convenient. Please make this change in a separate commit (same PR but its own commit).

I imagine the cfg would be done as something like:

macro_rules! custom_keyword {
    /* ... */
    clone_for_custom_keyword!(/* ... */);
    extra_traits_for_custom_keyword!(/* ... */);
}

#[cfg(feature = "clone-impls")]
macro_rules! clone_for_custom_keyword { /* ... */ }

#[cfg(not(feature = "clone-impls"))]
macro_rules! clone_for_custom_keyword { /* expand to nothing */ }

#[cfg(feature = "extra-traits")]
macro_rules! extra_traits_for_custom_keyword { /* ... */ }

#[cfg(not(feature = "extra-traits"))]
macro_rules! extra_traits_for_custom_keyword { /* expand to nothing */ }

@dtolnay dtolnay mentioned this pull request Sep 6, 2018
@dtolnay
Copy link
Owner

dtolnay commented Sep 6, 2018

Closing in favor of #481 which includes your commit. Thanks for getting the work started on this!

@dtolnay dtolnay closed this Sep 6, 2018
@kureuil
Copy link
Contributor Author

kureuil commented Sep 6, 2018

Awesome, thanks a lot for getting this over the line!

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

Successfully merging this pull request may close these issues.

2 participants