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

Add expression interpolation with #{expr} #88

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Collaborator

One annoyance I frequently run into with the quote! macro is that I
need to lift out all variable bindings that are being interpolated. For
example I'll often do:

let name = &foo.name;
let abi = &foo.abi;

quote! {
    extern #abi fn #name() { /* ... */ }
}

but I've recently had the idea that in addition to lighteight #name
interpolation there's also available syntax (I believe) to support
expression interpolation as well, implemented in this PR. The above
snippet could now be rewritten as:

quote! {
    extern #{foo.abi} fn #{foo.name} { /* ... */ }
}

@hcpl
Copy link
Contributor

hcpl commented Nov 16, 2018

There were several attempts to solve this problem before: #30, #32 and #83 but none were accepted. I think it's a good idea to explain how this PR is better (I can see that this is pretty minimalistic compared to previous ones) if you want to get this merged.

One annoyance I frequently run into with the `quote!` macro is that I
need to lift out all variable bindings that are being interpolated. For
example I'll often do:

    let name = &foo.name;
    let abi = &foo.abi;

    quote! {
        extern #abi fn #name() { /* ... */ }
    }

but I've recently had the idea that in addition to lighteight `#name`
interpolation there's also available syntax (I believe) to support
expression interpolation as well, implemented in this PR. The above
snippet could now be rewritten as:

    quote! {
        extern #{foo.abi} fn #{foo.name} { /* ... */ }
    }
@alexcrichton
Copy link
Collaborator Author

Ah thanks for the pointer, I was unaware of those!

Reading over those I personally feel like the main use case for this is what was outlined here, accessing fields. I do that so often in so many quote impls that being able to do that would be quite beneficial.

I will admit though that I haven't considered the repetition case at all here. I try to avoid using repetitions as they've always felt a little off to me running into issues like #7 and such.

I would personally advocate for considering this. From my (likely naive) point of view this seems to be a simple enough addition that it is lightweight enough to add and is worth it. It sounds like @dtolnay like you're also worried about misuse causing hard-to-read code, and I definitely agree that's possible. I don't think that'd persuade me, though, to think this shouldn't be added on that basis.

@alexcrichton
Copy link
Collaborator Author

ping @dtolnay, do you have thoughts on this perhaps?

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks for the reminder. I continue to believe that we should not support this.

I do that so often in so many quote impls that being able to do that would be quite beneficial.

I know you have been working on wasm-bindgen so I found impl ToTokens for ast::StructField as one seemingly typical instance of this pattern.

//! current code

let name = &self.name;
let struct_name = &self.struct_name;
let ty = &self.ty;
let getter = &self.getter;
quote! {
    #[no_mangle]
    #[doc(hidden)]
    #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))]
    pub unsafe extern "C" fn #getter(js: u32)
        -> <#ty as ::wasm_bindgen::convert::IntoWasmAbi>::Abi
    {
        use wasm_bindgen::__rt::{WasmRefCell, assert_not_null};
        use wasm_bindgen::convert::{GlobalStack, IntoWasmAbi};

        fn assert_copy<T: Copy>(){}
        assert_copy::<#ty>();

        let js = js as *mut WasmRefCell<#struct_name>;
        assert_not_null(js);
        let val = (*js).borrow().#name;
        <#ty as IntoWasmAbi>::into_abi(
            val,
            &mut GlobalStack::new(),
        )
    }
}

It seems highly likely that you would have used #{self.var} in this code if the feature existed. The thing is, I do not consider this an improvement. As a person reading your code, I would much prefer to read the original version.

//! proposed

quote! {
    #[no_mangle]
    #[doc(hidden)]
    #[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))]
    pub unsafe extern "C" fn #{self.getter}(js: u32)
        -> <#{self.ty} as ::wasm_bindgen::convert::IntoWasmAbi>::Abi
    {
        use wasm_bindgen::__rt::{WasmRefCell, assert_not_null};
        use wasm_bindgen::convert::{GlobalStack, IntoWasmAbi};

        fn assert_copy<T: Copy>(){}
        assert_copy::<#{self.ty}>();

        let js = js as *mut WasmRefCell<#{self.struct_name}>;
        assert_not_null(js);
        let val = (*js).borrow().#{self.name};
        <#{self.ty} as IntoWasmAbi>::into_abi(
            val,
            &mut GlobalStack::new(),
        )
    }
}

If the local variables are named the same as fields of self, like here, then skimming them does not hurt my ability to understand the code. The extra noise and punctuation and nesting and compile-time-vs-runtime dissonance inside the quote! invocation does though.

And in cases where the local variable would be named differently from the field (like you have let import_name = &self.shim later in the file), the new name gives an opportunity to attach context that makes the application of that field inside quote! easier to understand.

Keep in mind that complexity inside of a quote! invocation is far more expensive than a similar amount of complexity outside of an invocation.

@alexcrichton
Copy link
Collaborator Author

I found impl ToTokens for ast::StructField as one seemingly typical instance of this pattern.

That's all spot on!

Keep in mind that complexity inside of a quote! invocation is far more expensive than a similar amount of complexity outside of an invocation.

This is an interesting point I think. I believe I was largely struct with inspiration from this when I saw the typed-html macro crop up recently and it used interpolation for Rust expressions, and I thought "oh right I would use that a lot for quoting!". I figured that many interpolation syntaxes support expression-based interpolation (like JS strings's "${foo}").

Thinking more, though, this point about expressivity I think is key. In a string or in something like HTML the surrounding "stuff" is entirely different from what you're interpolating. With quote!, however, it's the exact same stuff as what you're interpolating with. For strings this means it's pretty easy (especially with syntax highlighting) to find what's interpolated, and even with HTML/Rust it's pretty easy to visually distinguish the two. With quote! and interpolated code, it can be confusing to follow as because both are so visually similar.

That all makes sense to me, and as I was unaware of the rejected proposals before I'm gonna go ahead and close this. Rationale makes sense to me!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants