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

feat: implement procedure annotation syntax #1510

Merged
merged 1 commit into from
Sep 27, 2024
Merged

Conversation

bitwalker
Copy link
Contributor

@bitwalker bitwalker commented Sep 23, 2024

This commit introduces the concept of attributes/annotations to Miden Assembly procedures. These can be used to represent interesting/useful bits of metadata associated with specific procedures in three different forms:

  • Marker attributes, e.g. #[inline], a name with no associated data
  • List attributes, e.g. #[inline(always)], i.e. a parameterized marker; multiple values can be provided as comma-delimited values.
  • Key-value attributes, e.g. #[key = <value>], where <value> can be a "meta expression" of three possible types: identifier, string, or integer (in either decimal or hexadecimal format).

Attributes will provide the foundation for upcoming changes that will rely on being able to attach metadata to procedures. For now, attributes may only be attached to procedure definitions, not re-exports or any other syntactic construct.

NOTE: This does not yet act on any attributes, nor store them anywhere when assembling to MAST. For now, they are simply parsed, made available in the AST, and ignored. Future PRs will introduce these as needed.

Closes #1434

@bitwalker bitwalker added enhancement New feature or request assembly Related to Miden assembly labels Sep 23, 2024
@bitwalker bitwalker self-assigned this Sep 23, 2024
@bitwalker bitwalker force-pushed the bitwalker/annotations branch from d855bcd to 86629e9 Compare September 23, 2024 21:00
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good! I left one question inline.

Also, I'm trying to think through the syntax choice #[...]. On the one hand, we use # for comments and so this syntax overloads comments a bit. But on the other hand, maybe that's not a bad thing.

An alternative could have been something like @[...]. It would make the attributes stand out a bit more syntactically - so, curious why you decided to go with #[...]. But to be honest, either way is fine.

For now, attributes may only be attached to procedure definitions, not re-exports or any other syntactic construct.

This is fine for the initial PR - but we'll probably need to lift this restriction fairly soon. The reason is that when defining account code, we'd want to attach attributes to re-exports. For example, we may define a basic wallet as something like this:

export.::miden::contracts::wallets::basic::receive_asset
export.::miden::contracts::wallets::basic::create_note
export.::miden::contracts::wallets::basic::move_asset_to_note

#[storage(offset = 0, size = 1)
export.::miden::contracts::auth::basic::auth_tx_rpo_falcon512

While, for example, a basic faucet could be defined as:

export.::miden::contracts::faucets::basic_fungible::distribute
export.::miden::contracts::faucets::basic_fungible::burn

#[storage(offset = 1, size = 1)
export.::miden::contracts::auth::basic::auth_tx_rpo_falcon512

So, in both cases we re-export miden::contracts::auth::basic::auth_tx_rpo_falcon512 procedure, but we attach different attributes to it based on the context.

assembly/src/ast/tests.rs Outdated Show resolved Hide resolved
assembly/src/ast/attribute.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@bitwalker
Copy link
Contributor Author

Also, I'm trying to think through the syntax choice #[...]. On the one hand, we use # for comments and so this syntax overloads comments a bit. But on the other hand, maybe that's not a bad thing.

Hmm, now that you mention it, I'm not super excited about that either. The choice itself is a common one, but I hadn't considered the edge cases parsing-wise.

I think my preference would be to use @name, @name(args, ..), and @name(key = value) instead. That's a small change to make, and syntactically should stand out much better.

For now, attributes may only be attached to procedure definitions, not re-exports or any other syntactic construct.

This is fine for the initial PR - but we'll probably need to lift this restriction fairly soon. The reason is that when defining account code, we'd want to attach attributes to re-exports. For example, we may define a basic wallet as something like this:

export.::miden::contracts::wallets::basic::receive_asset
export.::miden::contracts::wallets::basic::create_note
export.::miden::contracts::wallets::basic::move_asset_to_note

#[storage(offset = 0, size = 1)
export.::miden::contracts::auth::basic::auth_tx_rpo_falcon512

I didn't want to deal with some of the thorny questions around re-exports and attributes and their semantics, but it sounds like we'll need to deal with it, so:

  • Does a re-exported procedure inherit the attributes of the original definition? One might intuitively expect "yes", with respect to things like @inline, but:
  • What happens when the same attribute appears on both the original definition and a re-export? For example, @inline(never) on the original, and @inline(always) on the re-export. Or to use the expected use case here, conflicting storage offsets and size. With inlining, we could ignore the conflicting re-export attribute and raise a warning, but that may not be the right approach for some attributes, especially anything that the procedure code actually relies on.
  • If attributes can appear on re-exports, that means we can't just treat them as simple aliases of the original procedure, they effectively become their own unique definitions. That isn't exactly accurate, but a few things fall out of that:
    • We need to accumulate/resolve attributes when resolving procedure paths, so that we know the precise set of attributes that should be applied at a given call site
    • We need to store data associated with re-exported procedures, rather than just delegating to the original definition, in the MAST/Library/etc.
    • It isn't clear to me whether or not attributes can have an effect on how the code of the procedure is generated. Inlining isn't too problematic here, but imagine one using attributes for decorator-like cross-cutting concerns (i.e. not our usual notion of decorators, but the more general programming concept), e.g. #[global(name = FOO, data = 0x...)] which injects code to prepare data in memory from the advice provider, and replaces any reference to a pseudo-constant FOO in the procedure with the address at which the data is placed. This would affect the contents of the procedure.

There was something else as well, but it's early in the morning and I haven't fully woken up yet, so I lost it 😅, but I'll follow up when it comes back to me.

Anyway, we might be able to come up with some partial solutions for this for specific attributes, but for generality, we'll want to make sure we think through all of the implications before we lean too far on any one solution for this.

@bitwalker bitwalker force-pushed the bitwalker/annotations branch from 86629e9 to 89616a4 Compare September 25, 2024 16:55
This commit introduces the concept of attributes/annotations to Miden
Assembly procedures. These can be used to represent interesting/useful
bits of metadata associated with specific procedures in three different
forms:

* Marker attributes, e.g. `@inline`, a name with no associated data
* List attributes, e.g. `@inline(always)`, i.e. a parameterized
  marker; multiple values can be provided as comma-delimited metadata
  expressions.
* Key-value attributes, e.g. `@props(<key> = <value>, ...)`, where
  `<key>` must be a valid identifier, and `<value>` must be a valid
  metadata expression. Multiple key-value attributes can be set at once,
  or you can specify the same key-value attribute multiple times, so
  long as each instance does not have any keys that conflict with
  previous instances.

Metadata expressions come in three possible types:

* bare identifier, e.g. `foo`
* quoted string, e.g. `"some text"`
* integer value, either decimal or hexadecimal format, e.g. `1` or
  `0x01`

Attributes will provide the foundation for upcoming changes that will
rely on being able to attach metadata to procedures. For now, attributes
may _only_ be attached to procedure definitions, not re-exports or any
other syntactic construct.

NOTE: This does not yet act on any attributes, nor store them anywhere
when assembling to MAST. For now, they are simply parsed, made available
in the AST, and ignored. Future PRs will introduce these as needed.

Closes #1434
@bitwalker bitwalker force-pushed the bitwalker/annotations branch from 89616a4 to 8762dae Compare September 25, 2024 17:11
@bitwalker
Copy link
Contributor Author

bitwalker commented Sep 25, 2024

@bobbinth I've updated the PR with some syntax changes, improved code organization, and a few ergonomics improvements as well.

As noted in my other comment (and now in the commit and docs as well), annotations now come in the following varieties:

# A marker attribute, e.g. @inline
@<name>

 # A list attribute with a single item, e.g. @inline(always)
@<name>(<expr>)

 # A list attribute with multiple comma-delimited items, e.g. @deprecated("0.1.0", "Use foo::bar instead")
@<name>(<expr0>, .., <exprN>)

# A key-value attribute, e.g. @storage(offset = 1)
@<name>(<key> = <value>)

# A key-value attribute with multiple comma-delimited items, e.g. @storage(offset = 1, size = 2)
@<name>(<key0> = <value0>, .., <keyN> = <valueN>)

Furthermore, key-value annotations can be stacked, so long as the keys are unique, e.g.

@storage(offset = 1)
@storage(size = 2)
export.get_item
  ...
end

Conflicting declarations that trigger syntax errors:

  • Any two non-key-value attributes with the same name
  • Any two attributes with the same name, but differing type
  • Multiple key-value attributes with duplicated keys

In the process of making these changes, I also realized my test case was not actually testing anything because I forgot to update the equality implementation for Procedure 🤦, and I had actually specified !#[...] in the grammar originally, not #![...], so those attributes were being parsed as comments. In any case, I'm happier with the new syntax anyway, so that last bit is irrelevant. I've verified the tests are actually working now though, so it's actually exercising all the parser rules for annotations now.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you! I left just one non-blocking question inline.

assembly/src/parser/grammar.lalrpop Show resolved Hide resolved
assembly/src/parser/token.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assembly Related to Miden assembly enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants