Skip to content

#2806 bugfix: Added checks for JS keywords in signatures. #2855

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

Merged
merged 2 commits into from
Jun 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 63 additions & 13 deletions crates/macro-support/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,29 @@ use syn::spanned::Spanned;

thread_local!(static ATTRS: AttributeParseState = Default::default());

/// 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)]
struct AttributeParseState {
parsed: Cell<usize>,
Expand Down Expand Up @@ -462,6 +485,7 @@ impl<'a> ConvertToAst<(BindgenAttrs, &'a ast::ImportModule)> for syn::ForeignIte
self.vis.clone(),
false,
None,
false,
)?
.0;
let catch = opts.catch().is_some();
Expand Down Expand Up @@ -704,13 +728,19 @@ impl ConvertToAst<BindgenAttrs> for syn::ItemFn {
self.vis,
false,
None,
false,
)?;
attrs.check_used()?;
Ok(ret.0)
}
}

pub(crate) fn is_js_keyword(keyword: &str) -> bool {
JS_KEYWORDS.contains(&keyword)
}

/// Construct a function (and gets the self type if appropriate) for our AST from a syn function.
#[allow(clippy::too_many_arguments)]
fn function_from_decl(
decl_name: &syn::Ident,
opts: &BindgenAttrs,
Expand All @@ -719,6 +749,7 @@ fn function_from_decl(
vis: syn::Visibility,
allow_self: bool,
self_ty: Option<&Ident>,
is_from_impl: bool,
) -> Result<(ast::Function, Option<ast::MethodSelf>), Diagnostic> {
if sig.variadic.is_some() {
bail_span!(sig.variadic, "can't #[wasm_bindgen] variadic functions");
Expand Down Expand Up @@ -754,11 +785,21 @@ fn function_from_decl(
})
};

let replace_colliding_arg = |i: &mut syn::PatType| {
if let syn::Pat::Ident(ref mut i) = *i.pat {
let ident = i.ident.to_string();
if is_js_keyword(ident.as_str()) {
i.ident = Ident::new(format!("_{}", ident).as_str(), i.ident.span());
}
}
};

let mut method_self = None;
let arguments = inputs
.into_iter()
.filter_map(|arg| match arg {
syn::FnArg::Typed(mut c) => {
replace_colliding_arg(&mut c);
c.ty = Box::new(replace_self(*c.ty));
Some(c)
}
Expand All @@ -784,21 +825,29 @@ fn function_from_decl(
syn::ReturnType::Type(_, ty) => Some(replace_self(*ty)),
};

let (name, name_span, renamed_via_js_name) =
if let Some((js_name, js_name_span)) = opts.js_name() {
let kind = operation_kind(&opts);
let prefix = match kind {
OperationKind::Setter(_) => "set_",
_ => "",
};
(
format!("{}{}", prefix, js_name.to_string()),
js_name_span,
true,
)
let (name, name_span, renamed_via_js_name) = if let Some((js_name, js_name_span)) =
opts.js_name()
{
let kind = operation_kind(opts);
let prefix = match kind {
OperationKind::Setter(_) => "set_",
_ => "",
};
let name = if prefix.is_empty() && opts.method().is_none() && is_js_keyword(js_name) {
format!("_{}", js_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not silently replace the name of exported and imported identifiers where they make a semantic difference. Accessing them by bracket instead of dot syntax sounds like a reasonable workaround if the user actually does use a js keyword. I.e. wasm["class"] instead of silently changing to wasm._class against explicit annotation in

https://github.com/rustwasm/wasm-bindgen/pull/2855/files#diff-f7d340b325e0e9f30bd18d30036698d1edfece6974682dd3480457e4383806d3R17

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a reasonable solution that should be easy to implement, would you like to make a PR? Happy to review it.

} else {
(decl_name.to_string(), decl_name.span(), false)
format!("{}{}", prefix, js_name)
};
(name, js_name_span, true)
} else {
let name =
if !is_from_impl && opts.method().is_none() && is_js_keyword(&decl_name.to_string()) {
format!("_{}", decl_name)
} else {
decl_name.to_string()
};
(name, decl_name.span(), false)
};
Ok((
ast::Function {
arguments,
Expand Down Expand Up @@ -1054,6 +1103,7 @@ impl<'a, 'b> MacroParse<(&'a Ident, &'a str)> for &'b mut syn::ImplItemMethod {
self.vis.clone(),
true,
Some(class),
true,
)?;
let method_kind = if opts.constructor().is_some() {
ast::MethodKind::Constructor
Expand Down
24 changes: 24 additions & 0 deletions tests/wasm/js_keywords.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
const wasm = require("wasm-bindgen-test.js");
const assert = require("assert");

exports.js_keywords_compile = () => {
assert.strictEqual(wasm._throw(1), 1);
assert.strictEqual(wasm._class(1, 2), false);
assert.strictEqual(wasm.classy(3), 3);
let obj = new wasm.Class("class");
assert.strictEqual(wasm.Class.void("string"), "string");
assert.strictEqual(obj.catch, "class");
assert.strictEqual(obj.instanceof("Class"), "class is instance of Class");
};

exports.test_keyword_1_as_fn_name = (x) => {
return wasm._throw(x);
};

exports.test_keyword_2_as_fn_name = (x, y) => {
return wasm._class(x, y);
};

exports.test_keyword_as_fn_arg = (x) => {
return wasm.classy(x);
};
55 changes: 55 additions & 0 deletions tests/wasm/js_keywords.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
use wasm_bindgen::prelude::*;
use wasm_bindgen_test::*;

#[wasm_bindgen(module = "tests/wasm/js_keywords.js")]
extern "C" {
fn js_keywords_compile();
fn test_keyword_1_as_fn_name(x: u8) -> u8;
fn test_keyword_2_as_fn_name(x: u8, y: u8) -> bool;
fn test_keyword_as_fn_arg(x: u8) -> u8;
}

#[wasm_bindgen]
pub fn throw(class: u8) -> u8 {
class
}

#[wasm_bindgen(js_name = class)]
pub fn fn_parsed_to_keyword(instanceof: u8, catch: u8) -> bool {
instanceof > catch
}

#[wasm_bindgen(js_name = classy)]
pub fn arg_is_keyword(class: u8) -> u8 {
class
}

#[wasm_bindgen]
struct Class {
name: String,
}
#[wasm_bindgen]
impl Class {
#[wasm_bindgen(constructor)]
pub fn new(void: String) -> Self {
Class { name: void }
}
pub fn instanceof(&self, class: String) -> String {
format!("{} is instance of {}", self.name.clone(), class)
}
#[wasm_bindgen(getter)]
pub fn catch(&self) -> String {
self.name.clone()
}
pub fn void(void: String) -> String {
void
}
}

#[wasm_bindgen_test]
fn compile() {
js_keywords_compile();
assert_eq!(test_keyword_1_as_fn_name(1), 1);
assert_eq!(test_keyword_2_as_fn_name(1, 2), false);
assert_eq!(test_keyword_as_fn_arg(1), 1);
}
1 change: 1 addition & 0 deletions tests/wasm/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ pub mod futures;
pub mod getters_and_setters;
pub mod import_class;
pub mod imports;
pub mod js_keywords;
pub mod js_objects;
pub mod jscast;
pub mod math;
Expand Down