0.2.95 breaks imports that rely on context when using js_namespace

[maciejhirsz]

Describe the Bug

I have the following imports in my Rust code:

#[wasm_bindgen(js_namespace = document, js_name = createTextNode)]
pub(crate) fn text_node(t: &str) -> Node;

#[wasm_bindgen(js_namespace = document, js_name = createTextNode)]
pub(crate) fn text_node_num(t: f64) -> Node;

These follow the examples given in the docs so I don't think I'm doing anything weird.

The codegen for 0.2.93 looks as follows:

imports.wbg.__wbg_createTextNode_9e1923f20a1a5e7c = function(arg0, arg1) {
    const ret = document.createTextNode(getStringFromWasm0(arg0, arg1));
    return addHeapObject(ret);
};
imports.wbg.__wbg_createTextNode_91b927b5f0a34bb9 = function(arg0) {
    const ret = document.createTextNode(arg0);
    return addHeapObject(ret);
};

This works as intended.

Since 0.2.94 got yanked I can't check that, however in 0.2.95 the codegen has changed:

imports.wbg.__wbg_createTextNode_9e1923f20a1a5e7c = function(arg0, arg1) {
    const ret = document.createTextNode(getStringFromWasm0(arg0, arg1));
    return ret;
};
imports.wbg.__wbg_createTextNode_91b927b5f0a34bb9 = typeof document.createTextNode == 'function' ? document.createTextNode : notDefined('document.createTextNode');

Ignoring the change to reference passing (not sure why that happens yet but it's not the issue) the new codegen grabs a reference to the JS function directly without its context, which leads to errors when the function is a method and requires its context on call site.

You can easily reproduce the same error in the browser console:

Steps to Reproduce

Use this example from the docs:

#[wasm_bindgen]
extern "C" {
    #[wasm_bindgen(js_namespace = ["window", "document"])]
    fn write(s: &str);
}

write("hello, document!");

Change the parameter from &str to a primitive such as u32.

Expected Behavior

The codegen should maintain the callsite as it did in 0.2.93, or it should use the .bind method:

imports.wbg.__wbg_createTextNode_91b927b5f0a34bb9 = typeof document.createTextNode == 'function' ? document.createTextNode.bind(document) : notDefined('document.createTextNode');

Additional Context

I'm filing this as a bug since upgrading the version did break my code, however I'm not sure if the thing I'm doing (and the docs in question) really should be the correct way to do this. Having to provide my own little snippets for function calls like that is annoying, but in the long run probably conveys the intent clearer.

I reckon alternative to reverting any changes here would be to change the docs stating that js_namespace shouldn't be used on built-in objects with methods. Sadly JS is extremely inconsistent about this, while you can't grab methods from document without the context, console methods (such as console.log) generally don't care about context at all.

[maciejhirsz]

After filing this I now realize this behavior might have been there long before 0.2.95, but the change to passing things via reference (again not sure why that happens, might be something to do with Trunk) is what enables binding namespaced functions to imports directly.

[RunDevelopment]

I just looked at the code gen for imported JS function and wondered whether binding them as typeof fn == 'function' ? fn : notDefined('fn'); would cause problems. So I guess there is my answer.

I would agree that this code gen is incorrect, because it doesn't capture this.

You suggested fixing this with bind, but I think that even that is incorrect. The fix with bind is only correct if we assume that the namespace will always refer to the same object. If the namespace is an object that changes at runtime, then

imports.wbg.__wbg_fn = typeof obj.fn == 'function' ? obj.fn.bind(fn) : notDefined('obj.fn');

would still behave differently from

imports.wbg.__wbg_fn = function(...args) {
    return obj.fn(...args);
};

[maciejhirsz]

Think you're right. I'd also prefer just wrapping the caller in a function to make the call site explicit. I only suggested bind since that ties in nicer with the ternary expression, but I suppose the ternary is only there to make debugging easier and is not necessary with the function.