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

Replace all instances of Self in methods #3631

Closed
wants to merge 1 commit into from
Closed
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
75 changes: 53 additions & 22 deletions crates/macro-support/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,57 @@ pub(crate) fn is_js_keyword(keyword: &str) -> bool {
JS_KEYWORDS.contains(&keyword)
}

/// Replace instances of `Self` with the actual type `T`
fn replace_self(ty: syn::Type, self_ty: Option<&Ident>) -> syn::Type {
let self_ty = match self_ty {
Some(i) => i,
None => return ty,
};

let mut path = match get_ty(&ty) {
syn::Type::Path(syn::TypePath { qself: None, path }) => path.clone(),
other => return other.clone(),
};
Comment on lines +813 to +816
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should also handle types other than paths.


if path.segments.len() == 1 && path.segments[0].ident == "Self" {
let new_path = self_ty.clone().into();
return syn::Type::Path(syn::TypePath {
qself: None,
path: new_path,
});
}

fn walk_path_change_self(path: &mut syn::Path, self_ty: &Ident) {
for seg in path.segments.iter_mut() {
if let syn::PathArguments::AngleBracketed(ref mut brackets) = seg.arguments {
let types = brackets.args.iter_mut().filter_map(|arg| match arg {
syn::GenericArgument::Type(ref mut ty) => Some(ty),
_ => None,
});
for ty in types {
match ty {
syn::Type::Path(syn::TypePath { ref mut path, .. }) => {
if path.segments.len() == 1 && path.segments[0].ident == "Self" {
path.segments[0].ident = self_ty.clone();
} else {
walk_path_change_self(path, self_ty)
}
}
Comment on lines +835 to +841
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should handle types other than paths too. For example, Box<[Self]> is still broken because Type::Slice is ignored.

Copy link
Contributor Author

@snOm3ad snOm3ad Sep 23, 2023

Choose a reason for hiding this comment

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

Good catch! Yeah I'll try to handle all possible types. AFAIK references are not allowed in return types, but I did noticed this function is also called for types in arguments which do allow references. So maybe I'll handle function args differently.

_ => {}
}
}
}
}
}

let path_str = quote::quote! { #path }.to_string();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let path_str = quote::quote! { #path }.to_string();
let path_str = path.to_token_stream().to_string();

if path_str.contains("Self") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we just call walk_path_change_self() and let it just do it's thing anyway? I'm assuming to_token_stream() and to_string() have to go through the Path to build it anyway, so it wouldn't be more expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah now that you mentioned it, seems like there is no way around it. Will push an update later today!

walk_path_change_self(&mut path, self_ty);
}

syn::Type::Path(syn::TypePath { qself: None, path })
}

/// 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(
Expand All @@ -829,26 +880,6 @@ fn function_from_decl(

let syn::Signature { inputs, output, .. } = sig;

let replace_self = |t: syn::Type| {
let self_ty = match self_ty {
Some(i) => i,
None => return t,
};
let path = match get_ty(&t) {
syn::Type::Path(syn::TypePath { qself: None, path }) => path.clone(),
other => return other.clone(),
};
let new_path = if path.segments.len() == 1 && path.segments[0].ident == "Self" {
self_ty.clone().into()
} else {
path
};
syn::Type::Path(syn::TypePath {
qself: None,
path: new_path,
})
};

let replace_colliding_arg = |i: &mut syn::PatType| {
if let syn::Pat::Ident(ref mut i) = *i.pat {
let ident = i.ident.to_string();
Expand All @@ -864,7 +895,7 @@ fn function_from_decl(
.filter_map(|arg| match arg {
syn::FnArg::Typed(mut c) => {
replace_colliding_arg(&mut c);
c.ty = Box::new(replace_self(*c.ty));
c.ty = Box::new(replace_self(*c.ty, self_ty));
Some(c)
}
syn::FnArg::Receiver(r) => {
Expand All @@ -886,7 +917,7 @@ fn function_from_decl(

let ret = match output {
syn::ReturnType::Default => None,
syn::ReturnType::Type(_, ty) => Some(replace_self(*ty)),
syn::ReturnType::Type(_, ty) => Some(replace_self(*ty, self_ty)),
};

let (name, name_span, renamed_via_js_name) = if let Some((js_name, js_name_span)) =
Expand Down
28 changes: 28 additions & 0 deletions tests/wasm/codegen.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
//! This tests that the `wasm_bindgen` macro produces code that compiles for these use cases.
//! `cargo test --target wasm32-unknown-unknown` will not run if any of these tests breaks.
use wasm_bindgen::prelude::*;

macro_rules! my_export {
($i: ident, $s: ty) => {
#[wasm_bindgen]
pub fn $i(_: $s) {}
};
}

my_export!(should_compile, &[i32]);

#[wasm_bindgen]
pub struct A;

#[wasm_bindgen]
impl A {
pub fn test_only_self() -> Self {
A
}
pub fn test_option_self() -> Option<Self> {
None
}
pub fn test_nested_self() -> Result<Option<Self>, String> {
Ok(None)
}
}
12 changes: 0 additions & 12 deletions tests/wasm/macro_rules.rs

This file was deleted.

2 changes: 1 addition & 1 deletion tests/wasm/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pub mod bigint;
pub mod char;
pub mod classes;
pub mod closures;
pub mod codegen;
pub mod comments;
pub mod duplicate_deps;
pub mod duplicates;
Expand All @@ -35,7 +36,6 @@ pub mod js_keywords;
pub mod js_objects;
pub mod jscast;
pub mod link_to;
pub mod macro_rules;
pub mod math;
pub mod no_shims;
pub mod node;
Expand Down
Loading