peterhuene requested alexcrichton for a review on PR #4604.
peterhuene opened PR #4604 from ignore-alias-exports-for-types
to main
:
Currently, translation is ignoring type exports from components during
translation by skipping over them before adding them to the exports map.If a component instantiates an inner component and aliases a type export of
that instance, it will cause wasmtime to panic with a failure to find the
export in the exports map.The fix is to add a representation for exported types to the map that is simply
ignored when encountered. This also makes it easier to track places where we
would have to support type exports in translation in the future.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I think that this may not be quite right to ignore because this should add an item to the type namespace but this isn't doing that? I think that means that types defined after this
alias
wouldn't be numbered correctly.
peterhuene submitted PR review.
peterhuene created PR review comment:
That's a good point. Here's what I think might solve this:
- When we pop a type scope (i.e. finish parsing a component), associate it with the current translation to signify its statically-known types.
- Add a
ComponentTypeIndex
to theComponentItem::Type
variant.- When processing an alias to an exported type, lookup the
TypeDef
via the referenced translation's types and push it to the current type scope.Does that sound about right?
peterhuene edited PR review comment.
alexcrichton created PR review comment:
Thinking about this and also from our discussion in video just now, I think what I'm getting at is that I would naively expect this to be
enum ComponentItem { Type(TypeDef) }
where unlike the other variants ofComponentItem
types are "fully resolved" to a final type information which means we don't need to keep around the type scope information. Otherwise my worry is that when you import and instance with a type export and then alias a type from that during thetranslate.rs
phase we don't otherwise have a type scope to associate with that because we don't know how the import was satisfied.
alexcrichton submitted PR review.
peterhuene submitted PR review.
peterhuene created PR review comment:
Ah yeah, that makes more sense, no need to look up by index here, just push the
TypeDef
into the local type scope. I'll push up the fix.
peterhuene updated PR #4604 from ignore-alias-exports-for-types
to main
.
peterhuene submitted PR review.
peterhuene created PR review comment:
Pushed up the fix. Working on test cases to these two PRs now.
alexcrichton submitted PR review.
peterhuene updated PR #4604 from ignore-alias-exports-for-types
to main
.
peterhuene has enabled auto merge for PR #4604.
peterhuene merged PR #4604.
Last updated: Dec 23 2024 at 13:07 UTC