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

Add declaration attributes to the parser. #1488

Merged
merged 2 commits into from
May 10, 2022
Merged

Conversation

otrho
Copy link
Contributor

@otrho otrho commented May 6, 2022

These will parse Rust-like attributes for any top-level item.
E.g., #[attrib(arg0, arg1)]

They are ignored by the rest of the compiler at this stage.

This is the first major change I've made to the new parser and so I'm a little unsure I've taken the right approach. Very happy to be corrected if not @canndrew 🙂

Also, I've just gone with basic attributes, nothing too fancy like #[attrib = "value"], or #[attrib(key = "value")]

Oh, and the docs will need to mention these eventually, I haven't done that. But there's not much to say yet -- once the storage attribs are implemented we can actually use them as examples...

@otrho otrho added enhancement New feature or request language feature Core language features visible to end users labels May 6, 2022
@otrho otrho self-assigned this May 6, 2022
sezna
sezna previously approved these changes May 6, 2022
Copy link
Contributor

@sezna sezna left a comment

Choose a reason for hiding this comment

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

LGTM!

@adlerjohn
Copy link
Contributor

Ready to merge @sezna?

@otrho
Copy link
Contributor Author

otrho commented May 7, 2022

@adlerjohn We can merge though there might not be a huge rush yet -- the next step is getting them into the AST and then replacing pure/impure but that can be based on this branch for now. I'm keen to see if @canndrew has suggestions on whether there's something I missed in the parser infra which could be done a bit better.

@adlerjohn
Copy link
Contributor

:shipit:

Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Exciting stuff!

sway-parse/src/attribute.rs Show resolved Hide resolved
sway-parse/src/attribute.rs Show resolved Hide resolved
sway-parse/src/toplevel.rs Outdated Show resolved Hide resolved
These will parse Rust-like attributes for any top-level item.
E.g., `#[attrib(arg0, arg1)]`

They are ignored by the rest of the compiler at this stage.
@otrho otrho force-pushed the otrho/add_attributes_to_parser branch from 6bdbe08 to ffe57e6 Compare May 9, 2022 05:07
@otrho
Copy link
Contributor Author

otrho commented May 9, 2022

Have now renamed Item to ItemKind and TopLevelStatement back to Item. Put the new Item into item/mod.rs.

@adlerjohn adlerjohn requested review from sezna and mitchmindtree May 9, 2022 13:35
sezna
sezna previously approved these changes May 9, 2022
impl Parse for Attribute {
fn parse(parser: &mut Parser) -> ParseResult<Self> {
let name = parser.parse()?;
let args = parser.parse().ok();
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than calling ok here and potentially throwing away a parse-error it'd be better to use Parens::try_parse. That will return None if the parens aren't there and will otherwise try to parse the contents and give you a Some.

if self.attribute_list.is_empty() {
self.kind.span()
} else {
Span::join(self.attribute_list[0].span(), self.kind.span())
Copy link
Contributor

Choose a reason for hiding this comment

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

A small nit but I'd prefer to use match self.attribute_list.first() rather than using panicky expressions like slice[index] where it isn't necessary.

Copy link
Contributor Author

@otrho otrho May 10, 2022

Choose a reason for hiding this comment

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

But you'd still need .first().unwrap() anyway? Both are guaranteed to be OK by the is_empty() check and both are 'panicky'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've dropped the .is_empty() and instead am matching on first()...

// punctuation.
//
// Or perhaps to `enter_delimited()` and `parse_to_end()` except there is no delimiter
// here; we have zero or more attributes before a declaration.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's intentional that you can't try to parse an entire attribute and then do something else if it fails. Infinite look ahead shouldn't be necessary and it can easily lead to exponential blowup of parsing times. So the parser only allows you to use a Peeker to look ahead and you can only look ahead a finite number of token trees by calling peek, peek2, and peek3 (and peek4 etc. if someone needs that and wants to add it).

So if you don't know what to parse next (eg. an attribute or an item) then the way to do it is what you've done here - just peek to see if the next thing is the start of an attribute. I agree it's bad that this means that attribute syntax leaks over into other parsing methods. A cleaner way to do it might be to do define an AttributeStartToken type, which either contains or is aliased to HashToken and which implements Peek. Then Item::parse could check for that instead of checking for HashToken explicitly. I was on the fence about doing that sort of thing in other parts of the parser but figured it wasn't worth the extra boilerplate. It would keep things a bit more logically encapsulated though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, fair enough.

@otrho otrho merged commit dab7bc8 into master May 10, 2022
@otrho otrho deleted the otrho/add_attributes_to_parser branch May 10, 2022 01:41
@mitchmindtree mitchmindtree mentioned this pull request Jun 13, 2022
JoshuaBatty added a commit that referenced this pull request Sep 26, 2022
This PR carries over any attributes that were parsed and stored in
`sway_ast::Module` into the `ParseProgram` decl types.

My primary motivation for adding this was so the language server could
access:
1. the attribute tokens so we can assign correct semantic information to
them for correct syntax highlighting
2. allow access to the doc attribute so we can provide in-editor
documentation pop-ups for hover requests.

See `sway-lsp/src/utils/attributes.rs` mod for the intended API for
accessing these tokens.

I did notice that in #1488 @ortho mentioned the next step would be to
pass the attributes into the AST. Feel free to let me know if you had
another idea for how these would be stored. I'm converting the
`AttributeDecl` into the `Attribute` type below which basically throws
away bracket & punctuation tokens and retains each of the `Ident`s. Then
the `AttributesMap` is what is stored in the AST. Similar to how it
worked before but now instead of having the map just store the vec of
args, it also stores the attribute name.

```rust
pub enum AttributeKind {
    Doc,
    Storage,
}

pub struct Attribute {
    pub name: Ident,
    pub args: Vec<Ident>,
}

pub type AttributesMap = HashMap<AttributeKind, Vec<Attribute>>;
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request language feature Core language features visible to end users
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants