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

💖 wiggle: escape rust keywords, allow witx literals #1793

Merged

Conversation

cratelyn
Copy link
Member

Overview

This commit makes changes to the wiggle::from_witx procedural in order
to allow for escaping strict and reserved Rust keywords.

Additionally, this commit introduces the ability to use a witx_literal
field in the {..} object provided as an argument to
wiggle::from_witx. This field allows for witx documents to be provided
as inline string literals.

Documentation comments are added to the methods of
wiggle_generate::names::Names struct responsible for generating
proc_macro2::Ident words.

Keyword Escaping

Today, an interface that includes witx identifiers that conflict with
with Rust syntax will cause the from_witx macro to panic at
compilation time.

Here is a small example (adapted from
/crates/wiggle/tests/keywords.rs) that demonstrates this issue:

;; Attempts to define a module `self`, containing a trait `Self`. Both
;; of these are reserved keywords, and will thus cause a compilation
;; error.
(module $self
    (@interface func (export "betchya_cant_implement_this")
    )
)

Building off of code that (as of master today)
demonstrates a strategy for escaping keywords, we introduce an
internal escaping module to generate/src/config.rs that contains
code responsible for escaping Rust keywords in a generalized manner.

Some code related to special cases, such as accounting for
errno::2big while generating names for enum variants, is moved
into this module as well.

As mentioned in the document comments of this diff, we do not include
weak keywords like 'static or union. Their semantics do not impact
us in the same way from a code generation perspective.

witx_literal

First, some background. Trait names, type names, and so on use a
camel-cased naming convention. As such, Self is the only keyword that
can potentially conflict with these identifiers. (See the [Rust
Reference][key] for a complete list of strict, reserved, and weak
keywords.)

When writing tests, this meant that many tests had to be outlined into
separate files, as items with the name $self could not be defined in
the same namespace. As such, it seemed like a worthwhile feature to
implement while the above work was being developed.

The most important function to note is the load_document inherent
method added to WitxConf, and that WitxConf is now an enum
containing either (a) a collection of paths, identical to its current
functionality, or (b) a single string literal.

Note that a witx document given to from_witx using a string literal
provided to from_witx cannot include use (..) directives, per
the witx::parse documentation.
(See: https://docs.rs/witx/0.8.5/witx/fn.parse.html)

Two newtypes, Paths and Literal, are introduced to facilitate the
parsing of WitxConf values. Their public API and trait implementations
has been kept to the minimum required to satisfy compilation in order to
limit the scope of this diff. Additional surface for external consumers
can be added in follow-up commits if deemed necessary in review.

💖 ✨

 # Overview

This commit makes changes to the `wiggle::from_witx` procedural in order
to allow for escaping strict and reserved Rust keywords.

Additionally, this commit introduces the ability to use a `witx_literal`
field in the `{..}` object provided as an argument to
`wiggle::from_witx`. This field allows for witx documents to be provided
as inline string literals.

Documentation comments are added to the methods of
`wiggle_generate::names::Names` struct responsible for generating
`proc_macro2::Ident` words.

 ## Keyword Escaping

Today, an interface that includes witx identifiers that conflict with
with Rust syntax will cause the `from_witx` macro to panic at
compilation time.

Here is a small example (adapted from
`/crates/wiggle/tests/keywords.rs`) that demonstrates this issue:

```
;; Attempts to define a module `self`, containing a trait `Self`. Both
;; of these are reserved keywords, and will thus cause a compilation
;; error.
(module $self
    (@interface func (export "betchya_cant_implement_this")
    )
)
```

Building off of code that (as of `master` today)
[demonstrates a strategy][esc] for escaping keywords, we introduce an
internal `escaping` module to `generate/src/config.rs` that contains
code responsible for escaping Rust keywords in a generalized manner.

[esc]: https://github.com/bytecodealliance/wasmtime/blob/0dd77d36f895df70c6e82758f23f553365c2f25f/crates/wiggle/generate/src/names.rs#L106

Some code related to special cases, such as accounting for
[`errno::2big`][err] while generating names for enum variants, is moved
into this module as well.

[err]: https://github.com/WebAssembly/WASI/blob/master/phases/snapshot/docs.md#-errno-enumu16

As mentioned in the document comments of this diff, we do not include
weak keywords like `'static` or `union`. Their semantics do not impact
us in the same way from a code generation perspective.

 ## witx_literal

First, some background. Trait names, type names, and so on use a
camel-cased naming convention.  As such, `Self` is the only keyword that
can potentially conflict with these identifiers. (See the [Rust
Reference][key] for a complete list of strict, reserved, and weak
keywords.)

When writing tests, this meant that many tests had to be outlined into
separate files, as items with the name `$self` could not be defined in
the same namespace. As such, it seemed like a worthwhile feature to
implement while the above work was being developed.

The most important function to note is the `load_document` inherent
method added to `WitxConf`, and that `WitxConf` is now an enum
containing either (a) a collection of paths, identical to its current
functionality, or (b) a single string literal.

Note that a witx document given to `from_witx` using a string literal
provided to `from_witx` cannot include `use (..)` directives, per
the `witx::parse` documentation.
(See: https://docs.rs/witx/0.8.5/witx/fn.parse.html)

Two newtypes, `Paths` and `Literal`, are introduced to facilitate the
parsing of `WitxConf` values. Their public API and trait implementations
has been kept to the minimum required to satisfy compilation in order to
limit the scope of this diff. Additional surface for external consumers
can be added in follow-up commits if deemed necessary in review.
@github-actions github-actions bot added the wasi Issues pertaining to WASI label May 30, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @kubkon

This issue or pull request has been labeled: "wasi"

Thus the following users have been cc'd because of the following labels:

  • kubkon: wasi

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

(union $union
;; A union variant that will expand to a strict keyword `Self`.
(field $self (@witx pointer f32))
;; Oh it's true, that there's power in a union!
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it appropriate to add a joke of this kind in the source?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

Awesome work!

crates/wiggle/generate/src/names.rs Show resolved Hide resolved
@pchickey pchickey merged commit c32c2e8 into bytecodealliance:master May 30, 2020
@cratelyn cratelyn deleted the ktm/beep-beep-keywords-to-the-jeep branch June 1, 2020 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants