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

Rewrite the AST to be a bit more user-friendly #144

Merged
merged 1 commit into from
May 23, 2017

Conversation

alexcrichton
Copy link
Collaborator

This commit is a relatively large rewrite of the AST that syn exposes. The
main change is to expose enums-of-structs rather than
enums-with-huge-tuple-variants. The best example of this is ItemKind::Fn which
changed from:

enum ItemKind {
    Fn(Box<FnDecl>, Unsafety, Constness, Option<Abi>, Generics, Box<Block>),
    ...
}

to

enum ItemKind {
    Fn(ItemFn),
    ...
}

struct ItemFn {
    decl: Box<FnDecl>,
    unsafety: Unsafety,
    constness: Constness,
    abi: Option<Abi>,
    generics: Generics,
    block: Box<Block>,
}

This change serves a few purposes:

  • It's now much easier to add fields to each variant of the ast, ast struct
    fields tend to be "by default ignored" in most contexts.
  • It's much easier to document what each field is, as each field can have
    dedicated documentation.
  • There's now canonicalized names for each field (the name of the field) which
    can help match match statements more consistent across a codebase.

A downside of this representation is that it can be a little more verbose to
work with in match statements and during constructions. Overall though I'd
feel at least that the readability improved significantly despite the extra
words required to do various operations.

Closes #136

@alexcrichton
Copy link
Collaborator Author

Note that tons of the names here are bikesheddable, I wouldn't mind tweaking anything!

@dtolnay
Copy link
Owner

dtolnay commented May 23, 2017

Amazing!

How would you feel about putting these tuple variants in modules? For example expr::IfLet instead of ExprIfLet. That would help avoid the even-worse wall of types when you open the documentation. I'll merge this quickly and we can follow up separately if you agree.

@alexcrichton
Copy link
Collaborator Author

Sounds great to me!

@alexcrichton
Copy link
Collaborator Author

Ah so I've discovered that if we do IfLet as a struct name then you can't import both ExprKind::* and expr::* unfortunately, leaving us with a situation that chooses between a number of the following in a match statement:

// currently proposed
IfLet(ExprIfLet { ... }) => 

// Drop "Expr" prefix
ExprKind::IfLet(IfLet { ... }) => 
// or
IfLet(expr::IfLet { ... }) => 

// another random prefix?
IfLet(EIfLet { ... }) => 

@dtolnay
Copy link
Owner

dtolnay commented May 23, 2017

Let's keep the current approach for enums.

The build failure looks like TyPtr not printing "const".

@alexcrichton alexcrichton force-pushed the more-structs branch 2 times, most recently from c90be57 to 40d6a96 Compare May 23, 2017 15:48
This commit is a relatively large rewrite of the AST that `syn` exposes. The
main change is to expose enums-of-structs rather than
enums-with-huge-tuple-variants. The best example of this is `ItemKind::Fn` which
changed from:

    enum ItemKind {
        Fn(Box<FnDecl>, Unsafety, Constness, Option<Abi>, Generics, Box<Block>),
        ...
    }

to

    enum ItemKind {
        Fn(ItemFn),
        ...
    }

    struct ItemFn {
        decl: Box<FnDecl>,
        unsafety: Unsafety,
        constness: Constness,
        abi: Option<Abi>,
        generics: Generics,
        block: Box<Block>,
    }

This change serves a few purposes:

* It's now much easier to add fields to each variant of the ast, ast struct
  fields tend to be "by default ignored" in most contexts.
* It's much easier to document what each field is, as each field can have
  dedicated documentation.
* There's now canonicalized names for each field (the name of the field) which
  can help match `match` statements more consistent across a codebase.

A downside of this representation is that it can be a little more verbose to
work with in `match` statements and during constructions. Overall though I'd
feel at least that the readability improved significantly despite the extra
words required to do various operations.

Closes dtolnay#136
@alexcrichton
Copy link
Collaborator Author

Alright I think that'll do it!

@alexcrichton
Copy link
Collaborator Author

Ah looks like I missed a few things when testing in 1.12.1, but now it's getting into the realm of "may not be super easy to fix". Looks like this'd bump to 1.15.0+

@dtolnay
Copy link
Owner

dtolnay commented May 23, 2017

That's fine. I'll remove those Travis builds.

@dtolnay dtolnay merged commit 98b40c0 into dtolnay:master May 23, 2017
@dtolnay
Copy link
Owner

dtolnay commented May 23, 2017

Up to Rust 1.15.1 in d9e2dc9.

@alexcrichton alexcrichton deleted the more-structs branch May 23, 2017 16:18
@alexcrichton
Copy link
Collaborator Author

👍

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