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

proc_macro::TokenStream: provide AST node kind hint #50053

Open
abonander opened this issue Apr 18, 2018 · 8 comments
Open

proc_macro::TokenStream: provide AST node kind hint #50053

abonander opened this issue Apr 18, 2018 · 8 comments
Labels
A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-feature-request Category: A feature request, i.e: not implemented / a PR.

Comments

@abonander
Copy link
Contributor

abonander commented Apr 18, 2018

/// Enum representing AST nodes a #[proc_macro_attribute] may be applied to
// Bikeshedding welcome
pub enum SyntaxNodeKind {
    // when crates as macro inputs aren't pretty-printed as modules (#41430) 
    Crate,
    Item, // could be module, function, impl, etc. `syn` can figure the rest out
    Statement,
    Expression, 
    ExternItem, // since item kinds are restricted in `extern {}`
} 

impl TokenStream {
    /// If this token stream represents a valid syntax tree node, return its kind. 
    /// Returns `None` for raw tokens
    // Alternately it could simply panic when not available because that would only happen in
    // `#[proc_macro]` which should expect only raw tokens anyway
    pub fn syntax_node_kind(&self) -> Option<SyntaxNodeKind> {}
}

This would be exclusively for #[proc_macro_attribute]s which parse their input as AST nodes:

  • attributes that only accept one kind could assert equality and error immediately instead of attempting to parse their expected kind (allowing them to emit a concise error message instead of "expected [token] got [token]")
  • attributes that accept multiple kinds won't have to guess at what node kind they should attempt to parse

cc @alexcrichton @dtolnay @petrochenkov

@abonander
Copy link
Contributor Author

abonander commented Apr 18, 2018

Item could wrap another pub enum ItemKind { Struct, Enum, Other} to simplify derives that only work on structs or enums but not both.

@dtolnay I think this issue should be added to #47786

@abonander
Copy link
Contributor Author

abonander commented Apr 18, 2018

Alternately this could be encoded directly into #[proc_macro_attribute]'s input type, like:

pub struct AttributeInput {
    // assume we'd use getters instead
    pub tokens: TokenStream,
    pub node_kind: SyntaxNodeKind, 
}

#[proc_macro_attribute]
pub fn my_attr(attr: TokenStream, input: AttributeInput) -> TokenStream {}

Or as an optional third argument:

#[proc_macro_attribute]
pub fn my_attr(attr: TokenStream, input: TokenStream, kind: SyntaxNodeKind) -> TokenStream {}

@alexcrichton alexcrichton added the A-macros-2.0 Area: Declarative macros 2.0 (#39412) label Apr 18, 2018
@alexcrichton
Copy link
Member

I think this is a neat idea but I would prefer for it to not make procedural macros more wordy in all situations. We always have the support of adding multiple valid signatures for #[proc_macro_attribute] and friends, so this can be added backwards-compatibly down the road for sure.

In that sense if we wanted to change the defaults I'd prefer something like the AttributeInput example (which is also arguably pretty extensible for future additions). I think I'd still hold off for this initial round though without stronger use cases as what we have today is sort of the least common denominator.

@abonander do you feel like this greatly empowers macro authors to do more though over what we provide today?

@abonander
Copy link
Contributor Author

@alexcrichton

do you feel like this greatly empowers macro authors to do more though over what we provide today?

It's more of a quality of life issue and an error reporting one. Currently attributes can reasonably assume that they'll only be applied to items but that's not going to be the case forever. This would allow them to detect cases that they don't support, or decide which parsing methods to use instead of guessing and checking.

@abonander
Copy link
Contributor Author

Right now the approach I like the most is simply adding a method to TokenStream to get this information as that would touch the fewest bits of code. I'm still not sure whether it should return Option or simply panic when this is not available, which would only occur in a #[proc_macro] which shouldn't be expecting a well formed AST node anyway.

@alexcrichton
Copy link
Member

@abonander but you can do this today, right? In the sense that you can use a library like syn to parse an item and then only accept a whitelist of items. I see the possible major advantage here as not using a library like syn, but you'll probably be using one anyway to actually parse the result, right?

@abonander
Copy link
Contributor Author

@alexcrichton This is so you can choose what node to parse with syn because AFAICT it doesn't have a way to parse something that could be a File (when parsing a whole crate) or Item or Stmt or Expr, you have to pick one of those and then wait for it to error before trying another (if you accept more than one kind) and also try to decide whether it was a legitimate parsing error or you just picked the wrong kind.

For attributes that only accept one kind, like right now most are written to implicitly accept items only, they can immediately produce a clear and concise error ("this attribute only works on items") instead of a generic parsing error ("expected struct, fn, impl, etc., got let").

@alexcrichton
Copy link
Member

Yeah I don't doubt that an error can be generated more efficiently, but I don't think it's necessarily enabling anything fundamental that's not already being done, so I'm tempted to not add it yet and see how pressing it becomes over time.

@alexcrichton alexcrichton added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros-2.0 Area: Declarative macros 2.0 (#39412) C-feature-request Category: A feature request, i.e: not implemented / a PR.
Projects
None yet
Development

No branches or pull requests

2 participants