Update core type explainer text to match implementation #350
Conversation
design/mvp/Explainer.md
Outdated
| `core:alias` module declarators to *only* allow `outer` `type` aliases but, | ||
| in the future, more kinds of aliases would be meaningful and allowed. | ||
| `core:alias` module declarators to *only* allow `outer` `type` aliases to the | ||
| innermost enclosing component or component-type. In the future, more kinds of |
There was a problem hiding this comment.
I'm a bit confused by this sentence as well insofar as wasm-tools considers this valid
(component $A
(core type $t (func))
(component $B
(component $C
(alias outer $A $t (core type $t2))
)
)
)it was my intention to support arbitrary levels of nesting for aliases, but I wouldn't put it past me to have made a mistake here.
Basically I think it's reasonable to keep the spec as-is in this regard and fix the implementation instead.
There was a problem hiding this comment.
Ah yes, I guess I would need to further nuance the wording to allow that case. What I was mostly thinking about is that, atm, this example fails:
(component $A
(core type $t (func))
(component $B
(core type (module
(alias outer $A $t (type $t2))
))
)
)So are you saying you think this should work too?
There was a problem hiding this comment.
Aha, I see now! I didn't even realize that was an error. Looks like the implementation traces itself back to bytecodealliance/wasm-tools#640 which implements #44 which specifically mentions the 1-depth limit. (I'm learning this all as I'm going here too!)
It looks like that wording is still in Binary.md, although I can't immediately recall why such wording is there. That may also mean that wasm-tools isn't validating my component I pasted above correctly since it should be rejecting it like the component you have. The component you have though is rejected at the wast layer though rather than the wasmparser binary validation layer, so I'd also need to play around with things to see if the same validation is in wasmparser.
There was a problem hiding this comment.
If we're changing implementation anyways (to either accept more or reject more), if it's not too much work (b/c this doesn't seem like a huge priority, other than getting spec and impl to line up), would it make sense to remove the 1-depth limit?
There was a problem hiding this comment.
Yeah that's my thinking as well. I'll get to that probably next week and make sure it works out and post here?
There was a problem hiding this comment.
As probably expected this was easy enough to implement so I don't think there's any issue lifting the restrictions here
There was a problem hiding this comment.
Ok, great to hear, thanks!
This accompanies WebAssembly/component-model#350 and relaxes the previous constraint implemented in bytecodealliance#640 to reflect the spec-at-the-time, which has since been relaxed.
lukewagner
force-pushed
the
update-core-type-description
branch
from
9ad3390 to
4433389
Compare
|
Ok, I updated the PR to relax the restrictions and match the forthcoming implementation. @alexcrichton @rossberg PTAL |
This accompanies WebAssembly/component-model#350 and relaxes the previous constraint implemented in #640 to reflect the spec-at-the-time, which has since been relaxed.
3 participants
I noticed a small discrepancy between the explainer text for how core type definitions work with what's in Binary.md and what's implemented. What's implemented is actually a bit nicer (symbolic identifiers work, which I just checked), so this PR (should) bring the Explainer up to date.