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

wip(fix): import mapping resource overlap #557

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vados-cosmonic
Copy link
Contributor

@vados-cosmonic vados-cosmonic commented Jan 29, 2025

This commit attempts to fix the case of an import map that perfectly overlaps an existing component resource.

When a resource that exists in a component is overriden with an import map, the interface can change arbitrarily.


This handle failure seems to actually be much deeper... Where I've landed is that resources are broken when import mapped over.

I've been able to figure out that the import map'd resource that overlaps with the local one is being interpreted as a host & imported & owned resource.

They basically are being detected as owned when they should not be -- while it is possible to have a ResourceData::Host that is imported and owned, in this case the resource is actually coming from the import map (it's not the same resource as the component created/is in the WIT).

I can't tell if the right move here is to:

  • Disable host-mapping over resources that are defined by the component in question
  • Modify bindgen to figure out that import-mapped things (which are missing/don't construct properly) and use them differently somehow
  • make changes to componentize.js to maybe avoid producing lowering code (???) that may be causing the failures

I'm thinking it might be a componentize problem because the function in the WASM has no idea about the import mapping above, and is probably trying to lower using the definitions in the component (that don't match the import map).

The first issue was attempting to remove the resource that wasn't actually successfully created which I found a way to work around (that maybe makes sense), but I'm finding that while you can create the resource, you can't consume it (i.e. from consumeBar) inside the component.

@guybedford
Copy link
Collaborator

A local resource should never be an import - and imports always need to provide external resources that correspond to the correct imported resource type.

So this should be less about import mapping and more about whether the resource is imported or not.

That is, the approach in this PR doesn't sound correct to me - it would be good to dig in further to find the root cause fix here.

@guybedford
Copy link
Collaborator

Do we have a test case that replicates the issue?

@vados-cosmonic
Copy link
Contributor Author

This failure actually is a part of the test cases that was commented out (I'm assuming because something was wrong):

jco/test/cli.js

Line 564 in 46e729c

// strictEqual(m.consumeBar(m.createBar()), 'bar1');

Once I tried to uncomment that bit of the test, I ran into this issue and trying to trace it down, going through the generated code. What I ran into was that I described here. It was a bit ago but I tried to write enough to describe what I thought was happening.

Definitely open to ideas on what the problem could actually be here!

@guybedford
Copy link
Collaborator

A good starting point would be to uncomment that test in this PR and see if the tests are passing.

@vados-cosmonic
Copy link
Contributor Author

vados-cosmonic commented Feb 22, 2025

Sure, that's reasonable! just pushed, will rebase to main as well (core.rs has changed)

[EDIT] BTW, this is the error I was seeing (I don't think it's changed):

     RuntimeError: unreachable
      at  (wasm://wasm/0574e2aa:wasm-function[292]:0x2a60e)
      at  (wasm://wasm/0574e2aa:wasm-function[293]:0x2a61a)
      at  (wasm://wasm/0574e2aa:wasm-function[8312]:0x47aa1f)
      at consume-bar (wasm://wasm/0574e2aa:wasm-function[24267]:0xb7db0f)
      at Module.consumeBar (file:///tmp/qodNnt/out-component-dir/componentize.js:97:38)
      at Context.<anonymous> (file:///path/to/jco/test/cli.js:564:21)

This commit attempts to fix the case of an import map that perfectly
overlaps an existing component resource.

When a resource that exists in a component is overriden with an import
map, the interface can change arbitrarily.

Signed-off-by: Victor Adossi <[email protected]>
@vados-cosmonic vados-cosmonic force-pushed the fix=import-map-resource-overlap branch from 19cf57a to 5721251 Compare February 22, 2025 02:22
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.

2 participants