Skip to content

Implement a new use syntax #415

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

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Conversation

alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Mar 22, 2021

This commit implements a new use syntax which changes

(use "foo.witx")

to

(use $some $type $names from $foo)

This change ended up being much larger than originally intended. While
at a surface level switching this style of use statements isn't too
bad it has large ramifications on the mental model of how to interpret
and work with *.witx files. This necessitated some internal
refactorings which ended up as a bit of a yak shave. The other changes
here are:

  • The polyfill module is removed. I confirmed with Pat that this isn't
    used anymore and this came about trying to update it from the below
    refactorings.
  • The render module is removed. Documents can no longer be serialized
    to a standalone s-expression because they can refer to other documents
    in a structured manner. It's also believed there are no current users
    of this functionality.
  • The representation module was removed since it was largely only used
    by the polyfill module.
  • The Document type was renamed to Module, and the ability for a
    document to contain multiple modules has been removed. When working
    with multiple modules this must now be done so explicitly instead of
    having it all lumped into one parsed object. This means that
    Module is now a list of types and a list of functions.
  • Documentation has changed to document a set of modules instead of a
    single module. Additionally this is nontrivially changed to load types
    transitively from all modules referenced to ensure that all relevant
    types are documented.
  • Internal refactorings in the validation phase have been done to
    streamline a few things and consolidate where possible with this new
    structuring.
  • InterfaceFunc is now named Function
  • InterfaceFuncParam is now named Param
  • Module has a name (as before) which is now inferred from the
    filename it's loaded from.
  • Module has a ModuleId now to represent types that are defined in
    other modules, where ModuleId is intended to be a unique identifier
    amongst a set of modules. For now it's just a path name but can likely
    get more fancy in the future if needed.

All *.witx files have been updated to the new syntax in this
repository. Lots of changes look like they happened to the proposal
documentation, but that's just because the documentation order of types
has been shuffled around. I've checked to make sure no actual items were
lost from the documentation.

Closes #378
Closes #379

@devsnek
Copy link
Member

devsnek commented Mar 22, 2021

feel free to ignore, but it would be nice if something delineated the list of import names. maybe an extra pair of parens?

@sunfishcode
Copy link
Member

Overall this looks good! My biggest concern here is the removal of the top-level (module ) syntax. For what it's worth I proposed a simlar syntax change back when the original wat syntax was being designed and it was clear that we wanted a file format fold holding exactly-one-module. But now, with witx being derived from wat syntax, and wat syntax retaining the outer (module ) around it, it feels like witx should retain it too.

@sbc100
Copy link
Member

sbc100 commented Mar 29, 2021

Isn't the outer (module ..) syntax optional in the .wit format?

@alexcrichton
Copy link
Contributor Author

I am not personally wed to any particular syntax here, so I'm happy to tweak as necessary for imports/etc.

@sunfishcode are you ok with the one-module-per-text-file restriction? (same as *.wat) It sounds like the module wrapper would be purely syntactical but wanted to confirm.

It's also true yeah that at least in *.wat the (module ...) is optional but AFAIK almost no one omits it, so seems reasonable to start requiring (module here and it can always be relaxed in the future.

@sunfishcode
Copy link
Member

One-module-per-file is ok with me. Hmm, I actually forgot that (module ...) is optional; it's not optional in the spec, and all the official examples include it, and it does seem everyone use it includes it in practice.

I think my instinct is that we should support and require the (module ...) at this point, though I could be convinced otherwise.

This commit implements a new `use` syntax which changes

  (use "foo.witx")

to

  (use $some $type $names from $foo)

This change ended up being much larger than originally intended. While
at a surface level switching this style of `use` statements isn't too
bad it has large ramifications on the mental model of how to interpret
and work with `*.witx` files. This necessitated some internal
refactorings which ended up as a bit of a yak shave. The other changes
here are:

* The `polyfill` module is removed. I confirmed with Pat that this isn't
  used anymore and this came about trying to update it from the below
  refactorings.
* The `render` module is removed. Documents can no longer be serialized
  to a standalone s-expression because they can refer to other documents
  in a structured manner. It's also believed there are no current users
  of this functionality.
* The `representation` module was removed since it was largely only used
  by the `polyfill` module.
* The `Document` type was renamed to `Module`, and the ability for a
  document to contain multiple modules has been removed. When working
  with multiple modules this must now be done so explicitly instead of
  having it all lumped into one parsed object. This means that
  `Module` is now a list of types and a list of functions.
* Documentation has changed to document a set of modules instead of a
  single module. Additionally this is nontrivially changed to load types
  transitively from all modules referenced to ensure that all relevant
  types are documented.
* Internal refactorings in the validation phase have been done to
  streamline a few things and consolidate where possible with this new
  structuring.
* `InterfaceFunc` is now named `Function`
* `InterfaceFuncParam` is now named `Param`
* `Module` has a `name` (as before) which is now inferred from the
  filename it's loaded from.
* `Module` has a `ModuleId` now to represent types that are defined in
  other modules, where `ModuleId` is intended to be a unique identifier
  amongst a set of modules. For now it's just a path name but can likely
  get more fancy in the future if needed.

All `*.witx` files have been updated to the new syntax in this
repository. Lots of changes look like they happened to the proposal
documentation, but that's just because the documentation order of types
has been shuffled around. I've checked to make sure no actual items were
lost from the documentation.

Closes WebAssembly#378
Closes WebAssembly#379
@alexcrichton
Copy link
Contributor Author

Ok updated!

@sunfishcode sunfishcode merged commit fc3da39 into WebAssembly:main Mar 29, 2021
cratelyn pushed a commit to cratelyn/WASI that referenced this pull request Jun 21, 2021
@cratelyn cratelyn mentioned this pull request Jun 21, 2021
alexcrichton pushed a commit that referenced this pull request Jun 21, 2021
* Revert "Enable more than one `handle` type in `*.witx` (#421)"

This reverts commit 834679b.

* Revert "Implement a new `use` syntax (#415)"

This reverts commit fc3da39.
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.

[witx] Update witx parser for new use syntax [witx] Make use import individual names from other modules
4 participants