store argument in wasmtime.bindgen output seems redundant

[whitequark]

When used with a component, wasmtime.bindgen will generate something like this:

import wasmtime

class Root:
    def __init__(self, store: wasmtime.Store) -> None:
        ...
    def render_json(self, caller: wasmtime.Store, json: str) -> str:
        ...

At first I thought that caller != store is a valid configuration but it's clearly not since it aborts:

thread '<unnamed>' panicked at crates/wasmtime/src/runtime/func.rs:875:9:
assertion failed: self.comes_from_same_store(store)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Aborted

Why not capture the store then? It's annoying to have to thread it down into every call if there's only one valid value for it anyway.

[alexcrichton]

The original intention here was to model the Rust API in Python as well. This enables having Store.close as a method for example to be able to deallocate the store and not worry about dangling references to it too much by accident.

That being said I don't disagree with you and ergonomically it'd probably be best in Python to close over the store and then document that it's captured for the lifetime of the class.

[whitequark]

Wouldn't it be implicitly captured by the instance anyway? (I'm not sure how the Rust lifetime handling works in this case.)

[alexcrichton]

Currently no, the instance is a glorified index into the store so it doesn't actually close over anything. Now the same questions here could equally be applied to the Instance API of this package, however, since the Python Instance could very well close over the Store.

As I page this back in a bit, I do remember one other problem which is that there's no cycle collection between Rust and Python so I wanted to make it theoretically pretty hard to create an un-collectable cycle. That means that closures representing host functions shouldn't close over a Store (since then that'd introduce a cycle), and to enable closures to recursively call wasm that's where the "caller" bits came in too.

I realize though that this cyclic concern may not be applicable to most applications, especially if the component instance lives for the entire lifetime of the Python process as it does here. In that sense I also think it'd be reasonable to add a knob to bindgen for this as well.

[whitequark]

In that sense I also think it'd be reasonable to add a knob to bindgen for this as well.

This unfortunately isn't very helpful for cases like #219 (comment) where bindgen isn't invoked explicitly but rather during loading, since it's pretty difficult to adjust these knobs without introducing global variables and potentially messing things up for other wasmtime API consumers.

[whitequark]

I think the existing wasmtime.loader is an excellent example here since it has this line:

wasmtime-py/wasmtime/loader.py

Line 22 in 56e1fcd

# TODO: how to configure wasi?

There's not really a good way to resolve this TODO so I'd rather keep the amount of bindgen knobs to the absolute minimum and instead settle on some configuration that works for nearly everyone, perhaps at cost of some convenience or even performance.

[whitequark]

@alexcrichton Here's a few examples of foreign libraries that require returned objects to be used on the same thread:

• pyduktape2 (checks the invariant on each call)
• quickjs (segfaults if invariant is violated)

(edit: oops, this was meant to go to an adjacent thread)

[alexcrichton]

That's a good point yeah about minimizing the knobs. I also think components could be a bit of a "saving grace" here as the imports are provided in such a way that makes cycles somewhat nontrivial to create. In that sense I think it might be pretty reasonable to switch generation mode to just go ahead and close over the initial store.