-
Notifications
You must be signed in to change notification settings - Fork 82
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
base: main
Are you sure you want to change the base?
Conversation
This makes sense to me, yeah, I'll work to see how this refactoring feels in the parser soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 (component $C
(component $m (;0;))
(alias outer $C $m (component $target (;1;)))
(export (;2;) "v" (component $target))
) Here the (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 $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 I believe the above concern is basically only applicable to non-type exports. For types we've got |
Minor cleanups noticed during an attempt to implement WebAssembly/component-model#341
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
How does that sound? |
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... |
Minor cleanups noticed during an attempt to implement WebAssembly/component-model#341
824fdc5
to
74bd278
Compare
A good observation in #276 is that the way we bind new
$identifiers
in component-levelexport
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:we would instead write:
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:
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.