Import "new" becomes "_new"

[alockey]

I am trying to import window.__TAURI__.menu.Menu.new from Tauri.

#[wasm_bindgen(js_namespace = ["window", "__TAURI__", "menu"])]
extern "C" {
    #[wasm_bindgen]
    pub type Menu;

    #[wasm_bindgen(static_method_of = Menu)]
    pub async fn new(options: JsValue) -> Menu;
}

The generated JS is:

imports.wbg.__wbg_new_bcb7a76eea7b9ef8 = function() { return logError(function (arg0) {
    const ret = window.__TAURI__.menu.Menu._new(arg0);
    return ret;
}, arguments) };

new has become _new, which does not exist. Using js_name = "new" does not affect the generated JS. I have made an ugly workaround by aliasing _new to new.

Presumably this behaviour is deliberate because new is a JS keyword. However, it is also a valid function name ("valid" != "good").

Would it be possible to force the js_name to be retained, even for keywords? Maybe something like force_js_name = true to avoid breaking any existing code that relies on this? I have browsed around the code a bit but I can't find where this substitution is made. Are other keywords treated similarly?

Alternatively, it would be useful if the guide mentioned this behaviour. It took me ages to figure out what was going on. I can have a go at making that change myself if needed.

[daxpedda]

This is similar to #3929, which was resolved in #3930. I'm unsure about the exact details where new is allowed in JS and where it isn't.

I'm happy to review a fix.

[RunDevelopment]

default is currently handled as a special case to allow for default exports. So #3929 and #3930 have nothing to do with this issue.

The underlying problem is that WBG currently escapes(?) keywords by prepending an underscore. This behavior is just completely broken. The only reason we have it is that not doing it would generate syntactically incorrect JS. The proper fix should have been to either never allow JS keywords as identifiers and make the proc macro error, or to handle keyword identifiers properly in JS. Yes, it turns out, JS can export values as keyword identifiers without issue. Example:

function foo() {}
export { foo as class, foo as new, foo as function }

For imports, the situation is different. If the imported value is NOT namespaced, then it cannot be a keyword. E.g. the following makes no sense:

#[wasm_bindgen]
extern "C" {
    // It's impossible to have a variable called "class" in JS. This binding cannot work. Never.
    #[wasm_bindgen]
    fn class();
}

However, things change when we talk about namespaced values. Then only the first identifier of the namespace cannot be a keyword. The rest is fair game. Example:

// This is completely fine. The function call `not_a_keyword.class.new.var.function()` is valid JS.
#[wasm_bindgenjs_namespace = ["not_a_keyword", "class", "new", "var"])]
extern "C" {
    #[wasm_bindgen]
    fn function();
}

---

So my suggested course of action would be to properly fix keywords for imports now, and discuss how we want to handle exports later.

[daxpedda]

default is currently handled as a special case to allow for default exports. So #3929 and #3930 have nothing to do with this issue.

I was talking about how this issue was caused. Internally, in WBG the same code caused the same issue:

wasm-bindgen/crates/macro-support/src/parser.rs

Lines 22 to 45 in 94b2dc6

	/// Javascript keywords which are not keywords in Rust.
	const JS_KEYWORDS: [&str; 20] = [
	"class",
	"case",
	"catch",
	"debugger",
	"default",
	"delete",
	"export",
	"extends",
	"finally",
	"function",
	"import",
	"instanceof",
	"new",
	"null",
	"switch",
	"this",
	"throw",
	"var",
	"void",
	"with",
	];
	#[derive(Default)]

#3930 implemented a way to skip certain keywords, in that case default. new is in that list as well.

Or is there some other part of the code that is responsible for doing the renaming to _new I'm not aware of?

So my suggested course of action would be to properly fix keywords for imports now, and discuss how we want to handle exports later.

Agreed.

---

I want to note that this is actually a breaking change. But we have historically made the same kind of breaking change already in #3930, even if that was about exports and this is about imports.

I'm not very concerned about the breaking change because I'm predicting that nobody actually relies on such subtle and unintended behavior. But I might be wrong!

[alockey]

These keywords are "never allowed as identifiers" according to the ECMAScript language specification. But it seems browsers don't care so much (or Tauri, unfortunately for me!).

We don't want to generate invalid ECMAScript. Though function_from_decl currently allows keywords in method names (lines 1051 & 1061). It seems this currently could generate invalid ECMAScript.

I suggest the following logic:

if prefix_is_empty && is_js_keyword {
    if not OutputMode::Web {
        bail!("keyword cannot be identifier");
    }
    if opts.js_namespace().is_none() {
        bail!("keyword cannot be identifier outside namespace");
    } 
}

Nothing is ever escaped. Keywords are only valid inside namespaces on web. Methods are treated the same as other functions.

I have had a go at fixing this myself... but I went down several rabbit holes and got lost. I probably also broke exports. I'm happy to help but I will need time to understand the code better (and github - it's my first time).