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

fix the view macro being unhygienic and not importing ElementChild #3162

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jaskij
Copy link

@jaskij jaskij commented Oct 25, 2024

Should fix #3161 .

Using the same code, the expansion now properly imports ElementChild.

A proper solution should probably import this only if necessary, but I have no idea how to do that, this is my first time actually working on proc macros.

use leptos::{view, prelude::ClassAttribute, IntoView};
// this is not referenced directly anywhere
//use leptos::prelude::ElementChild;

#[allow(non_snake_case)]
pub fn BadView(title: String, body: String) -> impl IntoView {
    view! {
        <div class="content"><hgroup>
            <h1>{title}</h1>
        </hgroup></div>
        <div class="content">{body}</div>
    }
}
#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
use leptos::{view, prelude::ClassAttribute, IntoView};
#[allow(non_snake_case)]
pub fn BadView(title: String, body: String) -> impl IntoView {
    {
        #[allow(unused_braces)]
        {
            use ::leptos::prelude::ElementChild;
            ::leptos::prelude::View::new((
                ::leptos::tachys::html::element::div()
                    .child(
                        #[allow(unused_braces)]
                        {
                            ::leptos::tachys::html::element::hgroup()
                                .child(
                                    #[allow(unused_braces)]
                                    {
                                        ::leptos::tachys::html::element::h1()
                                            .child(
                                                #[allow(unused_braces)]
                                                { ::leptos::prelude::IntoRender::into_render({ title }) },
                                            )
                                    },
                                )
                        },
                    )
                    .class("content"),
                ::leptos::tachys::html::element::div()
                    .child(
                        #[allow(unused_braces)]
                        { ::leptos::prelude::IntoRender::into_render({ body }) },
                    )
                    .class("content"),
            ))
        }
    }
}

@CorvusPrudens
Copy link
Contributor

While this approach would solve the hygiene problem for any code outside the view macro, anything inside would be exposed.

To resolve this issue, you'd need to use fully qualified syntax for every trait method inside the macro. As an example, this function:

pub fn simple_counter(initial_value: i32, step: i32) -> impl IntoView {
    let (value, set_value) = signal(initial_value);

    view! {
        <div>
            <span>"Value: " {value} "!"</span>
            <button on:click=move |_| set_value.update(|value| *value += step)>"+1"</button>
        </div>
    }
}

Which normally expands to:

pub fn simple_counter(initial_value: i32, step: i32) -> impl IntoView {
    let (value, set_value) = signal(initial_value);

    {
        #[allow(unused_braces)]
        {
            ::leptos::prelude::View::new(
                ::leptos::tachys::html::element::div().child((
                    ::leptos::tachys::html::element::span().child((
                        "Value: ",
                        ::leptos::prelude::IntoRender::into_render({ value }),
                        "!",
                    )),
                    ::leptos::tachys::html::element::button()
                        .child(
                            #[allow(unused_braces)]
                            {
                                "+1"
                            },
                        )
                        .on(::leptos::tachys::html::event::click, move |_| {
                            set_value.update(|value| *value += step)
                        }),
                )),
            )
        }
    }
}

Would instead need to look like:

pub fn simple_counter(initial_value: i32, step: i32) -> impl IntoView {
    let (value, set_value) = signal(initial_value);

    {
        #[allow(unused_braces)]
        {
            ::leptos::prelude::View::new(
                <_ as ::leptos::prelude::ElementChild<_>>::child(
                    ::leptos::tachys::html::element::div(),
                    (
                        <_ as ::leptos::prelude::ElementChild<_>>::child(
                            ::leptos::tachys::html::element::span(),
                            (
                                "Value: ",
                                ::leptos::prelude::IntoRender::into_render({ value }),
                                "!",
                            ),
                        ),
                        <_ as ::leptos::prelude::OnAttribute<_, _>>::on(
                            <_ as ::leptos::prelude::ElementChild<_>>::child(
                                ::leptos::tachys::html::element::button(),
                                #[allow(unused_braces)]
                                {
                                    "+1"
                                },
                            ),
                            ::leptos::tachys::html::event::click,
                            move |_| set_value.update(|value| *value += step),
                        ),
                    ),
                )
            )
        }
    }
}

This is a little unfortunate for two reasons:

  1. It makes the expanded code much harder to read since trait-method-chaining becomes unavailable. For users looking to write views without macros, this can make getting started a bit harder. It may also make Leptos's maintenance a little harder.

  2. It produces a fair bit more tokens. I don't know if this would have any significant effect on compile times, but it's something to consider.

Personally, I think it would be very nice to have perfectly hygienic view macros. It won't come for free, though!

@gbj
Copy link
Collaborator

gbj commented Oct 25, 2024

A few observations:

  1. Agreed that using the expanded form rather than the method-chaining form for all methods used in the macro would be the "hygienic" way to do this.
  2. Agreed that there are real drawbacks to using that expanded form in the macro.
  3. I tested it out, and if I delete the prelude import from any given example, Rust correctly suggests 100% of the missing imports. Most of them (the ones in user code inside the macro) can be automatically imported in my editor via code actions from rust-analyzer. Others (the ones used in the macro code itself like .child()), Rust gives accurate suggestions on how to import them.

Personally, I think the trade-off here (readability + token length of macro expansion vs. manual imports if you don't want to use a prelude) is currently well-balanced. If the two user stories are "I import the prelude and it just works" and "I don't import the prelude and the compiler tells me exactly what to manually import," I am pretty comfortable.

But I am open to arguments against that.

@jaskij
Copy link
Author

jaskij commented Oct 26, 2024

Regarding stuff that's explicitly used inside the macro, like ClassAttribute, yes, those should be left to the user. RustRover (about the only IDE not using rust-analyzer) does correctly suggest the imports.

If that's your decision, I'm fine with dropping the topic.

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

Successfully merging this pull request may close these issues.

[0.7] view! is unhygienic
3 participants