Replace all instances of Self in methods
#3631
Conversation
Currently the `wasm_bindgen` macro only replaces top level occurrences of the generic parameter `Self` in exported methods. This leads to an error when returning parametrized types containing `Self` due to Rust inner items rules. This PR fixes the issue by drilling down the return type `TokenStream` and replacing all instances of `Self` with the actual type name. Signed-off-by: Oliver T <[email protected]>
| } | ||
| } | ||
|
|
||
| let path_str = quote::quote! { #path }.to_string(); |
There was a problem hiding this comment.
| let path_str = quote::quote! { #path }.to_string(); | |
| let path_str = path.to_token_stream().to_string(); |
| } | ||
|
|
||
| let path_str = quote::quote! { #path }.to_string(); | ||
| if path_str.contains("Self") { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah now that you mentioned it, seems like there is no way around it. Will push an update later today!
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
This should handle types other than paths too. For example, Box<[Self]> is still broken because Type::Slice is ignored.
There was a problem hiding this comment.
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 mut path = match get_ty(&ty) { | ||
| syn::Type::Path(syn::TypePath { qself: None, path }) => path.clone(), | ||
| other => return other.clone(), | ||
| }; |
There was a problem hiding this comment.
This should also handle types other than paths.
|
I was also thinking of turning the type into a |
I guess this could work and would obviously simplify the code a lot. The only thing I can think of that could use the word "Self" in a type is a macro or a const generic, Rust only supports integers right now. My impression is that the additional code required to cover the remaining bases shouldn't be too bad. I'm not too worried about what else Rust might add in the future, it would be most likely a breaking change in |
That would also catch types that contain |
That's so sad! Oh well, will try to have an update on this later today. Thank you both for your comments! |
Currently the
wasm_bindgenmacro only replaces top level occurrences of the generic parameterSelfin exported methods. This leads to an error when returning parametrized types containingSelfdue to Rust inner items rules.This PR fixes the issue by drilling down the return type
TokenStreamand replacing all instances ofSelfwith the actual type name.Fixes #3105