rustwasm / weedle

A WebIDL Parser
MIT License
43 stars 27 forks source link

Suggestion - remove non-semantic terminals from AST #13

Closed richard-uk1 closed 6 years ago

richard-uk1 commented 6 years ago

For me, they increase cognitive load for no benefit

Example:

pub struct NamespaceDefinition<'a> {
    pub attributes: Option<ExtendedAttributeList<'a>>,
    pub namespace: Namespace,
    pub identifier: Identifier<'a>,
    pub members: Parenthesized<NamespaceMembers<'a>>,
    pub semi_colon: SemiColon,
}

namespace and semi_colon are not adding anything, identifier can be just the inner type, and members can just be a NamespaceMembers. Since all the other symbols are required, getting rid of them is lossless, in the sense that you know they had to be there.

You could just have an ExtendedAttributeList for attributes, but this might not be lossless since currently you can distinguish between an empty attribute list and its absence.

Counter-arguments welcome!

alexcrichton commented 6 years ago

Good points! Right now this basically makes them super easy to write parser for. If you remove the fields as-is then it will no longer parse because the relevant keywords won't be parsed!

That being said this crate definitely isn't optimized for readability and usability, only mostly for compile times for us to use :)

richard-uk1 commented 6 years ago

Cool, that makes sense, I guess standard practice is to have a compiler pass to prune the AST into something leaner - but that's probably not a priority.

ohanar commented 6 years ago

I see a pretty straightforward path to remove the extra fields like namespace and semi_colon. Removing the Parenthesized is a bit orthogonal, but also pretty straightforward. It certainly isn't a priority for me at the moment though.

richard-uk1 commented 6 years ago

This probably isn't desirable, since leaving them in allows span information to be preserved in the future, so I'd suggest closing this issue.

alexcrichton commented 6 years ago

Ok!