Wasmtime GitHub notifications bot (Aug 04 2022 at 00:34): Wasmtime GitHub notifications bot (Aug 04 2022 at 00:34):peterhuene requested alexcrichton for a review on PR #4604.
Wasmtime GitHub notifications bot (Aug 04 2022 at 00:34):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.
Wasmtime GitHub notifications bot (Aug 04 2022 at 15:05):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 04 2022 at 15:05):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 04 2022 at 15:05):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
aliaswouldn't be numbered correctly.
Wasmtime GitHub notifications bot (Aug 04 2022 at 17:48):peterhuene submitted PR review.
Wasmtime GitHub notifications bot (Aug 04 2022 at 17:48):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
ComponentTypeIndexto theComponentItem::Typevariant.- When processing an alias to an exported type, lookup the
TypeDefvia the referenced translation's types and push it to the current type scope.Does that sound about right?
Wasmtime GitHub notifications bot (Aug 04 2022 at 17:54):peterhuene edited PR review comment.
Wasmtime GitHub notifications bot (Aug 04 2022 at 19:14):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 ofComponentItemtypes 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.rsphase we don't otherwise have a type scope to associate with that because we don't know how the import was satisfied.
Wasmtime GitHub notifications bot (Aug 04 2022 at 19:14):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 04 2022 at 19:35):peterhuene submitted PR review.
Wasmtime GitHub notifications bot (Aug 04 2022 at 19:35):peterhuene created PR review comment:
Ah yeah, that makes more sense, no need to look up by index here, just push the
TypeDefinto the local type scope. I'll push up the fix.
Wasmtime GitHub notifications bot (Aug 04 2022 at 19:48):peterhuene updated PR #4604 from ignore-alias-exports-for-types to main.
Wasmtime GitHub notifications bot (Aug 04 2022 at 19:48):peterhuene submitted PR review.
Wasmtime GitHub notifications bot (Aug 04 2022 at 19:48):peterhuene created PR review comment:
Pushed up the fix. Working on test cases to these two PRs now.
Wasmtime GitHub notifications bot (Aug 04 2022 at 20:11):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 04 2022 at 21:29):peterhuene updated PR #4604 from ignore-alias-exports-for-types to main.
Wasmtime GitHub notifications bot (Aug 04 2022 at 21:31):peterhuene has enabled auto merge for PR #4604.
Wasmtime GitHub notifications bot (Aug 04 2022 at 22:45):peterhuene merged PR #4604.
Last updated: Nov 22 2024 at 16:03 UTC