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

Tweak syntax for how component-level export identifiers are bound #341

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lukewagner
Copy link
Member

A good observation in #276 is that the way we bind new $identifiers in component-level export definitions is a bit irregular, being the only place in WAT where we bind an identifier without the identifier being right after the <sort> of the identifier (e.g., in (import "f" (func $f)) or (alias $c "e" (type $t))). As proposed in this PR, instead of binding a new identifier $t' like so:

(type $t (resource (rep i32)))
(export $t' "t" (type $t))

we would instead write:

(type $t (resource (rep i32)))
(export "t" (type $t) (type $t'))

and this would dovetail nicely with the type ascription, so that when you wanted to both bind an identifier and explicitly declare a type, you could write:

(type $t (resource (rep i32)))
(export "t" (type $t) (type $t' (sub resource)))

This PR doesn't affect the AST, semantics or binary format, just how identifier-binding works in the text format, but it is a breaking change. Thus, if we did go forward with this change, we'd probably want to support both the current and new style for a long time.

I don't think there is a big rush to make this change, though; I think it's mostly just a good cleanup to make before everything is finalized.

This was referenced Apr 13, 2024
@alexcrichton
Copy link
Collaborator

This makes sense to me, yeah, I'll work to see how this refactoring feels in the parser soon

Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

LGTM

@alexcrichton
Copy link
Collaborator

Ok in implementing this I think I've stumbled across an ambiguity which I'm not sure how best to resolve.

This is an existing test case (copied randomly from the list of failures) of what we have today before this PR:

(component $C
  (component $m)
  (alias outer $C $m (component $target))
  (export "v" (component $target))
)

That is desugared and/or printed with wasm-tools print, before this PR, as:

(component $C
  (component $m (;0;))
  (alias outer $C $m (component $target (;1;)))
  (export (;2;) "v" (component $target))
)

Here the (;2;) on the export annotation is there to indicate that it's creating a component with index 2. After this PR the above is printed as:

(component $C
  (component $m (;0;))
  (alias outer $C $m (component $target (;1;)))
  (export "v" (component $target) (component (;2;)))
)

The intention here is to continue to print the index in a comment, hence the (component (;2;)) here which would otherwise be replaced with (component $foo) if it were named. This, however, conflicts with the "inline definition of type syntax". For example if the above module is again piped through wasm-tools print it outputs:

(component $C
  (component $m (;0;))
  (alias outer $C $m (component $target (;1;)))
  (type (;0;)
    (component)
  )
  (export "v" (component $target) (component (;2;) (type 0)))
)

So basically what I'm getting at is that

(export "foo" (component $c) (component $other_name))

is ambiguous as to whether you're naming the new component $other_name or ascribing it with an empty component type.


I believe the above concern is basically only applicable to non-type exports. For types we've got (eq ..) or (sub resource) to disambiguate.

alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Apr 17, 2024
@lukewagner
Copy link
Member Author

Ah, thanks for implementing and pointing this out! Yes, that ambiguity makes sense.

The least-bad way to resolve this I could think of is to say that <sortexpr> is given precedence over <externdesc> in <externopt>, so that (sort) (sort $id) and (sort (; 0 ;)) are never parsed as a type ascription. With this rule:

  • Tooling can unconditionally emit (sort $id (; idx ;)), which seems like the priority use case
  • If you want to ascribe an empty type, you can, with (sort (type ...)) (although there's practically zero use case for this)

How does that sound?

@alexcrichton
Copy link
Collaborator

While I agree that's possible, personally I feel like the current situation is less-bad in the sense that while it's syntactically more wonky it doesn't involve special cases like that. I was trying to think of a syntax that doesn't run into these issues but I was also turning up blanks...

github-merge-queue bot pushed a commit to bytecodealliance/wasm-tools that referenced this pull request Apr 18, 2024
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.

3 participants